WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 86165
HTML parser should yield more to improve perceived page load time
https://bugs.webkit.org/show_bug.cgi?id=86165
Summary
HTML parser should yield more to improve perceived page load time
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.
Top of Page
Format For Printing
XML
Clone This Bug