Bug 69100 – Add logic to collect dirty objects as roots

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Add logic to collect dirty objects as roots

[Oliver Hunt]

Created attachment 109190 [details]
Patch

[Darin Adler]

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.

[Geoffrey Garen]

> 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".

[Geoffrey Garen]

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".

[Oliver Hunt]

Created attachment 109207 [details]
Patch

[Darin Adler]

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.

[Oliver Hunt]

(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)

[Oliver Hunt]

Created attachment 109211 [details]
Patch

[Geoffrey Garen]

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?

[Oliver Hunt]

Created attachment 109213 [details]
Patch

[Geoffrey Garen]

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.

[Oliver Hunt]

Committed r96372