Bug 40779 - [Chromium] Perf regression following landing of HTML5 parser
Summary: [Chromium] Perf regression following landing of HTML5 parser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 40849
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-17 08:41 PDT by Andrei Popescu
Modified: 2010-06-18 12:39 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.69 KB, patch)
2010-06-17 15:56 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (5.20 KB, patch)
2010-06-17 16:52 PDT, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (5.20 KB, patch)
2010-06-17 17:04 PDT, Tony Gentilcore
eric: review+
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Popescu 2010-06-17 08:41:38 PDT
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
Comment 1 Tony Gentilcore 2010-06-17 10:18:47 PDT
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.
Comment 3 Adam Barth 2010-06-17 13:10:31 PDT
> 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?
Comment 4 Tony Gentilcore 2010-06-17 13:12:33 PDT
(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.
Comment 5 Tony Gentilcore 2010-06-17 13:37:25 PDT
(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.
Comment 6 Tony Gentilcore 2010-06-17 14:24:29 PDT
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.
Comment 7 Adam Barth 2010-06-17 15:26:43 PDT
You can put the lexer into the RCDATA state to make it eat everything in the <noscript> tag.
Comment 8 Tony Gentilcore 2010-06-17 15:56:43 PDT
Created attachment 59042 [details]
Patch
Comment 9 Adam Barth 2010-06-17 16:02:08 PDT
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.)
Comment 10 Tony Gentilcore 2010-06-17 16:52:02 PDT
Created attachment 59047 [details]
Patch
Comment 11 Eric Seidel (no email) 2010-06-17 16:56:27 PDT
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.
Comment 12 Tony Gentilcore 2010-06-17 17:04:09 PDT
Created attachment 59049 [details]
Patch
Comment 13 Eric Seidel (no email) 2010-06-17 17:25:59 PDT
Comment on attachment 59049 [details]
Patch

You didn't remove the "frame" argument, but it's not a big deal. :)
Comment 14 Adam Barth 2010-06-17 20:40:54 PDT
Committed r61372: <http://trac.webkit.org/changeset/61372>
Comment 15 Tony Gentilcore 2010-06-18 10:46:37 PDT
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?
Comment 16 Adam Barth 2010-06-18 10:49:54 PDT
> 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