Changeset 144498 in webkit

Timestamp:

Mar 1, 2013 1:53:26 PM (11 years ago)

Author:

eric@webkit.org

Message:

Threaded HTML Parser has an extra copy of every byte from the network
​https://bugs.webkit.org/show_bug.cgi?id=111135

Reviewed by Adam Barth.

Source/WebCore:

Every LayoutTest executes this code in threaded parsing mode.

• dom/DecodedDataDocumentParser.cpp:

(WebCore::DecodedDataDocumentParser::appendBytes):

• Pass ownership of the decoded string to the parser.

(WebCore::DecodedDataDocumentParser::flush):

• Same.

• dom/DecodedDataDocumentParser.h:

(DecodedDataDocumentParser):

• dom/Document.cpp:

(WebCore::Document::setContent):

• dom/DocumentParser.h:

(DocumentParser):

• dom/RawDataDocumentParser.h:

(WebCore::RawDataDocumentParser::append):

• html/FTPDirectoryDocument.cpp:

(FTPDirectoryDocumentParser):
(WebCore::FTPDirectoryDocumentParser::append):

• html/parser/HTMLDocumentParser.cpp:

(WebCore::HTMLDocumentParser::append):

• html/parser/HTMLDocumentParser.h:

(HTMLDocumentParser):

• html/parser/HTMLViewSourceParser.cpp:

(WebCore::HTMLViewSourceParser::append):

• html/parser/HTMLViewSourceParser.h:

(HTMLViewSourceParser):

• html/parser/TextDocumentParser.cpp:

(WebCore::TextDocumentParser::append):

• html/parser/TextDocumentParser.h:

(TextDocumentParser):

• loader/DocumentWriter.cpp:

(WebCore::DocumentWriter::replaceDocument):

• xml/parser/XMLDocumentParser.cpp:

(WebCore::XMLDocumentParser::append):

• xml/parser/XMLDocumentParser.h:

(XMLDocumentParser):

• xml/parser/XMLDocumentParserLibxml2.cpp:

(WebCore::XMLDocumentParser::resumeParsing):

Source/WTF:

The threaded html parser needs to accept ownership
of a string buffer. The easiest way to do this seemed
to be to use a PassRefPtr<StringImpl>, but there was no way
to generated one from a String (easily), so I added one.

• wtf/text/WTFString.h:

(WTF::String::releaseImpl):

Location:

trunk/Source

Files:

• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/text/WTFString.h (modified) (1 diff)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/dom/DecodedDataDocumentParser.cpp (modified) (2 diffs)
• WebCore/dom/DecodedDataDocumentParser.h (modified) (1 diff)
• WebCore/dom/Document.cpp (modified) (1 diff)
• WebCore/dom/DocumentParser.h (modified) (2 diffs)
• WebCore/dom/RawDataDocumentParser.h (modified) (1 diff)
• WebCore/html/FTPDirectoryDocument.cpp (modified) (2 diffs)
• WebCore/html/parser/HTMLDocumentParser.cpp (modified) (3 diffs)
• WebCore/html/parser/HTMLDocumentParser.h (modified) (1 diff)
• WebCore/html/parser/HTMLViewSourceParser.cpp (modified) (1 diff)
• WebCore/html/parser/HTMLViewSourceParser.h (modified) (1 diff)
• WebCore/html/parser/TextDocumentParser.cpp (modified) (1 diff)
• WebCore/html/parser/TextDocumentParser.h (modified) (1 diff)
• WebCore/loader/DocumentWriter.cpp (modified) (1 diff)
• WebCore/xml/parser/XMLDocumentParser.cpp (modified) (1 diff)
• WebCore/xml/parser/XMLDocumentParser.h (modified) (1 diff)
• WebCore/xml/parser/XMLDocumentParserLibxml2.cpp (modified) (1 diff)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2013-03-01  Eric Seidel  <eric@webkit.org>
+
+        Threaded HTML Parser has an extra copy of every byte from the network
+        https://bugs.webkit.org/show_bug.cgi?id=111135
+
+        Reviewed by Adam Barth.
+
+        The threaded html parser needs to accept ownership
+        of a string buffer.  The easiest way to do this seemed
+        to be to use a PassRefPtr<StringImpl>, but there was no way
+        to generated one from a String (easily), so I added one.
+
+        * wtf/text/WTFString.h:
+        (WTF::String::releaseImpl):
+
 2013-02-28  Oliver Hunt  <oliver@apple.com>
 

trunk/Source/WTF/wtf/text/WTFString.h

@@ -168 +168 @@
 
     StringImpl* impl() const { return m_impl.get(); }
+    PassRefPtr<StringImpl> releaseImpl() { return m_impl.release(); }
 
     unsigned length() const

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-03-01  Eric Seidel  <eric@webkit.org>
+
+        Threaded HTML Parser has an extra copy of every byte from the network
+        https://bugs.webkit.org/show_bug.cgi?id=111135
+
+        Reviewed by Adam Barth.
+
+        Every LayoutTest executes this code in threaded parsing mode.
+
+        * dom/DecodedDataDocumentParser.cpp:
+        (WebCore::DecodedDataDocumentParser::appendBytes):
+         - Pass ownership of the decoded string to the parser.
+        (WebCore::DecodedDataDocumentParser::flush):
+         - Same.
+        * dom/DecodedDataDocumentParser.h:
+        (DecodedDataDocumentParser):
+        * dom/Document.cpp:
+        (WebCore::Document::setContent):
+        * dom/DocumentParser.h:
+        (DocumentParser):
+        * dom/RawDataDocumentParser.h:
+        (WebCore::RawDataDocumentParser::append):
+        * html/FTPDirectoryDocument.cpp:
+        (FTPDirectoryDocumentParser):
+        (WebCore::FTPDirectoryDocumentParser::append):
+        * html/parser/HTMLDocumentParser.cpp:
+        (WebCore::HTMLDocumentParser::append):
+        * html/parser/HTMLDocumentParser.h:
+        (HTMLDocumentParser):
+        * html/parser/HTMLViewSourceParser.cpp:
+        (WebCore::HTMLViewSourceParser::append):
+        * html/parser/HTMLViewSourceParser.h:
+        (HTMLViewSourceParser):
+        * html/parser/TextDocumentParser.cpp:
+        (WebCore::TextDocumentParser::append):
+        * html/parser/TextDocumentParser.h:
+        (TextDocumentParser):
+        * loader/DocumentWriter.cpp:
+        (WebCore::DocumentWriter::replaceDocument):
+        * xml/parser/XMLDocumentParser.cpp:
+        (WebCore::XMLDocumentParser::append):
+        * xml/parser/XMLDocumentParser.h:
+        (XMLDocumentParser):
+        * xml/parser/XMLDocumentParserLibxml2.cpp:
+        (WebCore::XMLDocumentParser::resumeParsing):
+
 2013-03-01  David Hyatt  <hyatt@apple.com>
 

trunk/Source/WebCore/dom/DecodedDataDocumentParser.cpp

@@ -48 +48 @@
 
     writer->reportDataReceived();
-    append(decoded);
+    append(decoded.releaseImpl());
 }
 
@@ -58 +58 @@
 
     writer->reportDataReceived();
-    append(remainingData);
+    append(remainingData.releaseImpl());
 }
 

trunk/Source/WebCore/dom/DecodedDataDocumentParser.h

@@ -42 +42 @@
 private:
     // append is used by DocumentWriter::replaceDocument.
-    virtual void append(const SegmentedString&) = 0;
+    virtual void append(PassRefPtr<StringImpl>) = 0;
 
     // appendBytes and flush are used by DocumentWriter (the loader).

trunk/Source/WebCore/dom/Document.cpp

@@ -1359 +1359 @@
 {
     open();
-    m_parser->append(content);
+    // FIXME: This should probably use insert(), but that's (intentionally)
+    // not implemented for the XML parser as it's normally synonymous with
+    // document.write(). append() will end up yielding, but close() will
+    // pump the tokenizer syncrhonously and finish the parse.
+    m_parser->pinToMainThread();
+    m_parser->append(content.impl());
     close();
 }

trunk/Source/WebCore/dom/DocumentParser.h

@@ -25 +25 @@
 #define DocumentParser_h
 
+#include <wtf/Forward.h>
 #include <wtf/RefCounted.h>
 
@@ -52 +53 @@
     virtual void pinToMainThread() { }
 
-    // FIXME: append() should be private, but DocumentWriter::replaceDocument
-    // uses it for now.
-    virtual void append(const SegmentedString&) = 0;
+    // FIXME: append() should be private, but DocumentWriter::replaceDocument uses it for now.
+    // FIXME: This really should take a PassOwnPtr to signify that it expects to take
+    // ownership of the buffer. The parser expects the PassRefPtr to hold the only ref of the StringImpl.
+    virtual void append(PassRefPtr<StringImpl>) = 0;
 
     virtual void finish() = 0;

trunk/Source/WebCore/dom/RawDataDocumentParser.h

@@ -57 +57 @@
     }
 
-    virtual void append(const SegmentedString&)
+    virtual void append(PassRefPtr<StringImpl>)
     {
         ASSERT_NOT_REACHED();

trunk/Source/WebCore/html/FTPDirectoryDocument.cpp

@@ -58 +58 @@
     }
 
-    virtual void append(const SegmentedString&);
+    virtual void append(PassRefPtr<StringImpl>);
     virtual void finish();
 
@@ -347 +347 @@
 }
 
-void FTPDirectoryDocumentParser::append(const SegmentedString& source)
-{
+void FTPDirectoryDocumentParser::append(PassRefPtr<StringImpl> inputSource)
+{
+    String source(inputSource);
+
     // Make sure we have the table element to append to by loading the template set in the pref, or
     // creating a very basic document with the appropriate table

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

@@ -655 +655 @@
 #endif
 
-void HTMLDocumentParser::append(const SegmentedString& source)
+void HTMLDocumentParser::append(PassRefPtr<StringImpl> inputSource)
 {
     if (isStopped())
@@ -665 +665 @@
             startBackgroundParser();
 
-        HTMLParserThread::shared()->postTask(bind(
-            &BackgroundHTMLParser::append, m_backgroundParser, source.toString().isolatedCopy()));
+        ASSERT(inputSource->hasOneRef());
+        Closure closure = bind(&BackgroundHTMLParser::append, m_backgroundParser, String(inputSource));
+        // NOTE: Important that the String temporary is destroyed before we post the task
+        // otherwise the String could call deref() on a StringImpl now owned by the background parser.
+        // We would like to ASSERT(closure.arg3()->hasOneRef()) but sadly the args are private.
+        HTMLParserThread::shared()->postTask(closure);
         return;
     }
@@ -674 +678 @@
     // but we need to ensure it isn't deleted yet.
     RefPtr<HTMLDocumentParser> protect(this);
+    String source(inputSource);
 
     if (m_preloadScanner) {

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

@@ -99 +99 @@
 protected:
     virtual void insert(const SegmentedString&) OVERRIDE;
-    virtual void append(const SegmentedString&) OVERRIDE;
+    virtual void append(PassRefPtr<StringImpl>) OVERRIDE;
     virtual void finish() OVERRIDE;
 

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

@@ -63 +63 @@
 }
 
-void HTMLViewSourceParser::append(const SegmentedString& input)
+void HTMLViewSourceParser::append(PassRefPtr<StringImpl> input)
 {
-    m_input.appendToEnd(input);
+    m_input.appendToEnd(String(input));
     pumpTokenizer();
 }

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

@@ -60 +60 @@
     // DocumentParser
     virtual void insert(const SegmentedString&);
-    virtual void append(const SegmentedString&);
+    virtual void append(PassRefPtr<StringImpl>);
     virtual void finish();
 

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

@@ -45 +45 @@
 }
 
-void TextDocumentParser::append(const SegmentedString& text)
+void TextDocumentParser::append(PassRefPtr<StringImpl> text)
 {
     if (!m_haveInsertedFakePreElement)

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

@@ -42 +42 @@
     explicit TextDocumentParser(HTMLDocument*);
 
-    virtual void append(const SegmentedString&);
+    virtual void append(PassRefPtr<StringImpl>);
     void insertFakePreElement();
 

trunk/Source/WebCore/loader/DocumentWriter.cpp

@@ -81 +81 @@
         if (DocumentParser* parser = m_frame->document()->parser()) {
             parser->pinToMainThread();
-            parser->append(source);
+            // Because we're pinned to the main thread we don't need to worry about
+            // passing ownership of the source string.
+            parser->append(source.impl());
         }
     }

trunk/Source/WebCore/xml/parser/XMLDocumentParser.cpp

@@ -113 +113 @@
 }
 
-void XMLDocumentParser::append(const SegmentedString& source)
-{
+void XMLDocumentParser::append(PassRefPtr<StringImpl> inputSource)
+{
+    SegmentedString source(inputSource);
     if (m_sawXSLTransform || !m_sawFirstElement)
         m_originalSourceForTransform.append(source);

trunk/Source/WebCore/xml/parser/XMLDocumentParser.h

@@ -108 +108 @@
         // From DocumentParser
         virtual void insert(const SegmentedString&);
-        virtual void append(const SegmentedString&);
+        virtual void append(PassRefPtr<StringImpl>);
         virtual void finish();
         virtual bool isWaitingForScripts() const;

trunk/Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp

@@ -1445 +1445 @@
     SegmentedString rest = m_pendingSrc;
     m_pendingSrc.clear();
-    append(rest);
+    // There is normally only one string left, so toString() shouldn't copy.
+    // In any case, the XML parser runs on the main thread and it's OK if
+    // the passed string has more than one reference.
+    append(rest.toString().impl());
 
     // Finally, if finish() has been called and write() didn't result