Bug 126555 - Copying should be generational
Summary: Copying should be generational
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: 121074
  Show dependency treegraph
 
Reported: 2014-01-06 17:26 PST by Mark Hahnenberg
Modified: 2014-02-13 03:50 PST (History)
9 users (show)

See Also:


Attachments
Patch (32.58 KB, patch)
2014-01-10 17:32 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (32.26 KB, patch)
2014-01-14 09:02 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (31.77 KB, patch)
2014-01-14 12:52 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2014-01-06 17:26:18 PST
Copying should also be generational. Eden collections should always trigger copying. Full collections should use our normal fragmentation-based heuristics.
Comment 1 Mark Hahnenberg 2014-01-10 17:32:22 PST
Created attachment 220912 [details]
Patch
Comment 2 EFL EWS Bot 2014-01-10 17:59:11 PST
Comment on attachment 220912 [details]
Patch

Attachment 220912 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/4947627439292416
Comment 3 EFL EWS Bot 2014-01-10 18:19:46 PST
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
Comment 4 Geoffrey Garen 2014-01-13 18:02:22 PST
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.
Comment 5 Mark Hahnenberg 2014-01-14 09:02:19 PST
Created attachment 221167 [details]
Patch
Comment 6 Mark Hahnenberg 2014-01-14 09:03:02 PST
(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.
Comment 7 EFL EWS Bot 2014-01-14 09:13:26 PST
Comment on attachment 221167 [details]
Patch

Attachment 221167 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6525023300878336
Comment 8 EFL EWS Bot 2014-01-14 09:25:07 PST
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
Comment 9 Geoffrey Garen 2014-01-14 12:07:25 PST
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!
Comment 10 Mark Hahnenberg 2014-01-14 12:52:39 PST
Created attachment 221189 [details]
Patch
Comment 11 Mark Hahnenberg 2014-01-14 12:52:57 PST
(In reply to comment #10)
> Created an attachment (id=221189) [details]
> Patch

One more upload for EWS to chew on.
Comment 12 Mark Hahnenberg 2014-01-14 14:58:55 PST
Committed r162017: <http://trac.webkit.org/changeset/162017>
Comment 13 Csaba Osztrogonác 2014-02-13 03:50:00 PST
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).