Title: [140478] trunk/Source/WebCore
Revision
140478
Author
aba...@webkit.org
Date
2013-01-22 15:14:56 -0800 (Tue, 22 Jan 2013)

Log Message

The BackgroundHTMLParser shouldn't pause when waiting for scripts
https://bugs.webkit.org/show_bug.cgi?id=107584

Reviewed by Eric Seidel.

Previously, the BackgroundHTMLParser would pause itself when it
encountered a scrip tag and wait for a signal from the main thread to
continue. After this patch, the BackgroundHTMLParser continues ahead
and the main thread keeps a queue of pending tokens.

This patch brings us closer to speculative parsing because when the
BackgroundHTMLParser is continuing ahead, it is speculating that it is
in the correct state. A future patch will let us abort incorret
speculations and resume from an eariler point in the input stream.

* html/parser/BackgroundHTMLParser.cpp:
(WebCore::checkThatTokensAreSafeToSendToAnotherThread):
(WebCore::BackgroundHTMLParser::BackgroundHTMLParser):
(WebCore::BackgroundHTMLParser::simulateTreeBuilder):
(WebCore::BackgroundHTMLParser::pumpTokenizer):
(WebCore::TokenDelivery::TokenDelivery):
(TokenDelivery):
(WebCore::TokenDelivery::execute):
(WebCore::BackgroundHTMLParser::sendTokensToMainThread):
* html/parser/BackgroundHTMLParser.h:
(BackgroundHTMLParser):
* html/parser/CompactHTMLToken.h:
(WebCore):
* html/parser/HTMLDocumentParser.cpp:
(WebCore::HTMLDocumentParser::didReceiveTokensFromBackgroundParser):
(WebCore):
(WebCore::HTMLDocumentParser::processTokensFromBackgroundParser):
(WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution):
* html/parser/HTMLDocumentParser.h:
(HTMLDocumentParser):

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (140477 => 140478)


--- trunk/Source/WebCore/ChangeLog	2013-01-22 23:13:58 UTC (rev 140477)
+++ trunk/Source/WebCore/ChangeLog	2013-01-22 23:14:56 UTC (rev 140478)
@@ -1,3 +1,41 @@
+2013-01-22  Adam Barth  <aba...@webkit.org>
+
+        The BackgroundHTMLParser shouldn't pause when waiting for scripts
+        https://bugs.webkit.org/show_bug.cgi?id=107584
+
+        Reviewed by Eric Seidel.
+
+        Previously, the BackgroundHTMLParser would pause itself when it
+        encountered a scrip tag and wait for a signal from the main thread to
+        continue. After this patch, the BackgroundHTMLParser continues ahead
+        and the main thread keeps a queue of pending tokens.
+
+        This patch brings us closer to speculative parsing because when the
+        BackgroundHTMLParser is continuing ahead, it is speculating that it is
+        in the correct state. A future patch will let us abort incorret
+        speculations and resume from an eariler point in the input stream.
+
+        * html/parser/BackgroundHTMLParser.cpp:
+        (WebCore::checkThatTokensAreSafeToSendToAnotherThread):
+        (WebCore::BackgroundHTMLParser::BackgroundHTMLParser):
+        (WebCore::BackgroundHTMLParser::simulateTreeBuilder):
+        (WebCore::BackgroundHTMLParser::pumpTokenizer):
+        (WebCore::TokenDelivery::TokenDelivery):
+        (TokenDelivery):
+        (WebCore::TokenDelivery::execute):
+        (WebCore::BackgroundHTMLParser::sendTokensToMainThread):
+        * html/parser/BackgroundHTMLParser.h:
+        (BackgroundHTMLParser):
+        * html/parser/CompactHTMLToken.h:
+        (WebCore):
+        * html/parser/HTMLDocumentParser.cpp:
+        (WebCore::HTMLDocumentParser::didReceiveTokensFromBackgroundParser):
+        (WebCore):
+        (WebCore::HTMLDocumentParser::processTokensFromBackgroundParser):
+        (WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution):
+        * html/parser/HTMLDocumentParser.h:
+        (HTMLDocumentParser):
+
 2013-01-22  Simon Fraser  <simon.fra...@apple.com>
 
         Fix scrollperf logging

Modified: trunk/Source/WebCore/html/parser/BackgroundHTMLParser.cpp (140477 => 140478)


--- trunk/Source/WebCore/html/parser/BackgroundHTMLParser.cpp	2013-01-22 23:13:58 UTC (rev 140477)
+++ trunk/Source/WebCore/html/parser/BackgroundHTMLParser.cpp	2013-01-22 23:14:56 UTC (rev 140478)
@@ -45,10 +45,10 @@
 
 #ifndef NDEBUG
 
-static void checkThatTokensAreSafeToSendToAnotherThread(const Vector<CompactHTMLToken>& tokens)
+static void checkThatTokensAreSafeToSendToAnotherThread(const CompactHTMLTokenStream* tokens)
 {
-    for (size_t i = 0; i < tokens.size(); ++i)
-        ASSERT(tokens[i].isSafeToSendToAnotherThread());
+    for (size_t i = 0; i < tokens->size(); ++i)
+        ASSERT(tokens->at(i).isSafeToSendToAnotherThread());
 }
 
 #endif
@@ -92,11 +92,11 @@
 }
 
 BackgroundHTMLParser::BackgroundHTMLParser(const HTMLParserOptions& options, ParserIdentifier identifier)
-    : m_isPausedWaitingForScripts(false)
-    , m_inForeignContent(false)
+    : m_inForeignContent(false)
     , m_tokenizer(HTMLTokenizer::create(options))
     , m_options(options)
     , m_parserIdentifer(identifier)
+    , m_pendingTokens(adoptPtr(new CompactHTMLTokenStream))
 {
 }
 
@@ -106,12 +106,6 @@
     pumpTokenizer();
 }
 
-void BackgroundHTMLParser::continueParsing()
-{
-    m_isPausedWaitingForScripts = false;
-    pumpTokenizer();
-}
-
 void BackgroundHTMLParser::finish()
 {
     markEndOfFile();
@@ -129,7 +123,7 @@
     m_input.close();
 }
 
-void BackgroundHTMLParser::simulateTreeBuilder(const CompactHTMLToken& token)
+bool BackgroundHTMLParser::simulateTreeBuilder(const CompactHTMLToken& token)
 {
     if (token.type() == HTMLTokenTypes::StartTag) {
         const String& tagName = token.data();
@@ -158,28 +152,21 @@
         if (threadSafeMatch(tagName, SVGNames::svgTag) || threadSafeMatch(tagName, MathMLNames::mathTag))
             m_inForeignContent = false;
         if (threadSafeMatch(tagName, scriptTag))
-            m_isPausedWaitingForScripts = true;
+            return false;
     }
 
     // FIXME: Need to set setForceNullCharacterReplacement based on m_inForeignContent as well.
     m_tokenizer->setShouldAllowCDATA(m_inForeignContent);
+    return true;
 }
 
 void BackgroundHTMLParser::pumpTokenizer()
 {
-    if (m_isPausedWaitingForScripts)
-        return;
-
     while (m_tokenizer->nextToken(m_input, m_token)) {
-        m_pendingTokens.append(CompactHTMLToken(m_token, TextPosition(m_input.currentLine(), m_input.currentColumn())));
+        m_pendingTokens->append(CompactHTMLToken(m_token, TextPosition(m_input.currentLine(), m_input.currentColumn())));
         m_token.clear();
 
-        simulateTreeBuilder(m_pendingTokens.last());
-
-        if (m_isPausedWaitingForScripts)
-            break;
-
-        if (m_pendingTokens.size() >= pendingTokenLimit)
+        if (!simulateTreeBuilder(m_pendingTokens->last()) || m_pendingTokens->size() >= pendingTokenLimit)
             sendTokensToMainThread();
     }
 
@@ -191,25 +178,17 @@
 public:
     TokenDelivery()
         : identifier(0)
-        , isPausedWaitingForScripts(false)
     {
     }
 
     ParserIdentifier identifier;
-    Vector<CompactHTMLToken> tokens;
-    // FIXME: This bool will be replaced by a CheckPoint object once
-    // we implement speculative parsing. Then the main thread will decide
-    // to either accept the speculative tokens we've already given it
-    // (or ask for them, depending on who ends up owning them), or send
-    // us a "reset to checkpoint message".
-    bool isPausedWaitingForScripts;
-
+    OwnPtr<CompactHTMLTokenStream> tokens;
     static void execute(void* context)
     {
         TokenDelivery* delivery = static_cast<TokenDelivery*>(context);
         HTMLDocumentParser* parser = parserMap().mainThreadParsers().get(delivery->identifier);
         if (parser)
-            parser->didReceiveTokensFromBackgroundParser(delivery->tokens, delivery->isPausedWaitingForScripts);
+            parser->didReceiveTokensFromBackgroundParser(delivery->tokens.release());
         // FIXME: Ideally we wouldn't need to call delete manually. Instead
         // we would like an API where the message queue owns the tasks and
         // takes care of deleting them.
@@ -219,20 +198,19 @@
 
 void BackgroundHTMLParser::sendTokensToMainThread()
 {
-    if (m_pendingTokens.isEmpty()) {
-        ASSERT(!m_isPausedWaitingForScripts);
+    if (m_pendingTokens->isEmpty())
         return;
-    }
 
 #ifndef NDEBUG
-    checkThatTokensAreSafeToSendToAnotherThread(m_pendingTokens);
+    checkThatTokensAreSafeToSendToAnotherThread(m_pendingTokens.get());
 #endif
 
     TokenDelivery* delivery = new TokenDelivery;
     delivery->identifier = m_parserIdentifer;
-    delivery->tokens.swap(m_pendingTokens);
-    delivery->isPausedWaitingForScripts = m_isPausedWaitingForScripts;
+    delivery->tokens = m_pendingTokens.release();
     callOnMainThread(TokenDelivery::execute, delivery);
+
+    m_pendingTokens = adoptPtr(new CompactHTMLTokenStream);
 }
 
 void BackgroundHTMLParser::createPartial(ParserIdentifier identifier, HTMLParserOptions options)
@@ -253,12 +231,6 @@
         parser->append(input);
 }
 
-void BackgroundHTMLParser::continuePartial(ParserIdentifier identifier)
-{
-    if (BackgroundHTMLParser* parser = parserMap().backgroundParsers().get(identifier))
-        parser->continueParsing();
-}
-
 void BackgroundHTMLParser::finishPartial(ParserIdentifier identifier)
 {
     if (BackgroundHTMLParser* parser = parserMap().backgroundParsers().get(identifier))

Modified: trunk/Source/WebCore/html/parser/BackgroundHTMLParser.h (140477 => 140478)


--- trunk/Source/WebCore/html/parser/BackgroundHTMLParser.h	2013-01-22 23:13:58 UTC (rev 140477)
+++ trunk/Source/WebCore/html/parser/BackgroundHTMLParser.h	2013-01-22 23:14:56 UTC (rev 140478)
@@ -42,7 +42,6 @@
     WTF_MAKE_FAST_ALLOCATED;
 public:
     void append(const String&);
-    void continueParsing();
     void finish();
 
     static PassOwnPtr<BackgroundHTMLParser> create(const HTMLParserOptions& options, ParserIdentifier identifier)
@@ -53,7 +52,6 @@
     static void createPartial(ParserIdentifier, HTMLParserOptions);
     static void stopPartial(ParserIdentifier);
     static void appendPartial(ParserIdentifier, const String& input);
-    static void continuePartial(ParserIdentifier);
     static void finishPartial(ParserIdentifier);
 
 private:
@@ -61,18 +59,17 @@
 
     void markEndOfFile();
     void pumpTokenizer();
-    void simulateTreeBuilder(const CompactHTMLToken&);
+    bool simulateTreeBuilder(const CompactHTMLToken&);
 
     void sendTokensToMainThread();
 
     SegmentedString m_input;
     HTMLToken m_token;
-    bool m_isPausedWaitingForScripts;
     bool m_inForeignContent; // FIXME: We need a stack of foreign content markers.
     OwnPtr<HTMLTokenizer> m_tokenizer;
     HTMLParserOptions m_options;
     ParserIdentifier m_parserIdentifer;
-    Vector<CompactHTMLToken> m_pendingTokens;
+    OwnPtr<CompactHTMLTokenStream> m_pendingTokens;
 };
 
 class ParserMap {

Modified: trunk/Source/WebCore/html/parser/CompactHTMLToken.h (140477 => 140478)


--- trunk/Source/WebCore/html/parser/CompactHTMLToken.h	2013-01-22 23:13:58 UTC (rev 140477)
+++ trunk/Source/WebCore/html/parser/CompactHTMLToken.h	2013-01-22 23:14:56 UTC (rev 140478)
@@ -81,6 +81,8 @@
     TextPosition m_textPosition;
 };
 
+typedef Vector<CompactHTMLToken> CompactHTMLTokenStream;
+
 }
 
 #endif // ENABLE(THREADED_HTML_PARSER)

Modified: trunk/Source/WebCore/html/parser/HTMLDocumentParser.cpp (140477 => 140478)


--- trunk/Source/WebCore/html/parser/HTMLDocumentParser.cpp	2013-01-22 23:13:58 UTC (rev 140477)
+++ trunk/Source/WebCore/html/parser/HTMLDocumentParser.cpp	2013-01-22 23:14:56 UTC (rev 140478)
@@ -262,8 +262,18 @@
 
 #if ENABLE(THREADED_HTML_PARSER)
 
-void HTMLDocumentParser::didReceiveTokensFromBackgroundParser(const Vector<CompactHTMLToken>& tokens, bool threadIsWaitingForScripts)
+void HTMLDocumentParser::didReceiveTokensFromBackgroundParser(PassOwnPtr<CompactHTMLTokenStream> tokens)
 {
+    if (isWaitingForScripts()) {
+        m_pendingTokens.append(tokens);
+        return;
+    }
+    ASSERT(m_pendingTokens.isEmpty());
+    processTokensFromBackgroundParser(tokens);
+}
+
+void HTMLDocumentParser::processTokensFromBackgroundParser(PassOwnPtr<CompactHTMLTokenStream> tokens)
+{
     ASSERT(shouldUseThreading());
 
     // didReceiveTokensFromBackgroundParser can cause this parser to be detached from the Document,
@@ -272,7 +282,7 @@
 
     // FIXME: Add support for InspectorInstrumentation.
 
-    for (Vector<CompactHTMLToken>::const_iterator it = tokens.begin(); it != tokens.end(); ++it) {
+    for (Vector<CompactHTMLToken>::const_iterator it = tokens->begin(); it != tokens->end(); ++it) {
         ASSERT(!isWaitingForScripts());
 
         // FIXME: Call m_xssAuditor.filterToken(*it).
@@ -286,13 +296,8 @@
         // as we do in canTakeNextToken;
 
         if (isWaitingForScripts()) {
-            ASSERT(threadIsWaitingForScripts); // We expect that the thread is waiting for us.
+            ASSERT(it + 1 == tokens->end()); // The </script> is assumed to be the last token of this bunch.
             runScriptsForPausedTreeBuilder();
-            if (!isWaitingForScripts()) {
-                ParserIdentifier identifier = ParserMap::identifierForParser(this);
-                HTMLParserThread::shared()->postTask(bind(&BackgroundHTMLParser::continuePartial, identifier));
-            }
-            ASSERT(it + 1 == tokens.end()); // The </script> is assumed to be the last token of this bunch.
             return;
         }
 
@@ -301,18 +306,11 @@
         // attemptToRunDeferredScriptsAndEnd(), prepareToStopParsing(), or
         // attemptToEnd() instead.
         if (it->type() == HTMLTokenTypes::EndOfFile) {
-            ASSERT(it + 1 == tokens.end()); // The EOF is assumed to be the last token of this bunch.
+            ASSERT(it + 1 == tokens->end()); // The EOF is assumed to be the last token of this bunch.
             prepareToStopParsing();
             return;
         }
     }
-
-    // If we got here and the parser thread is still waiting for scripts, then it paused unnecessarily
-    // (as can happen with a stray </script> tag), and we need to tell it to continue.
-    if (threadIsWaitingForScripts) {
-        ParserIdentifier identifier = ParserMap::identifierForParser(this);
-        HTMLParserThread::shared()->postTask(bind(&BackgroundHTMLParser::continuePartial, identifier));
-    }
 }
 
 #endif // ENABLE(THREADED_HTML_PARSER)
@@ -659,8 +657,11 @@
 
 #if ENABLE(THREADED_HTML_PARSER)
     if (shouldUseThreading()) {
-        ParserIdentifier identifier = ParserMap::identifierForParser(this);
-        HTMLParserThread::shared()->postTask(bind(&BackgroundHTMLParser::continuePartial, identifier));
+        while (!m_pendingTokens.isEmpty()) {
+            processTokensFromBackgroundParser(m_pendingTokens.takeFirst());
+            if (isWaitingForScripts())
+                return;
+        }
         return;
     }
 #endif

Modified: trunk/Source/WebCore/html/parser/HTMLDocumentParser.h (140477 => 140478)


--- trunk/Source/WebCore/html/parser/HTMLDocumentParser.h	2013-01-22 23:13:58 UTC (rev 140477)
+++ trunk/Source/WebCore/html/parser/HTMLDocumentParser.h	2013-01-22 23:14:56 UTC (rev 140478)
@@ -37,6 +37,7 @@
 #include "SegmentedString.h"
 #include "Timer.h"
 #include "XSSAuditor.h"
+#include <wtf/Deque.h>
 #include <wtf/OwnPtr.h>
 #include <wtf/text/TextPosition.h>
 
@@ -80,7 +81,7 @@
     virtual void resumeScheduledTasks();
 
 #if ENABLE(THREADED_HTML_PARSER)
-    void didReceiveTokensFromBackgroundParser(const Vector<CompactHTMLToken>&, bool threadIsWaitingForScripts);
+    void didReceiveTokensFromBackgroundParser(PassOwnPtr<CompactHTMLTokenStream>);
 #endif
 
 protected:
@@ -122,6 +123,7 @@
 #if ENABLE(THREADED_HTML_PARSER)
     void startBackgroundParser();
     void stopBackgroundParser();
+    void processTokensFromBackgroundParser(PassOwnPtr<CompactHTMLTokenStream>);
 #endif
 
     enum SynchronousMode {
@@ -167,6 +169,10 @@
     TextPosition m_textPosition;
     XSSAuditor m_xssAuditor;
 
+#if ENABLE(THREADED_HTML_PARSER)
+    Deque<OwnPtr<CompactHTMLTokenStream> > m_pendingTokens;
+#endif
+
     bool m_endWasDelayed;
     bool m_haveBackgroundParser;
     unsigned m_pumpSessionNestingLevel;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to