- Revision
- 120108
- Author
- [email protected]
- Date
- 2012-06-12 13:20:57 -0700 (Tue, 12 Jun 2012)
Log 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 <[email protected]> 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 <[email protected]> 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (120107 => 120108)
--- trunk/LayoutTests/ChangeLog 2012-06-12 20:09:22 UTC (rev 120107)
+++ trunk/LayoutTests/ChangeLog 2012-06-12 20:20:57 UTC (rev 120108)
@@ -1,3 +1,26 @@
+2012-06-12 Shrey Banga <[email protected]>
+
+ 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 <[email protected]>
LayoutTests/http/tests/xmlhttprequest/origin-exact-matching.html is giving trouble due to too many redirects
Added: trunk/LayoutTests/fast/parser/parser-yield-timing-expected.txt (0 => 120108)
--- trunk/LayoutTests/fast/parser/parser-yield-timing-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/parser/parser-yield-timing-expected.txt 2012-06-12 20:20:57 UTC (rev 120108)
@@ -0,0 +1,12 @@
+Runs 3 separate 1 second <script> blocks with a setTimeout schedule before each. The execution time of the setTimeout is when the HTML yielded to the event loop. The yields also represent painting opportunities. We want the parser to yield every 0.5 seconds once it has seen a script.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS new Date().getTime() - startTime is within 100 of 1000
+PASS new Date().getTime() - startTime is within 100 of 2000
+PASS new Date().getTime() - startTime is within 100 of 3000
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/parser/parser-yield-timing.html (0 => 120108)
--- trunk/LayoutTests/fast/parser/parser-yield-timing.html (rev 0)
+++ trunk/LayoutTests/fast/parser/parser-yield-timing.html 2012-06-12 20:20:57 UTC (rev 120108)
@@ -0,0 +1,56 @@
+<html>
+
+ <head>
+ <script type="text/_javascript_" src=""
+ <script>
+ if (window.layoutTestController) {
+ window.layoutTestController.waitUntilDone();
+ }
+
+ description("Runs 3 separate 1 second <script> blocks with a setTimeout schedule before each. The execution time of the setTimeout is when the HTML yielded to the event loop. The yields also represent painting opportunities. We want the parser to yield every 0.5 seconds once it has seen a script.");
+
+ var startTime = new Date().getTime();
+ var tolerance = 100; // 100 ms
+
+ function expectElapsedTime(expectedTime) {
+ shouldBeCloseTo("new Date().getTime() - startTime", expectedTime, tolerance);
+ }
+
+ function sleep(ms) {
+ var endTime = (new Date().getTime()) + ms;
+ while ((new Date().getTime()) < endTime);
+ }
+ </script>
+ </head>
+
+ <body>
+ <p></p>
+
+ <script>
+ setTimeout(function() { expectElapsedTime(1000) }, 0);
+ </script>
+ <script>
+ sleep(1000);
+ </script>
+ <script>
+ setTimeout(function() { expectElapsedTime(2000) }, 0);
+ </script>
+ <script>
+ sleep(1000);
+ </script>
+ <script>
+ setTimeout(function() {
+ expectElapsedTime(3000);
+
+ if (window.layoutTestController) {
+ layoutTestController.notifyDone();
+ }
+ }, 0);
+ </script>
+ <script>
+ sleep(1000);
+ </script>
+ <script type="text/_javascript_" src=""
+ </body>
+</html>
+
Modified: trunk/LayoutTests/http/tests/loading/gmail-assert-on-load-expected.txt (120107 => 120108)
--- trunk/LayoutTests/http/tests/loading/gmail-assert-on-load-expected.txt 2012-06-12 20:09:22 UTC (rev 120107)
+++ trunk/LayoutTests/http/tests/loading/gmail-assert-on-load-expected.txt 2012-06-12 20:20:57 UTC (rev 120108)
@@ -4,7 +4,7 @@
main frame - didFinishDocumentLoadForFrame
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.
Modified: trunk/Source/WebCore/ChangeLog (120107 => 120108)
--- trunk/Source/WebCore/ChangeLog 2012-06-12 20:09:22 UTC (rev 120107)
+++ trunk/Source/WebCore/ChangeLog 2012-06-12 20:20:57 UTC (rev 120108)
@@ -1,3 +1,25 @@
+2012-06-12 Shrey Banga <[email protected]>
+
+ 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 <[email protected]>
[chromium] Don't crash in CCLayerIterator if the root layer doesn't have a render surface
Modified: trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp (120107 => 120108)
--- trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp 2012-06-12 20:09:22 UTC (rev 120107)
+++ trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp 2012-06-12 20:20:57 UTC (rev 120108)
@@ -95,6 +95,7 @@
bool needsFirstPaint = document->view() && !document->view()->hasEverPainted();
if (needsFirstPaint && document->isLayoutTimerActive())
session.needsYield = true;
+ session.didSeeScript = true;
}
void HTMLParserScheduler::scheduleForResume()
Modified: trunk/Source/WebCore/html/parser/HTMLParserScheduler.h (120107 => 120108)
--- trunk/Source/WebCore/html/parser/HTMLParserScheduler.h 2012-06-12 20:09:22 UTC (rev 120107)
+++ trunk/Source/WebCore/html/parser/HTMLParserScheduler.h 2012-06-12 20:20:57 UTC (rev 120108)
@@ -47,12 +47,14 @@
, processedTokens(INT_MAX)
, startTime(0)
, needsYield(false)
+ , didSeeScript(false)
{
}
int processedTokens;
double startTime;
bool needsYield;
+ bool didSeeScript;
};
class HTMLParserScheduler {
@@ -67,13 +69,15 @@
// Inline as this is called after every token in the parser.
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.
if (!session.startTime)
session.startTime = currentTime();
session.processedTokens = 0;
+ session.didSeeScript = false;
+
double elapsedTime = currentTime() - session.startTime;
if (elapsedTime > m_parserTimeLimit)
session.needsYield = true;
