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.
Created attachment 122553 [details] Proposed patch Here's one way we could improve this - a PassWeak<T> class.
Created attachment 122572 [details] Proposed patch Let's see if Win EWS likes this better.
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.
(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. :)
Created attachment 125553 [details] Proposed patch v2
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.
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 ***