Bug 121236 - MarkedBlocks shouldn't be put in Allocated state if they didn't produce a FreeList
Summary: MarkedBlocks shouldn't be put in Allocated state if they didn't produce a Fre...
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: 2013-09-12 10:39 PDT by Mark Hahnenberg
Modified: 2013-09-16 12:47 PDT (History)
0 users

See Also:


Attachments
Patch (3.65 KB, patch)
2013-09-12 10:42 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (31.62 KB, patch)
2013-09-13 14:40 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 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>