Threaded HTML Parser should limit speculation to avoid using too much memory
Created attachment 192564 [details] wip, hangs several tests.
Comment on attachment 192564 [details] wip, hangs several tests. View in context: https://bugs.webkit.org/attachment.cgi?id=192564&action=review > Source/WebCore/dom/NodeRenderingContext.cpp:186 > + // display:none iframes don't need renderers. > + // However, if our ownerElement is not yet attached, we can't trust its renderer() being null. > + if (m_node->document()->ownerElement() > + && m_node->document()->ownerElement()->attached() > + && !m_node->document()->ownerElement()->renderer()) > + return false; This was from another patch. ignore.
Created attachment 192599 [details] Patch
*** Bug 111893 has been marked as a duplicate of this bug. ***
Comment on attachment 192599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192599&action=review > Source/WebCore/html/parser/HTMLDocumentParser.cpp:-348 > - if (!m_currentChunk) { > - // If there is no m_currentChunk, we must have already called didFailSpeculation > - // for this chunk. > - // FIXME: In this case, we're losing whatever state has been changed since > - // we called didFailSpeculation. See https://bugs.webkit.org/show_bug.cgi?id=110546 > - return; > - } I'm worried that we're not handling this case anymore. Maybe this patch fixes it? We should have test coverage for it.
Created attachment 192614 [details] Patch
Comment on attachment 192614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192614&action=review > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:53 > +static const size_t outstandingCheckpointStopLimit = 10; > +static const size_t outstandingCheckpointStartLimit = 3; We should tune these constants based on some performance data. > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:152 > + // No need to start speculating until the main thread is nearly caught up. > + if (m_input.outstandingCheckpointCount() > outstandingCheckpointStartLimit) > + return; How do you know that we'll actually finish parsing everything? Don't we need to bypass this check in the finish() call?
(In reply to comment #7) > (From update of attachment 192614 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192614&action=review > > > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:53 > > +static const size_t outstandingCheckpointStopLimit = 10; > > +static const size_t outstandingCheckpointStartLimit = 3; > > We should tune these constants based on some performance data. > > > Source/WebCore/html/parser/BackgroundHTMLParser.cpp:152 > > + // No need to start speculating until the main thread is nearly caught up. > > + if (m_input.outstandingCheckpointCount() > outstandingCheckpointStartLimit) > > + return; > > How do you know that we'll actually finish parsing everything? Don't we need to bypass this check in the finish() call? This code is correct because the main thread will eventually catch up, which will kick the background thread into action.
Comment on attachment 192614 [details] Patch Rejecting attachment 192614 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 192614, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: Hunk #6 succeeded at 486 (offset 2 lines). Hunk #7 succeeded at 886 (offset 2 lines). 2 out of 7 hunks FAILED -- saving rejects to file Source/WebCore/html/parser/HTMLDocumentParser.cpp.rej patching file Source/WebCore/html/parser/HTMLDocumentParser.h Hunk #1 succeeded at 143 (offset 2 lines). Hunk #2 succeeded at 196 (offset 2 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://webkit-commit-queue.appspot.com/results/17130351
Created attachment 192640 [details] Patch for landing
Created attachment 192644 [details] Patch for landing
Created attachment 192646 [details] Patch for landing
Created attachment 192648 [details] Patch for landing
Comment on attachment 192648 [details] Patch for landing Rejecting attachment 192648 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'validate-changelog', '--non-interactive', 192648, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-commit-queue.appspot.com/results/17174031
Created attachment 192649 [details] Patch for landing
I wonder if this is causing falkiness. Teh CQ is having trouble landing it.
Comment on attachment 192649 [details] Patch for landing I'm going to look at this again tomorrow before we land it.
Created attachment 193008 [details] updated
Created attachment 193012 [details] Patch for landing
Created attachment 193019 [details] Patch for landing
Comment on attachment 193019 [details] Patch for landing Attachment 193019 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17194175 New failing tests: inspector/debugger/dom-breakpoints.html inspector/audits/audits-panel-functional.html fast/parser/external-script-document-write.html http/tests/navigation/lockedhistory-iframe.html
Comment on attachment 193019 [details] Patch for landing Attachment 193019 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17215030 New failing tests: fast/parser/external-script-document-write.html
Comment on attachment 193019 [details] Patch for landing Attachment 193019 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17199079 New failing tests: fast/parser/external-script-document-write.html
The Mac failures are because I failed to add the necessary subresource for the new test in my diff. I'm not sure about the cr-linux failures. Investigating.
(In reply to comment #21) > (From update of attachment 193019 [details]) > Attachment 193019 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://webkit-commit-queue.appspot.com/results/17194175 > > New failing tests: > inspector/debugger/dom-breakpoints.html > inspector/audits/audits-panel-functional.html > http/tests/navigation/lockedhistory-iframe.html These other 3 are real failures from my patch. I'll investigate.
Created attachment 193072 [details] Fixed one test, but still breaks 2 inspector tests somehow
Comment on attachment 193072 [details] Fixed one test, but still breaks 2 inspector tests somehow Attachment 193072 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17132110 New failing tests: inspector/debugger/dom-breakpoints.html inspector/audits/audits-panel-functional.html http/tests/navigation/lockedhistory-iframe.html
I dumped printfs all over the parser. There are multiple documents being parsed at once in these tests, these limits change the order in which the documents complete. I think the inspector tests are just racy here, and I'm causing us to now alway lose the race. If I adjust the outstandingChunkLimit up to 10, then the failures go away. :)
Created attachment 193091 [details] Patch for landing
In the end this is just a rather awkward producer/consumer implementation, using the MessageQueue lock to build a semaphore. :) We could probably change this around to be simpler/more-explicit in future patches.
(In reply to comment #28) > I dumped printfs all over the parser. There are multiple documents being parsed at once in these tests, these limits change the order in which the documents complete. I think the inspector tests are just racy here, and I'm causing us to now alway lose the race. > > If I adjust the outstandingChunkLimit up to 10, then the failures go away. :) I unified the start/stop limits into one. What was happening was that if your document could parse with fewer than 10 checkpoints it would never yield. So short documents were beating out longer ones, especially since longer ones got throttled down to 3 checkpoints after the first one. :) So I removed the separate start/stop limits, and that magically made the larger document able to parse again before the smaller one as it did not have to wait for 7 checkpoints to get processed before it could start pumping again. Again, I believe this to be a race in the inspector tests for expecting one document to not load before the other but it's not longer possible to trigger it again for the meanwhile. :)
Comment on attachment 193091 [details] Patch for landing Clearing flags on attachment: 193091 Committed r145797: <http://trac.webkit.org/changeset/145797>
All reviewed patches have been landed. Closing bug.
To restate: The previous code worked that if your main document had more than 10 checkpoints (aka chunks) in it (1000 tokens, or a </script> tag cause a chunk/checkpoint) then it would yield, and continue yielding until the main thread had processed enough of those chunks until only 3 outstanding checkpoints/chunks were remaining. In that time, if it caused a sub-resource load and that load happend to fit under 10 chunks (or better yet, under 3) then it likely would be processed to completion before the main load was. This is what seemed to be biting the inspector tests. I didn't dig deep enough to figure out which page (it could even have been the inspector page itself) was being loaded "out of order", but such an investigation would be possible using an earlier version of my patch and some printfs in HTMLDocumentParser. It was important that the start and stop limits were separate, as that gave time for "little" documents to easily complete parsing in one go before the larger document (which had yielded the background thread) would have processed stop - start tokens and be able to continue/finish parsing.
*** Bug 110547 has been marked as a duplicate of this bug. ***
This was another big perf win on the PLTs. Quoting Nils: https://code.google.com/p/chromium/issues/detail?id=180819#c19 Moz times: -1.3% on chromium-rel-win7-webkit/moz/times http://chromium-perf.appspot.com/?tab=chromium-rel-win7-webkit&graph=times&trace=t_extcs1&rev=188340&history=150&master=ChromiumWebkit&testSuite=page_cycler_moz&details=true -1.3% and -2.0% on chromium-rel-win7-webkit/intl2/times http://chromium-perf.appspot.com/?tab=chromium-rel-win7-webkit&graph=times&trace=t_extwr&rev=188353&history=150&master=ChromiumWebkit&testSuite=page_cycler_intl2&details=true http://chromium-perf.appspot.com/?tab=chromium-rel-win7-webkit&graph=times&trace=t&rev=188368&history=150&master=ChromiumWebkit&testSuite=page_cycler_intl2&details=true Startup times: -2.4% chromium-rel-win7-webkit/startup/warm/profiling_scripts1-first change after rev 145805 http://chromium-perf.appspot.com/?tab=chromium-rel-win7-webkit&graph=warm&trace=profiling_scripts1-first&rev=188340&history=150&master=ChromiumWebkit&testSuite=startup_test&details=true