Bug 121236 – MarkedBlocks shouldn't be put in Allocated state if they didn't produce a FreeList

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

Right now, after a collection all MarkedBlocks are in the Marked block state. When lazy sweeping happens, if a block returns an empty free list after being swept, we call didConsumeFreeList(), which moves the block into the Allocated block state. This happens to both the block that was just being allocated out of (i.e. m_currentBlock) as well as any blocks who are completely full. We should distinguish between these two cases: m_currentBlock should transition to Allocated (because we were just allocating out of it) and any subsequent block that returns an empty free list should transition back to the Marked state. This will make the block state more consistent with the actual state the block is in, and it will also allow us to speed up moving all blocks the the Marked state during generational collection.

[Mark Hahnenberg]

Created attachment 211442 [details]
Patch

[Geoffrey Garen]

Comment on attachment 211442 [details]
Patch

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

r=me

> Source/JavaScriptCore/heap/MarkedAllocator.cpp:42
> +                block->didSweepButToNoAvail();

How about "didConsumeEmptyFreeList"? ("No avail" doesn't specify what you were trying to avail yourself of.)

[Mark Hahnenberg]

Comment on attachment 211442 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:16
> +        all blocks the the Marked state during generational collection.

s/the the/to the/

>> Source/JavaScriptCore/heap/MarkedAllocator.cpp:42
>> +                block->didSweepButToNoAvail();
> 
> How about "didConsumeEmptyFreeList"? ("No avail" doesn't specify what you were trying to avail yourself of.)

Sounds good to me!

[Mark Hahnenberg]

Committed r155632: <http://trac.webkit.org/changeset/155632>

[Mark Hahnenberg]

Reopening because the patch was rolled out. Need to figure out what went wrong.

[Mark Hahnenberg]

I think the issue was that there are other clients of canonicalize other than garbage collection that were screwing up the block state when they canonicalized prematurely.

[Mark Hahnenberg]

Created attachment 211592 [details]
Patch

[Geoffrey Garen]

Comment on attachment 211592 [details]
Patch

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

r=me

> Source/JavaScriptCore/heap/MarkedAllocator.h:135
> +inline void MarkedAllocator::uncanonicalizeCellLivenessData()
> +{
> +    if (!m_canonicalizedBlock)
> +        return;
> +
> +    m_freeList = m_canonicalizedBlock->uncanonicalizeCellLivenessData();
> +    m_currentBlock = m_canonicalizedBlock;
> +    m_canonicalizedBlock = 0;
> +}

Let's call these functions "stopAllocating" and "resumeAllocating". That way, we're a little clearer about what's prohibited in this state. Otherwise, "canonical" doesn't sound like such a bad thing.

> Source/JavaScriptCore/heap/MarkedSpace.h:86
> +    bool iterationInProgress() { return m_currentlyIterating; }

Let's call this "isIterating()" and "m_isIterating".

[Mark Hahnenberg]

Committed r155891: <http://trac.webkit.org/changeset/155891>