Bug 110547 – Free up background parser's checkpoints when speculation succeeds

• Component: WebCore Misc.
• Status: RESOLVED DUPLICATE

[Tony Gentilcore]

When speculation fails, we send a resumeFrom message to the background thread parser which causes it to free up all its checkpoints. We should send a message when speculation succeeds so that it can free up memory used for previous checkpoints.

[Eric Seidel (no email)]

Created attachment 191440 [details]
wip

[Eric Seidel (no email)]

I'm not even sure that compiles yet (my laptop is slow).  But I welcome your feedback.  I'm likely to go to sleep before finishing it tonight.

[Eric Seidel (no email)]

Created attachment 191448 [details]
Builds, but crashes fast/preloader tests

[Eric Seidel (no email)]

Comment on attachment 191448 [details]
Builds, but crashes fast/preloader tests

Old habits. :)  Didn't mean to mark for review.

[Eric Seidel (no email)]

Created attachment 191458 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 191458 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        pages, as we will no longer hold multiple copies of every
> +        source byte during the whole parse.

This is slightly misleading, as this is really only letting go of one copy.  The m_segments and m_checkpoint.input hold onto refcounted StringImpls, so there is just one shared copy which we're now releasing earlier.  I expect this will only effect particularly large loads.

[Eric Seidel (no email)]

I believe this patch is ready for review and landing?

[Daniel Bates]

Comment on attachment 191458 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Many LayoutTests exercise this code path, and I've manually (debugger and printf)

I think you're missing the word "verified" in this sentence.

> Source/WebCore/html/parser/BackgroundHTMLInputStream.cpp:58
> +    const Checkpoint& checkpoint = m_checkpoints[checkpointIndex];

Would it make sense to assert the invariant checkpointIndex < m_checkpoints.size() in this function?

[Adam Barth]

Comment on attachment 191458 [details]
Patch

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

I should look at this again when I'm less tired.  It's not as beautiful as I had hoped.

> Source/WebCore/html/parser/BackgroundHTMLInputStream.cpp:60
> +    for (size_t i = 0; i < checkpoint.numberOfSegmentsAlreadyAppended; ++i)
> +        m_segments[i] = String();

Does this work correctly in the case where there are multiple checkpoints per append?  Probably.

Also, this algorithm is n^2.  Should we keep some state for the smallest index that has actual data and start the loop from there?

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:462
> +        checkpointIndex = std::max(chunk->inputCheckpoint, checkpointIndex);

Why is this needed?

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:470
> +        // Processing a chunk can call document.write, causing us to invalidate any remaining speculations.
> +        // Similarly, if we're stopped just abort and don't bother to tell the parser about passing this checkpoint.
> +        if (m_speculations.isEmpty() || isStopped()) {
> +            checkpointIndex = 0;
> +            break;
> +        }

This is pretty ugly....

[Adam Barth]

Comment on attachment 191458 [details]
Patch

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

I realize my complaints below are mostly aesthetic...

> Source/WebCore/html/parser/BackgroundHTMLInputStream.cpp:56
> +void BackgroundHTMLInputStream::releaseDataUpThroughCheckpoint(HTMLInputCheckpoint checkpointIndex)

invalidateCheckpointsBefore ?

>> Source/WebCore/html/parser/BackgroundHTMLInputStream.cpp:58
>> +    const Checkpoint& checkpoint = m_checkpoints[checkpointIndex];
> 
> Would it make sense to assert the invariant checkpointIndex < m_checkpoints.size() in this function?

We should ASSERT that checkpoint hasn't been invalidated yet.

>> Source/WebCore/html/parser/BackgroundHTMLInputStream.cpp:60
>> +        m_segments[i] = String();
> 
> Does this work correctly in the case where there are multiple checkpoints per append?  Probably.
> 
> Also, this algorithm is n^2.  Should we keep some state for the smallest index that has actual data and start the loop from there?

Please fix the n^2 issue.

>> Source/WebCore/html/parser/HTMLDocumentParser.cpp:470
>> +        // Processing a chunk can call document.write, causing us to invalidate any remaining speculations.
>> +        // Similarly, if we're stopped just abort and don't bother to tell the parser about passing this checkpoint.
>> +        if (m_speculations.isEmpty() || isStopped()) {
>> +            checkpointIndex = 0;
>> +            break;
>> +        }
> 
> This is pretty ugly....

There must be a way to structure this code so that it's less subtle.  Maybe we should do this work in validateSpeculations instead?

[Eric Seidel (no email)]

Comment on attachment 191458 [details]
Patch

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

>> Source/WebCore/html/parser/BackgroundHTMLInputStream.cpp:56
>> +void BackgroundHTMLInputStream::releaseDataUpThroughCheckpoint(HTMLInputCheckpoint checkpointIndex)
> 
> invalidateCheckpointsBefore ?

Happy to name it whatever.  The tricky part for me was that it's inclusive of the checkpoint you pass it invalidateCheckpointsUntil(checkpoint)?

>>> Source/WebCore/html/parser/BackgroundHTMLInputStream.cpp:60
>>> +        m_segments[i] = String();
>> 
>> Does this work correctly in the case where there are multiple checkpoints per append?  Probably.
>> 
>> Also, this algorithm is n^2.  Should we keep some state for the smallest index that has actual data and start the loop from there?
> 
> Please fix the n^2 issue.

OK.  I'll keep a m_lastInvalidSegment or similar.

>> Source/WebCore/html/parser/HTMLDocumentParser.cpp:462
>> +        checkpointIndex = std::max(chunk->inputCheckpoint, checkpointIndex);
> 
> Why is this needed?

Since we can assume the chunks have checkpoints in ascending order, it's not.  We just need the last one.

>>> Source/WebCore/html/parser/HTMLDocumentParser.cpp:470
>>> +        }
>> 
>> This is pretty ugly....
> 
> There must be a way to structure this code so that it's less subtle.  Maybe we should do this work in validateSpeculations instead?

We need to know when the chunk is processed.  I'll have to check when validateSpeculations is called.

[Eric Seidel (no email)]

Created attachment 192294 [details]
Hopefully addressed Adam's concerns

[Adam Barth]

Comment on attachment 192294 [details]
Hopefully addressed Adam's concerns

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

I can't tell if you changed HTMLDocumentParser::pumpPendingSpeculations or if I've just read it enough times to understand what it's doing.  :)

> Source/WebCore/html/parser/BackgroundHTMLInputStream.h:72
> +    // Note: These indicies may === vector.size(), in which case there are no valid checkpoints/segments at this time.

===   ->    ==

(Triple-equals is JavaScript silliness.)

[Adam Barth]

Comment on attachment 192294 [details]
Hopefully addressed Adam's concerns

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

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:484
> +        HTMLParserThread::shared()->postTask(bind(&BackgroundHTMLParser::passedCheckpoint, m_backgroundParser, lastCheckpointPassed));

The only problem with this patch is that we'll never call passedCheckpoint if we never call pumpPendingSpeculations.  We only call pumpPendingSpeculations if we ended up queuing speculations.  It's entirely possible that we can process every chunk from the background parser immediately after receiving them.

[Eric Seidel (no email)]

(In reply to comment #14)
> (From update of attachment 192294 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192294&action=review
> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:484
> > +        HTMLParserThread::shared()->postTask(bind(&BackgroundHTMLParser::passedCheckpoint, m_backgroundParser, lastCheckpointPassed));
> 
> The only problem with this patch is that we'll never call passedCheckpoint if we never call pumpPendingSpeculations.  We only call pumpPendingSpeculations if we ended up queuing speculations.  It's entirely possible that we can process every chunk from the background parser immediately after receiving them.

That's true.  I'm not sure if I should do that in a follow-up, or go another round with this one.  I'm open to either.

[Eric Seidel (no email)]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 192294 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=192294&action=review
> > 
> > > Source/WebCore/html/parser/HTMLDocumentParser.cpp:484
> > > +        HTMLParserThread::shared()->postTask(bind(&BackgroundHTMLParser::passedCheckpoint, m_backgroundParser, lastCheckpointPassed));
> > 
> > The only problem with this patch is that we'll never call passedCheckpoint if we never call pumpPendingSpeculations.  We only call pumpPendingSpeculations if we ended up queuing speculations.  It's entirely possible that we can process every chunk from the background parser immediately after receiving them.
> 
> That's true.  I'm not sure if I should do that in a follow-up, or go another round with this one.  I'm open to either.

It seems inefficient to tell the background parser to release a chunk after each chunk.  But maybe I'm optimizing prematurely.  I have no data to suggest that messaging is expensive at all. :)

[Adam Barth]

Followup patch is fine.

[WebKit Review Bot]

Comment on attachment 192294 [details]
Hopefully addressed Adam's concerns

Clearing flags on attachment: 192294

Committed r145277: <http://trac.webkit.org/changeset/145277>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

This work continued in bug 112069.

[WebKit Review Bot]

Re-opened since this is blocked by bug 112170

[Eric Seidel (no email)]

*** This bug has been marked as a duplicate of bug 112069 ***