Bug 92819 – MarkedAllocator::tryAllocateHelper() should sweep another block if it can't sweep a Structure block

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

If we are forced to allocate a new block for Structures because we are unable to safely sweep our pre-existing Structure blocks, we should sweep another random block so that we can start sweeping Structure blocks sooner.

[Mark Hahnenberg]

Created attachment 155698 [details]
Patch

[Geoffrey Garen]

Comment on attachment 155698 [details]
Patch

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

Looks good but I think I see a bug.

> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92
> +    while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) {

I don't think you want this loop here, do you? The goal of this function is to sweep just one.

> Source/JavaScriptCore/heap/MarkedAllocator.cpp:38
> +            m_heap->sweeper()->sweepNextBlock();

This could use a "why" comment.

[Mark Hahnenberg]

(In reply to comment #2)
> (From update of attachment 155698 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155698&action=review
> 
> Looks good but I think I see a bug.
> 
> > Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92
> > +    while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) {
> 
> I don't think you want this loop here, do you? The goal of this function is to sweep just one.

The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false).

> This could use a "why" comment.

Will do.

[Mark Hahnenberg]

> The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false).

That being said, I'll come up with a better way to write it.

[Geoffrey Garen]

Comment on attachment 155698 [details]
Patch

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

>>> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92
>>> +    while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) {
>> 
>> I don't think you want this loop here, do you? The goal of this function is to sweep just one.
> 
> The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false).

OK, I see where you're going with that. I still think it would be simpler (and slightly faster) to remove the extra loop. I see sweeping a block with no destructors as just a special fast case, and not a reason to keep sweeping.

[Geoffrey Garen]

Comment on attachment 155698 [details]
Patch

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

Please add the "why" comment before landing.

>>>> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92
>>>> +    while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) {
>>> 
>>> I don't think you want this loop here, do you? The goal of this function is to sweep just one.
>> 
>> The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false).
> 
> OK, I see where you're going with that. I still think it would be simpler (and slightly faster) to remove the extra loop. I see sweeping a block with no destructors as just a special fast case, and not a reason to keep sweeping.

Eh, you convinced me. Let's keep it this way to keep things simple.

[Mark Hahnenberg]

Committed r124352: <http://trac.webkit.org/changeset/124352>