Bug 86165 – HTML parser should yield more to improve perceived page load time

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tony Gentilcore]

Created attachment 141305 [details]
Testcase

The HTML parser has heuristics to yield to the event loop. Mihai P pointed out:
> Based on HTMLParserScheduler::checkForYieldBeforeToken it will yield after 4096 tokens _and_ at least 0.5 seconds have passed.
> There is also HTMLParserScheduler::checkForYieldBeforeScript which makes sure we paint at least once before running scripts.

The attached test case demonstrates 5 separate inline script blocks which each take 1s to execute. Sometimes we'll yield after the first, but we always lock the main thread after that (presumably until 4,096 tokens are reached) . Locking the main thread is bad for obvious reasons (can't paint, schedule resource loads, etc). This example isn't completely contrived, it bit a popular Google property.

I propose we change the logic so that if .5 seconds have passed we always schedule a yield for after the next token is parsed. This will avoid the user ever staring at a blank page for too long. Thoughts? Other heuristic ideas?

[Adam Barth]

That sounds reasonable.  I think we avoid reading the clock on every cycle because that can be time consuming.  It might be worth checking the clock only after scripts or something like that.

[Shrey Banga]

Created attachment 146359 [details]
Patch

[WebKit Review Bot]

Attachment 146359 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Shrey Banga]

Created attachment 146375 [details]
Patch

[Adam Barth]

Comment on attachment 146375 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146375&action=review

> Source/WebCore/html/parser/HTMLParserScheduler.h:73
> +        if (session.processedTokens > m_parserChunkSize
> +            || session.didSeeScript) {

This can be on one line.  There's no 80 col limit in WebKit.

[Adam Barth]

This looks reasonable to me, but I'd like tonyg to take a look.

[WebKit Review Bot]

Comment on attachment 146375 [details]
Patch

Attachment 146375 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12917377

New failing tests:
fast/events/event-yield-timing.html
http/tests/loading/gmail-assert-on-load.html

[WebKit Review Bot]

Created attachment 146456 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tony Gentilcore]

Comment on attachment 146375 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146375&action=review

The approach looks great. Just a few nits to address and then this should be good to land.

> Source/WebCore/ChangeLog:11
> +        seconds once a script has been seen.

It would be nice to explain a bit more rationale in the ChangeLog since that's the first place people will look when understanding why this was done.

Something like:
"We want the parser to yield at least every 500ms to keep the page somewhat responsive and allow painting. Since it would be too expensive to check the time after each token, the previous heuristic was to check every 4,096 tokens. That works fine for most tokens, but a script may spend an arbitrary amount of time executing.

This patch fixes that by causing the parser to to check the elapsed time immediately after executing each script."

BTW, don't do anything in this patch, but have you considered whether 500ms is a reasonable constant?

> Source/WebCore/html/parser/HTMLParserScheduler.h:79
> +            if (session.processedTokens > m_parserChunkSize)

I'm not sure we need this. At the point of yielding, it should be fine to reset processedTokens, right?

> LayoutTests/fast/events/event-yield-timing.html:1
> +<html>

It doesn't seem right for a test which takes 2s to live in fast/, but I can't find a better place and I know we haven't been strict about what goes in fast/. In any case, putting this in fast/parser would be much more appropriate than fast/events.

> LayoutTests/fast/events/event-yield-timing.html:6
> +        window.layoutTestController.dumpAsText();

Recommend including ../js/resources/js-test-pre.js. It will call dumpAsText() for you and provide some helper methods like testPassed(), testFailed() and shouldBeGreaterThanOrEqual().

[Shrey Banga]

Created attachment 146891 [details]
Patch

[Shrey Banga]

(In reply to comment #9)
> BTW, don't do anything in this patch, but have you considered whether 500ms is a reasonable constant?
> 
I saw the FIXME and used some sites after setting it to 200ms and they seemed to work fine, but I thought it might require more rigorous testing, possibly on slower machines.

> > Source/WebCore/html/parser/HTMLParserScheduler.h:79
> > +            if (session.processedTokens > m_parserChunkSize)
> 
> I'm not sure we need this. At the point of yielding, it should be fine to reset processedTokens, right?
> 

I realized that processedTokens becomes redundant once didSeeScript is true because then we check for elapsedTime at every call to checkForYieldBeforeToken. So now I've modified the patch to clear didSeeScript every time we yield so that we go back to the default behavior after yielding to a script. Otherwise we would be doing the expensive currentTime() check all the time.

> > LayoutTests/fast/events/event-yield-timing.html:1
> > +<html>
> 
> It doesn't seem right for a test which takes 2s to live in fast/, but I can't find a better place and I know we haven't been strict about what goes in fast/. In any case, putting this in fast/parser would be much more appropriate than fast/events.
> 
> > LayoutTests/fast/events/event-yield-timing.html:6
> > +        window.layoutTestController.dumpAsText();
> 
> Recommend including ../js/resources/js-test-pre.js. It will call dumpAsText() for you and provide some helper methods like testPassed(), testFailed() and shouldBeGreaterThanOrEqual().

Done.

[Tony Gentilcore]

Comment on attachment 146891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146891&action=review

> Source/WebCore/html/parser/HTMLParserScheduler.h:83
> +                session.didSeeScript = false;

Now that you bring up the issue about checking after every token, I'm thinking this should go up under line 78 so that we only check the time once after each script. What do you think?

[Shrey Banga]

Comment on attachment 146891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146891&action=review

>> Source/WebCore/html/parser/HTMLParserScheduler.h:83
>> +                session.didSeeScript = false;
> 
> Now that you bring up the issue about checking after every token, I'm thinking this should go up under line 78 so that we only check the time once after each script. What do you think?

Yes, that would also make sure we don't test unnecessarily after small scripts which get executed well within 500ms.

[Shrey Banga]

Created attachment 146903 [details]
Patch

[Tony Gentilcore]

Comment on attachment 146903 [details]
Patch

Nice fix. Thanks.

[WebKit Review Bot]

Comment on attachment 146903 [details]
Patch

Rejecting attachment 146903 [details] from commit-queue.

New failing tests:
http/tests/loading/gmail-assert-on-load.html
Full output: http://queues.webkit.org/results/12941449

[WebKit Review Bot]

Created attachment 146946 [details]
Archive of layout-test-results from ec2-cq-01

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 146903 [details]
Patch

Attachment 146903 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12936758

New failing tests:
http/tests/loading/gmail-assert-on-load.html

[WebKit Review Bot]

Created attachment 146951 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Shrey Banga]

Created attachment 146953 [details]
Patch

[Shrey Banga]

Updated patch after rebaselining the failing test.

[Tony Gentilcore]

It isn't readily clear to me whether rebaselining that test is the correct thing to do. If it is, please add an explanation to the ChangeLog.

[Shrey Banga]

Created attachment 147115 [details]
Patch

[Tony Gentilcore]

Comment on attachment 147115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147115&action=review

> LayoutTests/ChangeLog:13
> +        it as passing.

Thanks for the explanation. Also, this gets the expectations back to where they were prior to http://trac.webkit.org/changeset/61234 .

One last nit before landing. Could you please collapse your ChangeLog entries into one and move this description like so:

2012-06-07  Shrey Banga  <[e-mail]>

        One-line
        <bug>

        Description

        * fast/paser/...: Added.
        * http/tests/loading/gmail-assert-on-load-expected.txt: The test was added for an assert that...

[Shrey Banga]

Created attachment 147125 [details]
Patch

[WebKit Review Bot]

Comment on attachment 147125 [details]
Patch

Clearing flags on attachment: 147125

Committed r120108: <http://trac.webkit.org/changeset/120108>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Looks like this was rolled out in <http://trac.webkit.org/changeset/120237>.

[Geoffrey Garen]

(In reply to comment #28)
> Looks like this was rolled out in <http://trac.webkit.org/changeset/120237>.

Actually, the ChangeLog said it was rolled out, but really there was just a test expectation update.