Bug 92819 - MarkedAllocator::tryAllocateHelper() should sweep another block if it can't sweep a Structure block
Summary: MarkedAllocator::tryAllocateHelper() should sweep another block if it can't s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-31 18:22 PDT by Mark Hahnenberg
Modified: 2012-08-01 11:55 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.92 KB, patch)
2012-07-31 18:27 PDT, Mark Hahnenberg
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>