Bug 121236

Summary: MarkedBlocks shouldn't be put in Allocated state if they didn't produce a FreeList
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch ggaren: review+

Description Mark Hahnenberg 2013-09-12 10:39:49 PDT
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.
Comment 1 Mark Hahnenberg 2013-09-12 10:42:10 PDT
Created attachment 211442 [details]
Patch
Comment 2 Geoffrey Garen 2013-09-12 11:04:43 PDT
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.)
Comment 3 Mark Hahnenberg 2013-09-12 11:05:43 PDT
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!
Comment 4 Mark Hahnenberg 2013-09-12 11:12:27 PDT
Committed r155632: <http://trac.webkit.org/changeset/155632>
Comment 5 Mark Hahnenberg 2013-09-12 15:39:03 PDT
Reopening because the patch was rolled out. Need to figure out what went wrong.
Comment 6 Mark Hahnenberg 2013-09-13 10:11:10 PDT
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.
Comment 7 Mark Hahnenberg 2013-09-13 14:40:35 PDT
Created attachment 211592 [details]
Patch
Comment 8 Geoffrey Garen 2013-09-16 10:59:21 PDT
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".
Comment 9 Mark Hahnenberg 2013-09-16 12:47:47 PDT
Committed r155891: <http://trac.webkit.org/changeset/155891>