[webkit-reviews] review granted: [Bug 126555] Copying should be generational : [Attachment 220912] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 126555: Copying should be generational
https://bugs.webkit.org/show_bug.cgi?id=126555

Attachment 220912: Patch
https://bugs.webkit.org/attachment.cgi?id=220912&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220912&action=review


r=me, with some suggestions

I don't understand the EFL bot's objection, so I think we should ignore it.

> Source/JavaScriptCore/ChangeLog:12
> +	   and a new generation of CopiedBlocks. During each mutator cycle new
CopiedSpace allocations reside

I think you meant "set of CopiedBlocks".

> Source/JavaScriptCore/ChangeLog:18
> +	   One key thing to remember is that both new and old generation
objects in the MarkedSpace can
> +	   refer to old or new generation allocations in CopiedSpace. In other
words, the generations in CopiedSpace
> +	   are independent of those in the MarkedSpace. This is why we must
fire write barriers when assigning 
> +	   to an old (MarkedSpace) object's Butterfly.

I don't think it's right to say that the spaces are "independent". Both spaces
promote at the same time. That makes them dependent. I think I would just
remove that middle sentence. The rest is good.

> Source/JavaScriptCore/heap/CopiedBlockInlines.h:42
> +    // this if this block was allocated during the last cycle. 

Remove extra "this".

> Source/JavaScriptCore/heap/CopiedBlockInlines.h:46
> +    // We want to add to live bytes if the owner isn't part of the
remembered set or
> +    // this if this block was allocated during the last cycle. 
> +    // If we always added live bytes we would double count for elements in
the remembered
> +    // set across collections. 
> +    // If we didn't always add live bytes to new blocks, we'd get too few.
> +    if (!isOwnerRemembered || !m_isOld) {

Let's factor this into a helper function -- maybe "shouldReportLiveBytes" --
and call the helper function at the call site, and assert that it's true inside
this function. That way, you can remove the double checking inside the
function, and establish consistency between what SlotVisitor records and what
CopiedBlock records.

I think you could tighten this comment by starting with each case we want to
avoid: "Don't report when copying on behalf of a remembered object because...";
"Don't report when copying an old object because..."

> Source/JavaScriptCore/heap/CopiedSpace.cpp:238
> +    DoublyLinkedList<CopiedBlock>* toSpace, *fromSpace;

Two lines, please. Multiple pointer declaration is just too easy to get wrong.

> Source/JavaScriptCore/heap/CopiedSpaceInlines.h:97
> +template <HeapOperation collectionType>
>  inline void CopiedSpace::recycleEvacuatedBlock(CopiedBlock* block)

I think it would be better just to pass HeapOperation as an argument. That way,
you could remove the if at the call site. Since this function is inlined, no
efficiency will be lost.

> Source/JavaScriptCore/heap/CopiedSpaceInlines.h:185
> +    DoublyLinkedList<CopiedBlock>* toSpace, *fromSpace, *oversizeBlocks;

Multiple lines, please.

> Source/JavaScriptCore/heap/CopiedSpaceInlines.h:217
> +    // TODO: Make sure we accurately keep track of the total live bytes for
EdenCollections.

Is this still a TODO? It seems like you did it.

> Source/JavaScriptCore/heap/CopiedSpaceInlines.h:247
> +    double totalFragmentation = ((double)totalLiveBytes + markedSpaceBytes)
/ ((double)totalUsableBytes + markedSpaceBytes);

static_cast, please.

> Source/JavaScriptCore/heap/GCThreadSharedData.cpp:206
> +	       unsigned index = 0;
> +	       CopiedBlock* block = m_copiedSpace->m_newGen.fromSpace->head();
> +	       // First we fill up the pre-existing space in the vector.
> +	       while (block) {
> +		   if (index >= m_blocksToCopy.size())
> +		       break;
> +		   m_blocksToCopy[index++] = block; 
> +		   block = block->next();
> +	       }
> +
> +	       // If we have no more blocks in the linked list we reduce the
size of the vector 
> +	       // to the size of the linked list. We do the resize here instead
of at the very 
> +	       // beginning because it's O(n) to get the length of the linked
list of blocks.
> +	       if (!block)
> +		   m_blocksToCopy.resize(index);
> +
> +	       // If we exhausted the pre-existing space in the vector and have
more blocks then start
> +	       // appending to the end of the vector.
> +	       while (block) {
> +		   m_blocksToCopy.append(block);
> +		   block = block->next();
> +	       }

You can just do a normal shrink(0) followed by append for each item. The
backing store will not deallocate from a shrink(0) -- only from clear(),
shrinkToFit(), or shrinkCapacity().

> Source/JavaScriptCore/heap/SlotVisitorInlines.h:231
> +    bool isRemembered = heap()->operationInProgress() == EdenCollection &&
MarkedBlock::blockFor(owner)->isRemembered(owner);
> +    if (!isRemembered)
> +	   m_bytesCopied += bytes;

This check is different from the check done inside CopiedBlock. See my comment
there for a suggestion of how to improve this.