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