[webkit-reviews] review granted: [Bug 174811] GC should be fine with trading blocks between destructor and non-destructor blocks : [Attachment 316351] the patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 174811: GC should be fine with trading blocks between destructor and
non-destructor blocks
https://bugs.webkit.org/show_bug.cgi?id=174811

Attachment 316351: the patch

https://bugs.webkit.org/attachment.cgi?id=316351&action=review




--- Comment #5 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 316351
  --> https://bugs.webkit.org/attachment.cgi?id=316351
the patch

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

r=me

> Source/JavaScriptCore/heap/MarkedBlockInlines.h:330
> +		       specializedSweep<true, NotEmpty, SweepOnly,
BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated,
MarksNotStale>(freeList, IsEmpty, SweepOnly, BlockHasDestructors, DontScribble,
DoesNotHaveNewlyAllocated, MarksNotStale, destroyFunc);
> +		       return true;
> +		   case MarksStale:
> +		       specializedSweep<true, NotEmpty, SweepOnly,
BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated,
MarksStale>(freeList, IsEmpty, SweepOnly, BlockHasDestructors, DontScribble,
DoesNotHaveNewlyAllocated, MarksStale, destroyFunc);
> +		       return true;
> +		   }
> +	       case SweepToFreeList:
> +		   switch (marksMode) {
> +		   case MarksNotStale:
> +		       specializedSweep<true, NotEmpty, SweepToFreeList,
BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated,
MarksNotStale>(freeList, IsEmpty, SweepToFreeList, BlockHasDestructors,
DontScribble, DoesNotHaveNewlyAllocated, MarksNotStale, destroyFunc);
> +		       return true;
> +		   case MarksStale:
> +		       specializedSweep<true, NotEmpty, SweepToFreeList,
BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated,
MarksStale>(freeList, IsEmpty, SweepToFreeList, BlockHasDestructors,
DontScribble, DoesNotHaveNewlyAllocated, MarksStale, destroyFunc);

Behavior-wise, it doesn't matter that you pass "IsEmpty" for the emptyMode (2nd
argument) because it will get overridden to NotEmpty in specializedSweep()
because the specialize flag is true.  However, for consistency, I think we
should pass NotEmpty given that we know that the block's emptyMode is NotEmpty.