[webkit-reviews] review denied: [Bug 69100] Add logic to collect dirty objects as roots : [Attachment 109190] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied  review:
Bug 69100: Add logic to collect dirty objects as roots
https://bugs.webkit.org/show_bug.cgi?id=69100

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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".