Bug 111130

Summary: The threaded HTML parser shouldn't need to invalidate the speculation buffer on every document.write
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, esprehn+autocc, mjs, ojan.autocc, tonyg, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 106127    
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Adam Barth 2013-02-28 16:57:33 PST
The threaded HTML parser shouldn't need to invalidate the speculation buffer on every document.write
Comment 1 Adam Barth 2013-02-28 17:12:01 PST
Created attachment 190851 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-02-28 17:15:22 PST
Comment on attachment 190851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190851&action=review

Looks amazing.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:337
> +    if (m_currentChunk->tokenizerState == HTMLTokenizer::DataState && m_tokenizer->state() == HTMLTokenizer::DataState && m_input.current().isEmpty()) {

I would split this off into a nicely named funtion.  isSafeToContinueSpeculation()?
Comment 3 Adam Barth 2013-02-28 17:21:35 PST
Created attachment 190854 [details]
Patch for landing
Comment 4 Adam Barth 2013-02-28 17:42:17 PST
> I would split this off into a nicely named funtion.  isSafeToContinueSpeculation()?

We talked about this in person and decided to add a detailed comment instead.
Comment 5 WebKit Review Bot 2013-02-28 18:14:50 PST
Comment on attachment 190854 [details]
Patch for landing

Clearing flags on attachment: 190854

Committed r144407: <http://trac.webkit.org/changeset/144407>
Comment 6 WebKit Review Bot 2013-02-28 18:14:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Adam Barth 2013-02-28 19:10:49 PST
To give some more color to this bug, I wrote this patch based on running the threaded parser through the moz PLT dataset.  One page in the data set (voodooextreme.com) showed a 200% regression with the threaded parser.  This patch is meant to address that regression by improving our ability to re-use the speculation buffer.

I don't have the ability to run the moz PLT locally, but I did run the Voodoo Extreme page from the moz dataset through DumpRenderTree in three configurations: (1) main thread, (2) old threaded parser, (3) the threaded parser after this patch.  Here are the results for the average of six runs (after a warm-up run for each configuration):

Main thread: 0.33s
Old threaded parser: 0.47s
New threaded parser: 0.33s

These numbers have the old threaded parser being a 30.36% slowdown and the new threaded parser being a 0.46% slowdown (the latter seems likely to be noise).

I also instrumented checkForSpeculationFailure.  Before the patch, we dumped the speculation buffer many times (roughly once for each document.write).  After the patch, we fail only once (at the end when the page document.writes "<!--" to simulate a noscript tag).

Ideally I would have put this information into the ChangeLog entry rather than the bug.  :)
Comment 8 Maciej Stachowiak 2013-02-28 22:08:38 PST
It wouldn't be completely wrong to add some of this to the ChangeLog after the fact. :-)

I believe that pages generated with lots of document.write() are far less common these days than when the Mozilla Base test dataset was collected, but it's still good not to massively regress them.