Changeset 120108 in webkit

Timestamp:

Jun 12, 2012 1:20:57 PM (12 years ago)

Author:

commit-queue@webkit.org

Message:

HTML parser should yield more to improve perceived page load time
​https://bugs.webkit.org/show_bug.cgi?id=86165

Source/WebCore:

Patch by Shrey Banga <​banga@chromium.org> on 2012-06-12
Reviewed by Tony Gentilcore.

Test: fast/events/event-yield-timing.html

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 the issue by causing the parser to check the elapsed time immediately after executing a script.

• html/parser/HTMLParserScheduler.cpp:

(WebCore::HTMLParserScheduler::checkForYieldBeforeScript):

• html/parser/HTMLParserScheduler.h:

(WebCore::PumpSession::PumpSession):
(PumpSession):
(WebCore::HTMLParserScheduler::checkForYieldBeforeToken):

LayoutTests:

Added test for parser yield times after seeing a script

Patch by Shrey Banga <​banga@chromium.org> on 2012-06-12
Reviewed by Tony Gentilcore.

• fast/parser/parser-yield-timing-expected.txt: Added.
• fast/parser/parser-yield-timing.html: Added.
• http/tests/loading/gmail-assert-on-load-expected.txt: The test was added

for an assert that failed on debug builds when an iframe removed itself
from its parent window after blocking for 1 second. In previous builds,
the destroyIt() function was called without yielding to the event loop,
which lead to didFailLoadWithError being called. The fix for bug 86165
ensures that we yield after executing the script, which leads to
didHandleOnloadEventsForFrame being called instead. This is the cause
of the change in output of the test. The test still successfully removes
the iframe and since no asserts fail in the debug build, we should consider
it as passing.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/parser/parser-yield-timing-expected.txt (added)
• LayoutTests/fast/parser/parser-yield-timing.html (added)
• LayoutTests/http/tests/loading/gmail-assert-on-load-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/parser/HTMLParserScheduler.cpp (modified) (1 diff)
• Source/WebCore/html/parser/HTMLParserScheduler.h (modified) (4 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2012-06-12  Shrey Banga  <banga@chromium.org>
+
+        HTML parser should yield more to improve perceived page load time
+        https://bugs.webkit.org/show_bug.cgi?id=86165
+
+        Added test for parser yield times after seeing a script
+
+        Reviewed by Tony Gentilcore.
+
+        * fast/parser/parser-yield-timing-expected.txt: Added.
+        * fast/parser/parser-yield-timing.html: Added.
+        * http/tests/loading/gmail-assert-on-load-expected.txt: The test was added
+        for an assert that failed on debug builds when an iframe removed itself 
+        from its parent window after blocking for 1 second. In previous builds,
+        the destroyIt() function was called without yielding to the event loop,
+        which lead to didFailLoadWithError being called. The fix for bug 86165
+        ensures that we yield after executing the script, which leads to 
+        didHandleOnloadEventsForFrame being called instead. This is the cause
+        of the change in output of the test. The test still successfully removes
+        the iframe and since no asserts fail in the debug build, we should consider
+        it as passing.
+
+
 2012-06-12  Pablo Flouret  <pablof@motorola.com>
 

trunk/LayoutTests/http/tests/loading/gmail-assert-on-load-expected.txt

@@ -5 +5 @@
 frame "<!--framePath //<!--frame0-->-->" - didCommitLoadForFrame
 frame "<!--framePath //<!--frame0-->-->" - didFinishDocumentLoadForFrame
-frame "<!--framePath //<!--frame0-->-->" - didFailLoadWithError
+main frame - didHandleOnloadEventsForFrame
 main frame - didFinishLoadForFrame
 This test provokes HTMLTokenizer::timerFired to be called and from within timerFired we want to call WebCore::pageDestroyed.

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-06-12  Shrey Banga  <banga@chromium.org>
+
+        HTML parser should yield more to improve perceived page load time
+        https://bugs.webkit.org/show_bug.cgi?id=86165
+
+        Reviewed by Tony Gentilcore.
+
+        Test: fast/events/event-yield-timing.html
+
+        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 the issue by causing the parser to check the elapsed time immediately after executing a script.
+
+        * html/parser/HTMLParserScheduler.cpp:
+        (WebCore::HTMLParserScheduler::checkForYieldBeforeScript):
+        * html/parser/HTMLParserScheduler.h:
+        (WebCore::PumpSession::PumpSession):
+        (PumpSession):
+        (WebCore::HTMLParserScheduler::checkForYieldBeforeToken):
+
 2012-06-12  Sami Kyostila  <skyostil@chromium.org>
 

trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp

@@ -96 +96 @@
     if (needsFirstPaint && document->isLayoutTimerActive())
         session.needsYield = true;
+    session.didSeeScript = true;
 }
 

trunk/Source/WebCore/html/parser/HTMLParserScheduler.h

@@ -48 +48 @@
         , startTime(0)
         , needsYield(false)
+        , didSeeScript(false)
     {
     }
@@ -54 +55 @@
     double startTime;
     bool needsYield;
+    bool didSeeScript;
 };
 
@@ -68 +70 @@
     void checkForYieldBeforeToken(PumpSession& session)
     {
-        if (session.processedTokens > m_parserChunkSize) {
+        if (session.processedTokens > m_parserChunkSize || session.didSeeScript) {
             // currentTime() can be expensive.  By delaying, we avoided calling
             // currentTime() when constructing non-yielding PumpSessions.
@@ -75 +77 @@
 
             session.processedTokens = 0;
+            session.didSeeScript = false;
+
             double elapsedTime = currentTime() - session.startTime;
             if (elapsedTime > m_parserTimeLimit)