[webkit-reviews] review granted: [Bug 172548] ObjectToStringAdaptiveInferredPropertyValueWatchpoint should not reinstall itself nor handleFire if it's dying shortly. : [Attachment 311163] proposed patch.

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 172548: ObjectToStringAdaptiveInferredPropertyValueWatchpoint should not
reinstall itself nor handleFire if it's dying shortly.
https://bugs.webkit.org/show_bug.cgi?id=172548

Attachment 311163: proposed patch.

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




--- Comment #8 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 311163
  --> https://bugs.webkit.org/attachment.cgi?id=311163
proposed patch.

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

> Source/JavaScriptCore/heap/FreeList.cpp:48
> +bool FreeList::contains(const void* target) const
> +{
> +    if (remaining) {
> +	   const void* start = (payloadEnd - remaining);
> +	   const void* end = payloadEnd;
> +	   return (start <= target) && (target < end);
> +    }
> +
> +    FreeCell* candidate = head;
> +    while (candidate) {
> +	   if (candidate == target)
> +	       return true;
> +	   candidate = candidate->next;
> +    }
> +
> +    return false;
> +}
> +

Nice.

> Source/JavaScriptCore/heap/HeapCellInlines.h:42
> +    if (markedBlockHandle.isFreeListed())
> +	   return !markedBlockHandle.isFreeListedCell(this);

I think you commented to me earlier that we could say that free-listed objects
are "live".  I get now that this would be correct for the purposes of your
patch, but would just be inconsistent with the GC's lingo (in other contexts
it's important to know that you're not "live" if you're still on the free
list).

Maybe we want to eventually add something like
HeapCell::isPendingDestruction(), which could just be:

if (isLargeAllocation())
    return !largeAllocation().isLive();
auto& markedBlockHandle = markedBlock().handle();
if (markedBlockhandle.isFreeListed())
    return false;
return !markedBlockHandle.isLive(this);

Basically, we'd say that isPendingDestruction() can only be called on objects
that are either live right now, or objects that ceased to be live but have not
yet been destructed.

You don't have to make this change.  It's just an observation.