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

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied 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 109211: Patch
https://bugs.webkit.org/attachment.cgi?id=109211&action=review

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