[webkit-reviews] review granted: [Bug 82670] First step toward incremental Weak<T> finalization : [Attachment 135219] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 82670: First step toward incremental Weak<T> finalization
https://bugs.webkit.org/show_bug.cgi?id=82670

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

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=135219&action=review


R=me except for the one comment about possibly not having to do a round of
tracing after handling dead weak handles.

> Source/JavaScriptCore/heap/Heap.cpp:716
> +	   {
> +	       ParallelModeEnabler enabler(visitor);
> +	       visitor.donateAndDrain();
> +#if ENABLE(PARALLEL_GC)
> +	       visitor.drainFromShared(SlotVisitor::MasterDrain);
> +#endif
> +	   }

Why do we have to retrace?  I don't see marking in visitDeadWeakImpls.

> Source/JavaScriptCore/heap/Heap.h:138
> +	   WeakHeap* weakHeap() { return &m_weakHeap; }

Random nit: whenever I look at this code I get confused by the terms "WeakHeap"
and "HandleHeap".  Those aren't really heaps.  I mean, they do some of their
own memory management, but so do many things in our code, and yet we don't call
them "heaps" for that reason.

I feel like in the future - not in this patch - it might be good to rename
"HandleHeap" and "WeakHeap" to "HandleSet" and "WeakSet".  Thoughts?

> Source/JavaScriptCore/heap/WeakHeap.h:73
> +    if (!allocator)

UNLIKELY() ?