Bug 86165

Summary:

HTML parser should yield more to improve perceived page load time

Product:

WebKit

Reporter:

Tony Gentilcore <tonyg>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, banga, dglazkov, ggaren, koivisto, mihaip, paulirish, simonjam, sullivan, webkit.review.bot

Priority:

Keywords:

Performance

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

88923

Bug Blocks:

89812

Attachments:

Description	Flags
Testcase	none
Patch	none
Patch	none
Archive of layout-test-results from ec2-cr-linux-02	none
Patch	none
Patch	none
Archive of layout-test-results from ec2-cq-01	none
Archive of layout-test-results from ec2-cr-linux-04	none
Patch	none
Patch	none
Patch	none

Tony Gentilcore

Reported 2012-05-10 18:12:17 PDT

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?

Attachments
Testcase (1.10 KB, text/html) 2012-05-10 18:12 PDT, Tony Gentilcore	no flags	Details
Patch (6.64 KB, patch) 2012-06-07 12:20 PDT, Shrey Banga	no flags	Details Formatted Diff Diff
Patch (6.66 KB, patch) 2012-06-07 13:43 PDT, Shrey Banga	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-02 (469.93 KB, application/zip) 2012-06-07 19:34 PDT, WebKit Review Bot	no flags	Details
Patch (7.15 KB, patch) 2012-06-11 13:15 PDT, Shrey Banga	no flags	Details Formatted Diff Diff
Patch (7.03 KB, patch) 2012-06-11 14:06 PDT, Shrey Banga	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cq-01 (769.04 KB, application/zip) 2012-06-11 15:58 PDT, WebKit Review Bot	no flags	Details
Archive of layout-test-results from ec2-cr-linux-04 (457.91 KB, application/zip) 2012-06-11 16:14 PDT, WebKit Review Bot	no flags	Details
Patch (7.94 KB, patch) 2012-06-11 16:19 PDT, Shrey Banga	no flags	Details Formatted Diff Diff
Patch (8.82 KB, patch) 2012-06-12 11:17 PDT, Shrey Banga	no flags	Details Formatted Diff Diff
Patch (8.67 KB, patch) 2012-06-12 11:39 PDT, Shrey Banga	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2012-05-10 18:22:49 PDT

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

Comment 2 2012-06-07 12:20:41 PDT

Created attachment 146359 [details] Patch

WebKit Review Bot

Comment 3 2012-06-07 12:23:44 PDT

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

Comment 4 2012-06-07 13:43:36 PDT

Created attachment 146375 [details] Patch

Adam Barth

Comment 5 2012-06-07 14:27:03 PDT

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

Comment 6 2012-06-07 14:27:32 PDT

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

WebKit Review Bot

Comment 7 2012-06-07 19:34:49 PDT

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

Comment 8 2012-06-07 19:34:53 PDT

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 9 2012-06-09 09:20:56 PDT

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

Comment 10 2012-06-11 13:15:49 PDT

Created attachment 146891 [details] Patch

Shrey Banga

Comment 11 2012-06-11 13:18:22 PDT

(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 12 2012-06-11 13:34:18 PDT

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 13 2012-06-11 14:02:52 PDT

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

Comment 14 2012-06-11 14:06:33 PDT

Created attachment 146903 [details] Patch

Tony Gentilcore

Comment 15 2012-06-11 14:33:02 PDT

Comment on attachment 146903 [details] Patch Nice fix. Thanks.

WebKit Review Bot

Comment 16 2012-06-11 15:58:17 PDT

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

Comment 17 2012-06-11 15:58:22 PDT

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 18 2012-06-11 16:14:37 PDT

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

Comment 19 2012-06-11 16:14:41 PDT

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

Comment 20 2012-06-11 16:19:18 PDT

Created attachment 146953 [details] Patch

Shrey Banga

Comment 21 2012-06-11 16:20:09 PDT

Updated patch after rebaselining the failing test.

Tony Gentilcore

Comment 22 2012-06-12 06:26:33 PDT

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

Comment 23 2012-06-12 11:17:18 PDT

Created attachment 147115 [details] Patch

Tony Gentilcore

Comment 24 2012-06-12 11:31:46 PDT

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 <banga@chromium.org> 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

Comment 25 2012-06-12 11:39:36 PDT

Created attachment 147125 [details] Patch

WebKit Review Bot

Comment 26 2012-06-12 13:21:13 PDT

Comment on attachment 147125 [details] Patch Clearing flags on attachment: 147125 Committed r120108: <http://trac.webkit.org/changeset/120108>

WebKit Review Bot

Comment 27 2012-06-12 13:21:20 PDT

All reviewed patches have been landed. Closing bug.

Geoffrey Garen

Comment 28 2012-06-13 13:40:16 PDT

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

Geoffrey Garen

Comment 29 2012-06-13 13:43:20 PDT

(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.

Note You need to log in before you can comment on or make changes to this bug.