Bug 92819

Summary: MarkedAllocator::tryAllocateHelper() should sweep another block if it can't sweep a Structure block
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch ggaren: review+

Description Mark Hahnenberg 2012-07-31 18:22:09 PDT
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.
Comment 1 Mark Hahnenberg 2012-07-31 18:27:27 PDT
Created attachment 155698 [details]
Patch
Comment 2 Geoffrey Garen 2012-07-31 20:08:33 PDT
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.
Comment 3 Mark Hahnenberg 2012-07-31 20:10:33 PDT
(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.
Comment 4 Mark Hahnenberg 2012-07-31 20:16:09 PDT
> 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.
Comment 5 Geoffrey Garen 2012-07-31 20:40:33 PDT
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.
Comment 6 Geoffrey Garen 2012-08-01 11:13:45 PDT
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.
Comment 7 Mark Hahnenberg 2012-08-01 11:55:23 PDT
Committed r124352: <http://trac.webkit.org/changeset/124352>