Bug 126555 – Copying should be generational

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

[Mark Hahnenberg]

Copying should also be generational. Eden collections should always trigger copying. Full collections should use our normal fragmentation-based heuristics.

[Mark Hahnenberg]

Created attachment 220912 [details]
Patch

[EFL EWS Bot]

Comment on attachment 220912 [details]
Patch

Attachment 220912 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/4947627439292416

[EFL EWS Bot]

Comment on attachment 220912 [details]
Patch

Attachment 220912 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5079382943072256

[Geoffrey Garen]

Comment on attachment 220912 [details]
Patch

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.

[Mark Hahnenberg]

Created attachment 221167 [details]
Patch

[Mark Hahnenberg]

(In reply to comment #5)
> Created an attachment (id=221167) [details]
> Patch

Just to make sure we're on the same page w.r.t. SlotVisitor::copyLater, CopiedBlock::shouldReportLiveBytes, and CopiedBlock::reportLiveBytes.

[EFL EWS Bot]

Comment on attachment 221167 [details]
Patch

Attachment 221167 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6525023300878336

[EFL EWS Bot]

Comment on attachment 221167 [details]
Patch

Attachment 221167 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/6152784122478592

[Geoffrey Garen]

Comment on attachment 221167 [details]
Patch

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

r=me

You should send email to webkit-dev inviting the EFL and GTK maintainers to resolved their respective build issues.

> Source/WTF/wtf/Platform.h:760
> +#define ENABLE_GGC 1

-1 point for Gryffindor!

[Mark Hahnenberg]

Created attachment 221189 [details]
Patch

[Mark Hahnenberg]

(In reply to comment #10)
> Created an attachment (id=221189) [details]
> Patch

One more upload for EWS to chew on.

[Mark Hahnenberg]

Committed r162017: <http://trac.webkit.org/changeset/162017>

[Csaba Osztrogonác]

Comment on attachment 221189 [details]
Patch

Cleared review? from attachment 221189 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).