Bug 111130

Created attachment 190851 [details] Patch

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()?

Created attachment 190854 [details] Patch for landing

> 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 on attachment 190854 [details] Patch for landing Clearing flags on attachment: 190854 Committed r144407: <http://trac.webkit.org/changeset/144407>

All reviewed patches have been landed. Closing bug.

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. :)

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.