Bug 76335 - Reduce HandleHeap churn when managing weak handles.
Summary: Reduce HandleHeap churn when managing weak handles.
Status: RESOLVED DUPLICATE of bug 78740
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-14 10:23 PST by Andreas Kling
Modified: 2012-02-18 10:36 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch (8.85 KB, patch)
2012-01-14 10:57 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch (10.72 KB, patch)
2012-01-15 12:17 PST, Andreas Kling
ggaren: review-
Details | Formatted Diff | Diff
Proposed patch v2 (11.81 KB, patch)
2012-02-05 19:51 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2012-01-14 10:23:20 PST
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.
Comment 1 Andreas Kling 2012-01-14 10:57:12 PST
Created attachment 122553 [details]
Proposed patch

Here's one way we could improve this - a PassWeak<T> class.
Comment 2 Andreas Kling 2012-01-15 12:17:43 PST
Created attachment 122572 [details]
Proposed patch

Let's see if Win EWS likes this better.
Comment 3 Geoffrey Garen 2012-01-16 12:33:04 PST
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.
Comment 4 Andreas Kling 2012-02-03 23:14:00 PST
(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. :)
Comment 5 Andreas Kling 2012-02-05 19:51:37 PST
Created attachment 125553 [details]
Proposed patch v2
Comment 6 Darin Adler 2012-02-06 17:34:24 PST
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.
Comment 7 Geoffrey Garen 2012-02-15 14:58:26 PST
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 ***