Fwd: Expected morejs perf regression with an upcoming change

Martin Kosiba

unread,

Nov 15, 2011, 2:06:16 PM11/15/11

to chromi...@chromium.org, kerz, chase

Hi all,

I'd like to attempt to land http://codereview.chromium.org/8117003/ again. The change was reverted because it introduced a ~10% perf regression to the morejs suite.

Now that I've had some time to investigate the reason for this I'd like to move forward with the change and rebaseline the perf expectations.

The reason the change causes a perf regression lies in this piece of code in RenderViewImpl::didFailProvisionalLoad:

// Don't display an error page if this is simply a cancelled load. Aside

// from being dumb, WebCore doesn't expect it and it will cause a crash.

if (error.reason == net::ERR_ABORTED)

return;

// The code after this loads an error page.

Currently trying to load an unsupported scheme will return in an ERR_ABORTED error (which it shouldn't since ERR_ABORTED means "load cancel"). The reason my change will make the morejs suite run longer is that we'll start showing error pages for iframes that point to a URL with an unsupported schema.

Right now if you go to a link like this you'll get an empty iframe. My change will cause the iframe to display the error page. This does mean iframes that point to URLs that are supported by external applications will show an error page as well. I don't expect this to be an issue, but I'd like to know your thoughts. For comparison, Firefox will show an empty frame and a popup dialog, similarly to what Chromium does today.

Thanks!

Martin

Martin Kosiba

unread,

Nov 15, 2011, 4:08:42 PM11/15/11

to chromi...@chromium.org, kerz, chase

And if you're wondering about the mysterious link here it is: data:text/html,<html><body><iframe src="foo://bar/" /></body></html>

Chase Phillips

unread,

Nov 15, 2011, 5:31:23 PM11/15/11

to Martin Kosiba, chromi...@chromium.org, kerz

Hi Martin, It sounds like there's agreement to make this change in general based on the code review, and from what I can see the resulting regression in morejs would be expected since the data we use to run the test isn't without its own errors (including invalid schemes). Ultimately we should fix this in the page cycler and this requires making a new page cycler based on morejs.

The most important thing from my point of view is that these tests show only expected regressions and improvements, and also alert us to unexpected changes. So if this is an expected regression and we understand why (ie. this would not regress in a version of morejs without the invalid schemes), then I'm inclined to say okay, have you land it, and rebaseline.

The catch: you will still be on the hook if later we discover there is another regression related to this change. But that's the case all the time anyway. Sound good?

Thanks,

Chase

Ojan Vafai

unread,

Nov 15, 2011, 5:35:09 PM11/15/11

to c...@google.com, Martin Kosiba, chromi...@chromium.org, kerz

What unsupported scheme are these pages trying to load? Is the a side-effect of the way we load pages in the page cycler or it is something the pages are actually doing?

If it's the latter, then we'd actually be making these sites slower by fixing this bug. If that's the case, can we fix the bug but still not show an error page in the iframe?

If it's just a side-effect of the page cycler infrastructure, then it's much less concerning and I agree with everything Chase said.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

Tony Chang

unread,

Nov 15, 2011, 5:43:38 PM11/15/11

to ojan+...@google.com, c...@google.com, Martin Kosiba, chromi...@chromium.org, kerz

grepping for :// in the pages bring up schemes "foowoaiefjo://www.google.com/" and "foowoaiefjo://mt.google.com". When importing page cyclers, there's normally a pass where we change external references to a junk URL to avoid hitting the live internet when running the page cyclers.

I think these are safe to ignore since it's not representative of what web pages do in the wild.

Chase Phillips

unread,

Nov 15, 2011, 5:44:06 PM11/15/11

to Ojan Vafai, Martin Kosiba, chromi...@chromium.org, kerz

On Tue, Nov 15, 2011 at 2:35 PM, Ojan Vafai <oj...@chromium.org> wrote:

What unsupported scheme are these pages trying to load?

Here's one example: aowiejfijwe. It's a made-up scheme that was used to avoid loading remote resources when the page cycler was created.

Is the a side-effect of the way we load pages in the page cycler or it is something the pages are actually doing?

The pages actually load these resources themselves via JavaScript. For the morejs page cycler loading these resources is unnecessary in order to stress test the JS code itself and measure start-up time for the JS engine on page load.

If it's the latter, then we'd actually be making these sites slower by fixing this bug. If that's the case, can we fix the bug but still not show an error page in the iframe?

If it's just a side-effect of the page cycler infrastructure, then it's much less concerning and I agree with everything Chase said.

Sounds like we're on the same page, but note I see it as an unintended side-effect of the morejs data set, not a side-effect of the infrastructure itself.

Reply all

Reply to author

Forward