[webkit-reviews] review denied: [Bug 55105] Make weaklist processing deal with weak handles being removed during the iteration : [Attachment 83582] 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 55105: Make weaklist processing deal with weak handles being removed during
the iteration
https://bugs.webkit.org/show_bug.cgi?id=55105

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=83582&action=review

I think it's worth making the simplifications I suggested, so I'm going to say
r- here -- let's talk about this on IRC or in person if you disagree.

> Source/JavaScriptCore/collector/handles/HandleHeap.cpp:36
> +    , m_nextToFinalize(0)

I would call this "m_nodeBeingFinalized". It's not the *next* thing that will
finalize -- it's being finalized right now.

> Source/JavaScriptCore/collector/handles/HandleHeap.h:43
> +typedef void (*Finalizer)(JSGlobalData&, Handle<Unknown>, void*);
> +
> +class ComplexFinalizer {

I would call these FinalizerCallback and FinalizerObject. The class isn't
necessarily more complex than the function -- the function can be as crazy as
anyone makes it! -- what distinguishes them is that one is pointer to function,
and the other is pointer to object (with extra data attached to it).

That said, I think you can get away with only one kind of finalizer -- see
below.

> Source/JavaScriptCore/collector/handles/HandleHeap.h:46
> +    virtual ~ComplexFinalizer() {}

This seems wrong. The HandleHeap never deletes a pointer to ComplexFinalizer,
so there's no need for a virtual ~ComplexFinalizer().

> Source/JavaScriptCore/collector/handles/HandleHeap.h:51
> +    enum WeakPointerVisitRule { NormalVisit, IgnoreLiveness };

A simpler solution is just to clearMarks() before doing teardown.

Alternatively, I would name these: ClearAll and ClearkUnmarkedOnly.

> Source/JavaScriptCore/collector/handles/HandleHeap.h:119
> +	   union {
> +	       Finalizer m_finalizer;
> +	       ComplexFinalizer* m_complexFinalizer;
> +	   };
> +	   void* m_finalizerContext;
> +	   uintptr_t m_prevAndHasComplexFinalizer;

I think you could do away with this two-finalizer setup and its related
complexity by just requiring all finalizers to use the "complex" object-based
API. A client that doesn't need context-specific data in its finalizer object
can just use a shared, static object declared inside a function. This is
thread-safe since the object has only one data member -- a
compile-time-constant vtable pointer.