Bug 55105 – Make weaklist processing deal with weak handles being removed during the iteration

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Make weaklist processing deal with weak handles being removed during the iteration

[Oliver Hunt]

Created attachment 83582 [details]
Patch

[Geoffrey Garen]

Comment on attachment 83582 [details]
Patch

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.

[Oliver Hunt]

(In reply to comment #2)
> (From update of attachment 83582 [details])
> 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.

No it's not.  It is the next thing to be finalized.  We're currently finalising the node pointed to be current, the next node to finalize is pointed to by the local 'node', and m_nextToFinalize is pointing to node.

> 
> > 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().

virtual methods means you need a virtual destructor, otherwise we get a warning and so fail to build :-(

> 
> > 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.

Either works, i'm not sure which i prefer.

> 
> > 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.

Righto.

[Oliver Hunt]

Created attachment 83679 [details]
Patch

[Gavin Barraclough]

Comment on attachment 83679 [details]
Patch

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

Please put up a new patch & ping me if you think these changes need a rereview, but r+ with changes above.

> Source/JavaScriptCore/collector/handles/HandleHeap.cpp:82
> +        

Please to be removing the whitespace change!

> Source/JavaScriptCore/collector/handles/HandleHeap.cpp:97
> +            m_nextToFinalize = &node;
> +            deallocate(toHandle(current));
> +            node = *m_nextToFinalize;
> +            m_nextToFinalize = 0;

Per our discussion, since current cannot == node, can remove the m_nextToFinalize dance & replace with an ASSERT.

> Source/JavaScriptCore/collector/handles/HandleHeap.h:124
> +    Node** m_nextToFinalize;

Per discussion, it seems this can be a Node* rather than a Node**.

[Oliver Hunt]

Committed r79601: <http://trac.webkit.org/changeset/79601>