Threaded HTML Parser should limit speculation to avoid using too much memory

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Threaded HTML Parser should limit speculation to avoid using too much memory

[Eric Seidel (no email)]

Created attachment 192564 [details]
wip, hangs several tests.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 192599 [details]
Patch

[Eric Seidel (no email)]

*** Bug 111893 has been marked as a duplicate of this bug. ***

[Adam Barth]

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.

[Eric Seidel (no email)]

Created attachment 192614 [details]
Patch

[Adam Barth]

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?

[Adam Barth]

(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.

[WebKit Review Bot]

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

[Eric Seidel (no email)]

Created attachment 192640 [details]
Patch for landing

[Eric Seidel (no email)]

Created attachment 192644 [details]
Patch for landing

[Eric Seidel (no email)]

Created attachment 192646 [details]
Patch for landing

[Eric Seidel (no email)]

Created attachment 192648 [details]
Patch for landing

[WebKit Review Bot]

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

[Eric Seidel (no email)]

Created attachment 192649 [details]
Patch for landing

[Eric Seidel (no email)]

I wonder if this is causing falkiness.  Teh CQ is having trouble landing it.

[Eric Seidel (no email)]

Comment on attachment 192649 [details]
Patch for landing

I'm going to look at this again tomorrow before we land it.

[Eric Seidel (no email)]

Created attachment 193008 [details]
updated

[Eric Seidel (no email)]

Created attachment 193012 [details]
Patch for landing

[Eric Seidel (no email)]

Created attachment 193019 [details]
Patch for landing

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

(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.

[Eric Seidel (no email)]

Created attachment 193072 [details]
Fixed one test, but still breaks 2 inspector tests somehow

[WebKit Review Bot]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Created attachment 193091 [details]
Patch for landing

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[WebKit Review Bot]

Comment on attachment 193091 [details]
Patch for landing

Clearing flags on attachment: 193091

Committed r145797: <http://trac.webkit.org/changeset/145797>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

*** Bug 110547 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

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