Bug 80705 – Simplify memory usage tracking in CopiedSpace

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

Currently we have a complex and bug-riddled way of keeping track of the amount of memory allocated/utilized in CopiedSpace. We should rip this junk out and replace it with a simple, verifiably correct alternative because getting it wrong can cause big problems with respect to collection. For example, since our utilization is an unsigned number, if our book keeping causes it to wrap around, we will never do any more non-berserker GCs.

[Mark Hahnenberg]

Created attachment 131048 [details]
Patch

[Geoffrey Garen]

Comment on attachment 131048 [details]
Patch

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

> Source/JavaScriptCore/heap/CopiedSpace.cpp:245
> +size_t CopiedSpace::totalMemoryAllocated()

Is this function used anywhere? I don't see its use inside JavaScriptCore.

> Source/JavaScriptCore/heap/CopiedSpace.cpp:268
> +    for (CopiedBlock* block = static_cast<CopiedBlock*>(m_fromSpace->head()); block; block = static_cast<CopiedBlock*>(block->next()))

I don't think it's right to count from space toward utilization. Only to space contains utilized memory.

> Source/JavaScriptCore/heap/CopiedSpace.h:69
> +    size_t totalMemoryAllocated();
> +    size_t totalMemoryUtilized();

Our typical names for these concepts are "size" and "capacity".

[Mark Hahnenberg]

(In reply to comment #2)
> (From update of attachment 131048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131048&action=review
> 
> > Source/JavaScriptCore/heap/CopiedSpace.cpp:245
> > +size_t CopiedSpace::totalMemoryAllocated()
> 
> Is this function used anywhere? I don't see its use inside JavaScriptCore.

I was using it to get an overall picture of our memory utilization (e.g. blocks are 95% full), and I meant to remove it, will do.

> > Source/JavaScriptCore/heap/CopiedSpace.cpp:268
> > +    for (CopiedBlock* block = static_cast<CopiedBlock*>(m_fromSpace->head()); block; block = static_cast<CopiedBlock*>(block->next()))
> 
> I don't think it's right to count from space toward utilization. Only to space contains utilized memory.

From-space should always be empty when we're running this function anyways, so I'll get rid of that loop and add an assertion for that condition.

> > Source/JavaScriptCore/heap/CopiedSpace.h:69
> > +    size_t totalMemoryAllocated();
> > +    size_t totalMemoryUtilized();
> 
> Our typical names for these concepts are "size" and "capacity".

Will change.

This patch also resulted in a performance regression due to the fact that we iterate all blocks every time we run out of room in the current block. I'll work on a patch that does the book keeping in the simple cases until we hit a GC, which will then check all blocks to get a full picture. The complexity in the old code mostly came from the copying phase, so I think it should still be able to keep it simple.

[Geoffrey Garen]

> This patch also resulted in a performance regression due to the fact that we iterate all blocks every time we run out of room in the current block.

A design I'd suggest is to use the watermark strategy used by MarkedSpace:
    - Calculate size once, right after GC, to determine a "high water mark"
    - After running out of room in a block, add the block to the "water mark"
    - Before allocating a new block, check "water mark" against "high water mark", and collect if exceeded

[Mark Hahnenberg]

(In reply to comment #4)
> > This patch also resulted in a performance regression due to the fact that we iterate all blocks every time we run out of room in the current block.
> 
> A design I'd suggest is to use the watermark strategy used by MarkedSpace:
>     - Calculate size once, right after GC, to determine a "high water mark"
>     - After running out of room in a block, add the block to the "water mark"
>     - Before allocating a new block, check "water mark" against "high water mark", and collect if exceeded

This is essentially what I was planning on, although it's good to have a common terminology :-)

[Mark Hahnenberg]

Created attachment 131362 [details]
Patch

[Geoffrey Garen]

Comment on attachment 131362 [details]
Patch

A few notes here:

- Please change Heap::size() and Heap::capacity() to include CopiedSpace, and remove code that manually adds in CopiedSpace.

- I don't think we want an explicit CopiedSpace::calculateWaterMark(). Rather, any allocation should automatically add to the watermark, and the start of GC should clear the watermark. In cases where that's inefficient, you should defer adding to the watermark until the allocate slow case. You can copy the design of MarkedSpace here.

- You should add capacity() to the watermark, not size().

[Mark Hahnenberg]

*** Bug 81321 has been marked as a duplicate of this bug. ***

[Mark Hahnenberg]

Created attachment 132444 [details]
Patch

[WebKit Review Bot]

Comment on attachment 132444 [details]
Patch

Clearing flags on attachment: 132444

Committed r111877: <http://trac.webkit.org/changeset/111877>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.