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.
Created attachment 131048 [details] Patch
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".
(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.
> 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
(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 :-)
Created attachment 131362 [details] Patch
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().
*** Bug 81321 has been marked as a duplicate of this bug. ***
Created attachment 132444 [details] Patch
Comment on attachment 132444 [details] Patch Clearing flags on attachment: 132444 Committed r111877: <http://trac.webkit.org/changeset/111877>
All reviewed patches have been landed. Closing bug.