Bug 76335 – Reduce HandleHeap churn when managing weak handles.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: JavaScriptCore

[Andreas Kling]

Storing JSC::Weak<T> in HashMaps is currently quite inefficient as we have to go through the copy constructor when storing/peeking into the map. We should come up with a way to avoid churning the HandleHeap in these scenarios.

[Andreas Kling]

Created attachment 122553 [details]
Proposed patch

Here's one way we could improve this - a PassWeak<T> class.

[Andreas Kling]

Created attachment 122572 [details]
Proposed patch

Let's see if Win EWS likes this better.

[Geoffrey Garen]

Comment on attachment 122572 [details]
Proposed patch

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

Just curious: Is this performance issue measurable somewhere?

PassWeak<T> seems like a good idea to me, but it needs to work like PassOwnPtr, not like PassRefPtr, since weak references are single-owner.

> Source/JavaScriptCore/heap/Weak.h:51
> +    PassWeak(const Weak<T>& o)
> +        : m_slot(o.slot())
> +    {
> +    }

The only way to create a PassWeak from a Weak should be to release the Weak. This behavior shares the Handle, which is incorrect, since the original Weak can delete the Handle at any time.

[Andreas Kling]

(In reply to comment #3)
> (From update of attachment 122572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122572&action=review
> 
> Just curious: Is this performance issue measurable somewhere?

Yeah, we're hitting the Weak copy ctor in 1% of samples on the Dromaeo dom-query benchmark. :)

[Andreas Kling]

Created attachment 125553 [details]
Proposed patch v2

[Darin Adler]

Comment on attachment 125553 [details]
Proposed patch v2

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

> Source/JavaScriptCore/heap/Weak.h:41
> +    PassWeak(HandleSlot slot) : m_slot(slot) { }

Seems dangerous to take ownership here with just a type conversion. Normally we’d require an explicit adopt function call.

[Geoffrey Garen]

Darn! Somehow, I missed v2 -- I ended up duplicating most of this code in bug 78740. I think I'll mark this a dup of 78740, since 78740 is a superset, and it addresses Darin's comment.

*** This bug has been marked as a duplicate of bug 78740 ***