Bug 69100 - Add logic to collect dirty objects as roots
Summary: Add logic to collect dirty objects as roots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-29 12:43 PDT by Oliver Hunt
Modified: 2011-09-29 15:54 PDT (History)
1 user (show)

See Also:


Attachments
Patch (13.85 KB, patch)
2011-09-29 12:45 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (14.74 KB, patch)
2011-09-29 14:55 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (14.67 KB, patch)
2011-09-29 15:08 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (15.09 KB, patch)
2011-09-29 15:42 PDT, Oliver Hunt
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-09-29 12:43:00 PDT
Add logic to collect dirty objects as roots
Comment 1 Oliver Hunt 2011-09-29 12:45:10 PDT
Created attachment 109190 [details]
Patch
Comment 2 Darin Adler 2011-09-29 13:33:12 PDT
Comment on attachment 109190 [details]
Patch

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

Not sure I’m expert enough in the code to say review+, but it looks good.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:1209
> +    jit.andPtr(TrustedImm32(MarkedBlock::cardCount - 1), scratch2);

Relies on cardCount being a power of 2. The guarantee of that is far away from here. I’d suggest having a mask constant as well as a count so you can use the mask directly here.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:167
> +void MarkedBlock::gatherDirtyObjects(Vector<JSCell*>& dirtyObjects)

Don’t you have to repeat “inline” here for it to be effective?

> Source/JavaScriptCore/heap/AllocationSpace.cpp:179
> +    const size_t metaOffset = firstAtom() * atomSize % objectSize;

What does the const on this line mean? Is this really a compile-time constant?

> Source/JavaScriptCore/heap/AllocationSpace.cpp:197
> +            if (i == m_cards.cardCount - 1 && ((ptr + objectSize) > end))

Seems like there are extra parentheses on this line. I would find it easier to read without any.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:206
> +class TakeIfDirty {

This class should be marked non-copyable.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:210
> +    TakeIfDirty(Vector<JSCell*>*);

This constructor should be explicit.

> Source/JavaScriptCore/heap/Heap.cpp:476
> +    // Perform young gen sweep 90% of the time

This is a perfect example of a “what” comment. What we need here is a “why” comment. Also please use a period and don’t abbreviate generation to “gen”.

> Source/JavaScriptCore/heap/Heap.cpp:493
> +    if (size_t dirtyObjectCount = dirtyObjects.size()) {
> +        for (size_t i = 0; i < dirtyObjectCount; i++) {
> +            visitor.visitChildren(dirtyObjects[i]);
> +            visitor.drain();
> +        }
> +    }

Outer if seems redundant. Loop already does noting if count is zero.

> Source/JavaScriptCore/heap/SlotVisitor.h:34
> +    friend class Heap;

Unpleasant to have to do this.
Comment 3 Geoffrey Garen 2011-09-29 13:46:44 PDT
> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:1209
> +    jit.andPtr(TrustedImm32(MarkedBlock::cardCount - 1), scratch2);

You should make MarkedBlock::cardMask and MarkedBlock::cardShift constants for this.

Would be good to comment this bug fix in your ChangeLog.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:31
> +#include "JSObject.h"

It would be really nice for the Heap not to have to know about JSObject. What is this header used for? I don't see any direct use of JSObject below.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:167
> +void MarkedBlock::gatherDirtyObjects(Vector<JSCell*>& dirtyObjects)

I prefer to use the word "cell" in heap code, to emphasize that these are not JSObjects.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:170
> +    if (m_state == New || m_state == FreeListed)
> +        return;

You should ASSERT you're not in these states.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:173
> +    if (m_state == Allocated)
> +        m_state = Marked;

m_state == Marked means "all unmarked objects in this block are dead". But I don't think that's true after visiting dirty objects. It's possible for the block to contain a newly allocated cell, which is unmarked and not dead. I think this state change is incorrect. (Why are you doing it?)

> Source/JavaScriptCore/heap/AllocationSpace.cpp:178
> +    size_t objectSize = cellSize();

This should be "size_t cellSize = this->cellSize()", to avoid giving two names to one thing.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:179
> +    const size_t metaOffset = firstAtom() * atomSize % objectSize;

I'd call this firstCellOffset.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:186
> +            ptr += metaOffset + objectSize * ((i * bytesPerCard + objectSize - 1 - metaOffset) / objectSize);

I'm afraid of this expression. I hope it's right.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:194
> +            if (isMarked(cell) && cell->structure() && cell->structure()->typeInfo().type() >= CompoundType)

How can cell->structure() be NULL at this point?

Why do a special test for objects without children? (Is it common for old strings to get marked dirty?)

> Source/JavaScriptCore/heap/AllocationSpace.cpp:198
> +            if (i == m_cards.cardCount - 1 && ((ptr + objectSize) > end))
> +                break;

If you start out by setting end to min(end, this + m_endAtom * atomSize), you can avoid these two branches each time through the loop.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:203
> +    m_state = Marked;

Ditto about setting m_state to Marked.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:206
> +class TakeIfDirty {

Let's not call this "Take", since it doesn't remove blocks from the heap. How about "GatherDirtyCells".

> Source/JavaScriptCore/heap/AllocationSpace.cpp:212
> +    ReturnType returnValue() { return 0; }

Another OK option here is to return PassOwnPtr<Vector>, instead of having the calling pass in the Vector.

> Source/JavaScriptCore/heap/Heap.cpp:475
> +    Vector<JSCell*> dirtyObjects;

Inline capacity of 32, please.

> Source/JavaScriptCore/heap/Heap.cpp:478
> +    // Perform young gen sweep 90% of the time
> +    if (WTF::randomNumber() > 0.1)
> +        m_objectSpace.gatherDirtyObjects(dirtyObjects);

This is pretty bad, but I guess it's OK since it's #ifdefed out for now.

> Source/JavaScriptCore/heap/MarkStack.cpp:54
> -inline void SlotVisitor::visitChildren(JSCell* cell)
> +void SlotVisitor::visitChildren(JSCell* cell)

Is this OK for performance? You should perf test with GGC off.

> Source/JavaScriptCore/heap/SlotVisitor.h:34
> +    friend class Heap;

A slightly better solution than making all Heap code a friend is to make HeapRootVisitor a friend (as it already is for MarkStack), and give it a visitChildren function. HeapRootVisitor is our abstraction for "I know I'm doing abnormal marking, but it's OK".
Comment 4 Geoffrey Garen 2011-09-29 13:47:32 PDT
Comment on attachment 109190 [details]
Patch

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

I think there's enough reworking here for an r-.

>> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:1209
>> +    jit.andPtr(TrustedImm32(MarkedBlock::cardCount - 1), scratch2);
> 
> Relies on cardCount being a power of 2. The guarantee of that is far away from here. I’d suggest having a mask constant as well as a count so you can use the mask directly here.

You should make MarkedBlock::cardMask and MarkedBlock::cardShift constants for this.

Would be good to comment this bug fix in your ChangeLog.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:31
> +#include "JSObject.h"

It would be really nice for the Heap not to have to know about JSObject. What is this header used for? I don't see any direct use of JSObject below.

>> Source/JavaScriptCore/heap/AllocationSpace.cpp:167
>> +void MarkedBlock::gatherDirtyObjects(Vector<JSCell*>& dirtyObjects)
> 
> Don’t you have to repeat “inline” here for it to be effective?

I prefer to use the word "cell" in heap code, to emphasize that these are not JSObjects.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:170
> +    if (m_state == New || m_state == FreeListed)
> +        return;

You should ASSERT you're not in these states.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:173
> +    if (m_state == Allocated)
> +        m_state = Marked;

m_state == Marked means "all unmarked objects in this block are dead". But I don't think that's true after visiting dirty objects. It's possible for the block to contain a newly allocated cell, which is unmarked and not dead. I think this state change is incorrect. (Why are you doing it?)

> Source/JavaScriptCore/heap/AllocationSpace.cpp:178
> +    size_t objectSize = cellSize();

This should be "size_t cellSize = this->cellSize()", to avoid giving two names to one thing.

>> Source/JavaScriptCore/heap/AllocationSpace.cpp:179
>> +    const size_t metaOffset = firstAtom() * atomSize % objectSize;
> 
> What does the const on this line mean? Is this really a compile-time constant?

I'd call this firstCellOffset.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:186
> +            ptr += metaOffset + objectSize * ((i * bytesPerCard + objectSize - 1 - metaOffset) / objectSize);

I'm afraid of this expression. I hope it's right.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:194
> +            if (isMarked(cell) && cell->structure() && cell->structure()->typeInfo().type() >= CompoundType)

How can cell->structure() be NULL at this point?

Why do a special test for objects without children? (Is it common for old strings to get marked dirty?)

> Source/JavaScriptCore/heap/AllocationSpace.cpp:198
> +            if (i == m_cards.cardCount - 1 && ((ptr + objectSize) > end))
> +                break;

If you start out by setting end to min(end, this + m_endAtom * atomSize), you can avoid these two branches each time through the loop.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:203
> +    m_state = Marked;

Ditto about setting m_state to Marked.

>> Source/JavaScriptCore/heap/AllocationSpace.cpp:206
>> +class TakeIfDirty {
> 
> This class should be marked non-copyable.

Let's not call this "Take", since it doesn't remove blocks from the heap. How about "GatherDirtyCells".

> Source/JavaScriptCore/heap/AllocationSpace.cpp:212
> +    ReturnType returnValue() { return 0; }

Another OK option here is to return PassOwnPtr<Vector>, instead of having the calling pass in the Vector.

> Source/JavaScriptCore/heap/Heap.cpp:475
> +    Vector<JSCell*> dirtyObjects;

Inline capacity of 32, please.

> Source/JavaScriptCore/heap/Heap.cpp:478
> +    // Perform young gen sweep 90% of the time
> +    if (WTF::randomNumber() > 0.1)
> +        m_objectSpace.gatherDirtyObjects(dirtyObjects);

This is pretty bad, but I guess it's OK since it's #ifdefed out for now.

> Source/JavaScriptCore/heap/MarkStack.cpp:54
> -inline void SlotVisitor::visitChildren(JSCell* cell)
> +void SlotVisitor::visitChildren(JSCell* cell)

Is this OK for performance? You should perf test with GGC off.

>> Source/JavaScriptCore/heap/SlotVisitor.h:34
>> +    friend class Heap;
> 
> Unpleasant to have to do this.

A slightly better solution than making all Heap code a friend is to make HeapRootVisitor a friend (as it already is for MarkStack), and give it a visitChildren function. HeapRootVisitor is our abstraction for "I know I'm doing abnormal marking, but it's OK".
Comment 5 Oliver Hunt 2011-09-29 14:55:36 PDT
Created attachment 109207 [details]
Patch
Comment 6 Darin Adler 2011-09-29 15:00:04 PDT
Comment on attachment 109207 [details]
Patch

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

Looks like you didn’t see my comments. Or disagreed.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:166
> +class GatherDirtyCells {

Normally class names would be nouns, not verbs. Even if it is a function object. Like maybe DirtyCellGatherer.

This class needs to be marked non-copyable.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:170
> +    GatherDirtyCells(MarkedBlock::DirtyCellVector*);

This constructor should be marked explicit.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:190
> +    GatherDirtyCells GatherDirtyCells(&dirtyCells);

The local variable’s first letter should be lowercase.

> Source/JavaScriptCore/heap/Heap.cpp:476
> +    // Perform young gen sweep 90% of the time

Still a “what” comment where we need a “why” comment. Still missing a period. Still abbreviating generation as “gen”.

> Source/JavaScriptCore/heap/Heap.cpp:489
> +    if (size_t dirtyObjectCount = dirtyCells.size()) {
> +        for (size_t i = 0; i < dirtyObjectCount; i++) {

Still unneeded if statement where the loop already deals fine with a count of 0.
Comment 7 Oliver Hunt 2011-09-29 15:05:07 PDT
(In reply to comment #6)
> (From update of attachment 109207 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109207&action=review
> 
> Looks like you didn’t see my comments. Or disagreed.
> 
> > Source/JavaScriptCore/heap/AllocationSpace.cpp:166
> > +class GatherDirtyCells {
> 
> Normally class names would be nouns, not verbs. Even if it is a function object. Like maybe DirtyCellGatherer.
> 
> This class needs to be marked non-copyable.
> 
> > Source/JavaScriptCore/heap/AllocationSpace.cpp:170
> > +    GatherDirtyCells(MarkedBlock::DirtyCellVector*);
> 

I was using Geoff's suggestion for the name

> This constructor should be marked explicit.
> 
> > Source/JavaScriptCore/heap/AllocationSpace.cpp:190
> > +    GatherDirtyCells GatherDirtyCells(&dirtyCells);
> 
> The local variable’s first letter should be lowercase.

Indeed it should be -- i would have expected the style checker to see that (bad search/repalce action)
Comment 8 Oliver Hunt 2011-09-29 15:08:13 PDT
Created attachment 109211 [details]
Patch
Comment 9 Geoffrey Garen 2011-09-29 15:25:22 PDT
Comment on attachment 109211 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:1208
>      jit.rshift32(TrustedImm32(MarkedBlock::log2CardSize), scratch2);

This should use MarkedBlock::cardShift.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:1209
> +    jit.andPtr(TrustedImm32(MarkedBlock::cardMask), scratch2);

Still think you should call out this bug fix in your ChangeLog.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator32_64.cpp:1258
>      jit.rshift32(TrustedImm32(MarkedBlock::log2CardSize), scratch2);

This should use MarkedBlock::cardShift.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:166
> +class GatherDirtyCells {

Should be non-copyable, as Darin mentioned.

> Source/JavaScriptCore/heap/AllocationSpace.cpp:170
> +    GatherDirtyCells(MarkedBlock::DirtyCellVector*);

Should be explicit, as Darin mentioned.

> Source/JavaScriptCore/heap/Heap.cpp:476
> +    // Perform young gen sweep 90% of the time

Should explain why this is the policy, as Darin mentioned.

> Source/JavaScriptCore/heap/MarkedBlock.h:76
> +        static const int log2CardSize = 10; // ~0.1% overhead

Please rename log2CardSize to cardShift, and add a comment specifying this is log2 of cardSize. I think that's a better way to have the name reflect what it's used for, with a comment explaining why it works. (Also, I worry that having a cardMask without a cardShift constant might confuse people into thinking that you can apply just the cardMask to get the card.)

> Source/JavaScriptCore/heap/MarkedBlock.h:319
> +    if (m_state == Allocated)
> +        m_state = Marked;

Now that I understand what this is for:

You have to do this unconditionally, not just for allocated blocks.

I think you should just put this in the GatherDirtyCells functor, with a comment explaining that it's there as an optimization to avoid walking the heap twice.

> Source/JavaScriptCore/heap/MarkedBlock.h:344
> +        end = ptr;

I think this is a dead store. Did you mean to do something with end here?
Comment 10 Oliver Hunt 2011-09-29 15:42:20 PDT
Created attachment 109213 [details]
Patch
Comment 11 Geoffrey Garen 2011-09-29 15:49:31 PDT
Comment on attachment 109213 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:39
> +           Tidy the write barrier a bit.

"I rearranged the order of these instructions because it makes them smaller on some platforms with some card sizes".

> Source/JavaScriptCore/heap/AllocationSpace.cpp:171
> +    GatherDirtyCells(MarkedBlock::DirtyCellVector*);

I award you zero points, and may god have mercy on your soul.
Comment 12 Oliver Hunt 2011-09-29 15:54:59 PDT
Committed r96372