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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for 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 Darin Adler <darin at apple.com>
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.