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: | P2 | Keywords: | Performance | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Bug Depends on: | 88923 | ||||||||||||||||||||||||||
Bug Blocks: | 89812 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Tony Gentilcore
2012-05-10 18:12:17 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. Created attachment 146359 [details]
Patch
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.
Created attachment 146375 [details]
Patch
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. This looks reasonable to me, but I'd like tonyg to take a look. 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 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
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(). Created attachment 146891 [details]
Patch
(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. 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? 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. Created attachment 146903 [details]
Patch
Comment on attachment 146903 [details]
Patch
Nice fix. Thanks.
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 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
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 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
Created attachment 146953 [details]
Patch
Updated patch after rebaselining the failing test. 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. Created attachment 147115 [details]
Patch
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... Created attachment 147125 [details]
Patch
Comment on attachment 147125 [details] Patch Clearing flags on attachment: 147125 Committed r120108: <http://trac.webkit.org/changeset/120108> All reviewed patches have been landed. Closing bug. Looks like this was rolled out in <http://trac.webkit.org/changeset/120237>. (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. |