Bug 40779 – [Chromium] Perf regression following landing of HTML5 parser

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andrei Popescu]

A new HTML 5 parser landed in WebKit in r61234:

http://trac.webkit.org/changeset/61234

This new parser was turned on in Chromium in

http://src.chromium.org/viewvc/chrome?view=rev&revision=49936

Ever since that change, the Vista and OSX 10.6 perf bots started failing. The Vista bot consistently fails, while the OSX 10.6 one is flaky:

http://build.chromium.org/buildbot/waterfall/builders/Mac10.6%20Perf(1)/builds/4306
http://build.chromium.org/buildbot/waterfall/builders/Vista%20Perf%20(1)/builds/5397

See also the first regression in the following graph:

http://build.chromium.org/buildbot/perf/vista-release-dual-core/intl1/report.html?history=150&rev=-1

[Tony Gentilcore]

Adding some more data to the bug.

First, this only seems to happen on the Vista cycler (XP is fine).

Second, the regression seems to be due to a few outliers, rather than a consistent move across all runs (see below). This is a good thing as it means it is probably a bug rather than something inherently slower about the new parser.

Sample run times:
[68,65,149,41,49,60,14,183,49,139,104,114,230,140,90,224,84,70,191,38,65,69,102,209,77,53,148,137,95,28,43365,39,61398,63,129,170,98,131,38,168,85,43696,84,203,220,143,93,102,403,199,258,306,210,294,122,312,27,10,74,27,24,47,11,111,28,89,69,67,92,64,56,122,72,49,119,30,40,43,69,129,41,34,88,98,76,18,108,48,215,50,76,115,76,78,31,80,47,21389,64,149,168,115,72,60,280,140,216,214,168,214,90,231,28,10,74,28,25,44,10,112,30,90,70,68,96,63,58,123,71,45,119,28,39,40,72,132,41,34,83,97,76,18,108,48,214,52,76,118,75,75,30,79,47,79,65,147,167,117,61,60,278,137,221,203,157,206,89,227,28,11,74,28,25,43,9,112,28,90,69,67,92,64,58,122,69,45,121,28,38,40,70,131,41,34,83,122,77,17,109,48,215,51,77,117,76,77,31,79,47,79,64,148,166,114,61,60,279,135,218,199,175,206,88,229,27,10,73,28,25,44,10,111,29,90,69,70,92,66,58,125,69,45,118,28,38,40,72,130,41,35,83,121,74,18,111,29,216,52,76,116,74,76,31,81,47,79,64,148,167,114,62,60,282,139,231,205,165,206,89,229,28,10,74,28,26,44,10,113,29,90,70,68,96,65,59,123,71,59,129,28,40,41,74,133,42,35,84,97,76,19,110,29,217,53,78,117,76,76,31,80,47,80,67,149,167,114,61,60,283,139,221,207,158,209,89,228,27,13,74,29,26,45,10,113,30,91,69,68,93,63,59,125,70,46,120,27,39,40,73,130,42,37,84,123,75,18,110,47,217,52,77,117,75,76,31,79,47,80,65,150,167,115,62,60,284,139,221,204,161,207,91,231,29,12,77,29,25,44,10,117,29,90,69,69,93,64,60,124,71,46,120,29,39,43,73,132,42,35,84,97,75,18,110,49,218,52,77,118,75,76,32,81,48,80,64,148,167,119,62,61,284,140,222,203,160,208,90,231,28,12,74,29,28,44,10,113,30,92,70,69,94,64,59,125,70,46,122,28,39,41,74,131,43,35,84,98,79,18,43384,26,219,52,77,117,76,76,32,81,48,82,64,149,169,115,62,60,285,140,221,205,162,206,89,231,28,11,74,29,25,44,10,116,28,90,69,69,93,65,60,123,70,46,118,28,39,41,73,130,42,35,84,97,78,19,111,48,220,53,77,118,75,74,31,83,46,80,67,149,168,114,63,60,286,140,223,206,160,217,93,229] ms


Reference run times:
[679,87,163,46,62,68,17,207,53,154,112,113,269,144,85,259,89,101,199,72,71,66,111,234,56,36,124,105,73,22,163,56,329,64,157,148,79,148,31,144,70,82,81,215,229,153,105,104,375,204,462,328,182,451,124,269,37,13,77,31,29,53,12,117,33,102,75,74,103,64,63,127,75,55,133,32,43,43,82,150,34,34,85,85,67,15,108,25,194,54,79,95,66,68,29,72,49,73,58,146,168,107,69,66,329,124,261,227,206,205,81,201,28,13,75,29,28,48,14,109,34,93,72,72,108,66,61,133,75,49,132,31,43,43,80,153,45,39,99,136,68,20,109,26,203,56,81,115,64,69,28,71,42,72,58,150,152,99,53,56,278,146,261,222,183,251,96,235,34,13,77,31,29,49,12,108,34,103,75,74,107,66,61,125,79,54,134,32,43,43,83,153,37,33,84,84,68,15,109,31,222,50,69,120,67,76,33,80,49,93,71,162,159,120,69,62,334,124,222,188,185,196,75,211,30,12,79,32,28,50,12,116,35,102,76,74,104,65,64,126,68,52,121,29,36,37,68,127,43,41,99,133,85,24,125,31,230,55,81,95,67,66,29,72,47,74,60,147,151,101,69,65,335,119,222,201,197,216,94,220,34,12,77,29,29,49,13,116,33,91,76,78,105,69,60,129,74,51,130,56,44,43,84,159,41,32,83,85,70,22,110,46,199,55,82,121,66,70,29,72,44,74,59,152,166,125,54,57,280,134,265,222,208,225,99,237,34,10,72,27,24,43,10,109,33,103,76,76,105,67,64,125,79,50,135,27,36,37,68,128,46,40,100,136,68,15,108,50,217,55,69,124,79,77,34,80,50,87,72,166,162,99,55,55,278,153,264,223,185,220,94,235,36,14,77,27,24,44,10,117,35,104,76,75,104,64,65,128,76,53,134,27,35,38,68,128,44,41,100,136,84,24,125,31,233,55,73,86,80,76,34,82,53,90,71,169,167,99,55,58,280,116,223,202,219,202,81,195,28,13,75,30,29,47,15,119,36,96,72,73,104,68,62,132,74,50,131,32,43,44,85,152,37,32,84,84,71,20,110,25,209,49,81,114,66,70,29,73,48,73,61,151,152,104,55,57,281,121,223,195,171,194,80,216,28,10,72,27,23,45,11,109,28,103,76,75,104,67,65,126,77,50,134,32,44,44,85,148,37,33,85,86,68,15,122,38,242,56,70,125,82,77,34,81,51,87,72,163,144,119,55,55,280,150,265,229,168,218,95,236] ms

Notice that in the reference run, all times are 3 digits. In the sample run, most times seem to line up with the occasional 5 digit time.

[Adam Barth]

(In reply to comment #0)
> Ever since that change, the Vista and OSX 10.6 perf bots started failing. The Vista bot consistently fails, while the OSX 10.6 one is flaky:
> 
> http://build.chromium.org/buildbot/waterfall/builders/Mac10.6%20Perf(1)/builds/4306

http://build.chromium.org/buildbot/perf/mac-release-10.6/morejs/report.html?history=150&rev=50051

http://build.chromium.org/buildbot/perf/mac-release-10.6/moz/report.html?history=150&rev=50051

It's hard to see a regression on these charts, but maybe I'm not great at reading them.

> http://build.chromium.org/buildbot/waterfall/builders/Vista%20Perf%20(1)/builds/5397

Similarly,

http://build.chromium.org/buildbot/perf/vista-release-dual-core/morejs/report.html?history=100&rev=-1

However, intl1 clearly shows something wacky going on:

http://build.chromium.org/buildbot/perf/vista-release-dual-core/intl1/report.html?history=100&rev=-1

[Adam Barth]

> Notice that in the reference run, all times are 3 digits. In the sample run, most times seem to line up with the occasional 5 digit time.

Does that correspond to individual pages that we're bad at?

@tonyg, would you be willing to investigate?

[Tony Gentilcore]

(In reply to comment #3)
> > Notice that in the reference run, all times are 3 digits. In the sample run, most times seem to line up with the occasional 5 digit time.
> 
> Does that correspond to individual pages that we're bad at?

I talked to chase about this. He was unclear if they should line up or not.

One strange thing is that the number of times is not evenly divisible by the number of urls.

> 
> @tonyg, would you be willing to investigate?

I'm trying to investigate now, but haven't gotten any solid leads yet.

[Tony Gentilcore]

(In reply to comment #4)
> (In reply to comment #3)
> > > Notice that in the reference run, all times are 3 digits. In the sample run, most times seem to line up with the occasional 5 digit time.
> > 
> > Does that correspond to individual pages that we're bad at?
> 
> I talked to chase about this. He was unclear if they should line up or not.
> 
> One strange thing is that the number of times is not evenly divisible by the number of urls.
> 
> > 
> > @tonyg, would you be willing to investigate?
> 
> I'm trying to investigate now, but haven't gotten any solid leads yet.

Finally have a solid local repro.

Our local versions of news.163.com and sport24.gr will repro consistently.

I'm still determining where/how we stall.

[Tony Gentilcore]

I've got a test case now. It looks like the bug is caused by an external resource inside a <noscript> tag.

In some of our page cyclers, the paths for resources loaded inside a <noscript> were not re-written to be on the local file system (which is fine). I'm guessing the old preload scanner skipped <noscript> tags whereas the new does not. If so, I'll put together a patch to make the new preload scanner skip <noscript>s.

[Adam Barth]

You can put the lexer into the RCDATA state to make it eat everything in the <noscript> tag.

[Tony Gentilcore]

Created attachment 59042 [details]
Patch

[Adam Barth]

I see.  We're wrong both before and after this patch, we're just changing it to be the most usual case...  Maybe we should just fix the bug?  We need to know whether scripts are enabled to decide whether noscript should enter that state.  (I think frames should always enter because we don't have a way to disable frames.)

[Tony Gentilcore]

Created attachment 59047 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 59047 [details]
Patch

WebCore/html/HTML5TreeBuilder.h:86
 +      static bool isScriptingFlagEnabled(Frame* frame);
No need for the argument name.

WebCore/html/HTML5TreeBuilder.cpp:142
 +      if (tagName == styleTag || tagName == iframeTag || tagName == xmpTag || tagName == noembedTag || tagName == noframesTag || (tagName == noscriptTag && isScriptingFlagEnabled(frame)))
Can we wrap this line? :)

Looks good to me.  I'd like Adam to look too to make sure scriptController->canExecuteScripts(NotAboutToExecuteScript) is the right flavor of "can we script now" call.

[Tony Gentilcore]

Created attachment 59049 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 59049 [details]
Patch

You didn't remove the "frame" argument, but it's not a big deal. :)

[Adam Barth]

Committed r61372: <http://trac.webkit.org/changeset/61372>

[Tony Gentilcore]

I'd like to create a subsequent patch with a LayoutTest for this. The case is something like:

<script src="slow-script-to-pause-parser-long-enough-for-preload-scanner.js"></script>
<noscript>
    <img src="existent-image.png" onload="failTest()">
</noscript>

However, the PreloadScanner (correctly) doesn't trigger onload for the image. So I need another way to determine if the image was fetched (not loaded). Is that possible in a LayoutTest? Any suggestions?

[Adam Barth]

> However, the PreloadScanner (correctly) doesn't trigger onload for the image. So I need another way to determine if the image was fetched (not loaded). Is that possible in a LayoutTest? Any suggestions?

Yes!  Eric and I found a way to test the preloader:

http://trac.webkit.org/browser/trunk/LayoutTests/fast/preloader