Bug 111382 - [V8] Use implicit references instead of object groups to keep registered MutationObservers alive
Summary: [V8] Use implicit references instead of object groups to keep registered Muta...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-04 16:42 PST by Adam Klein
Modified: 2013-03-06 15:58 PST (History)
8 users (show)

See Also:


Attachments
Patch (11.60 KB, patch)
2013-03-04 17:01 PST, Adam Klein
no flags Details | Formatted Diff | Diff
WIP: two-phase approach (5.81 KB, patch)
2013-03-05 12:30 PST, Adam Klein
no flags Details | Formatted Diff | Diff
WIP: two-phase with vector (5.97 KB, patch)
2013-03-05 17:12 PST, Adam Klein
no flags Details | Formatted Diff | Diff
WIP: two-phase vector approach, mark 2 (5.76 KB, patch)
2013-03-05 18:32 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (8.16 KB, patch)
2013-03-06 15:17 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2013-03-04 16:42:54 PST
[V8] Use implicit references instead of object groups to keep registered MutationObservers alive
Comment 1 Adam Klein 2013-03-04 17:01:12 PST
Created attachment 191357 [details]
Patch
Comment 2 Adam Barth 2013-03-04 17:13:53 PST
Comment on attachment 191357 [details]
Patch

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

> Source/WebCore/bindings/v8/V8GCController.cpp:168
> +        observerWrappers.append(v8::Persistent<v8::Value>((*iter)->wrapper()));

I don't think this will work correctly for MutationObservers created by content scripts.  They'll have their wrappers stored in the DOMDataStore hashmap, not inline in the object.

> Source/WebCore/dom/Node.cpp:2310
> +            observers.add(registry->at(i)->observer());

Do you need the de-duping of the HashSet?
Comment 3 Adam Klein 2013-03-05 12:30:06 PST
Created attachment 191534 [details]
WIP: two-phase approach
Comment 4 Adam Barth 2013-03-05 13:26:38 PST
Comment on attachment 191534 [details]
WIP: two-phase approach

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

> Source/WebCore/bindings/v8/V8GCController.cpp:105
> +        HashMap<void*, OwnPtr<HashSet<void*> > >::AddResult result = m_implicitReferences.add(parent, nullptr);

So many hash maps and sets....

If you look on line 113 of this file (pre-patch), you'll see that we use std::sort to do the object grouping instead of using a hashset.  We found by benchmarking the GC time that std::sort was significantly faster (presumably because the mallocs are expensive).  I wonder if we should use a sorting based algorithm for the implicit references too.

The benchmarks I used were the gc-* tests in http://trac.webkit.org/browser/trunk/PerformanceTests/Bindings/.
Comment 5 Adam Klein 2013-03-05 16:09:52 PST
Comment on attachment 191534 [details]
WIP: two-phase approach

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

>> Source/WebCore/bindings/v8/V8GCController.cpp:105
>> +        HashMap<void*, OwnPtr<HashSet<void*> > >::AddResult result = m_implicitReferences.add(parent, nullptr);
> 
> So many hash maps and sets....
> 
> If you look on line 113 of this file (pre-patch), you'll see that we use std::sort to do the object grouping instead of using a hashset.  We found by benchmarking the GC time that std::sort was significantly faster (presumably because the mallocs are expensive).  I wonder if we should use a sorting based algorithm for the implicit references too.
> 
> The benchmarks I used were the gc-* tests in http://trac.webkit.org/browser/trunk/PerformanceTests/Bindings/.

I'll have to make some gc-* tests that actually touch this code to do a proper comparison, but I will note that using a Vector of pairs and sorting in place of a HashMap<void, HashSet<void> > was slow enough that it timed out one of the tests in fast/dom/MutationObserver/ in debug mode.
Comment 6 Adam Barth 2013-03-05 16:21:51 PST
OK.  :)

For the object group case, I think we had a large number of groups with only one entry.
Comment 7 Adam Klein 2013-03-05 17:12:49 PST
Created attachment 191609 [details]
WIP: two-phase with vector
Comment 8 Adam Klein 2013-03-05 17:14:51 PST
(In reply to comment #6)
> OK.  :)
> 
> For the object group case, I think we had a large number of groups with only one entry.

Turns out I had an infinite loop in my vector version. So that's definitely the timeout :)

Uploaded for contrast, will work on perf tests in the meanwhile. I think I'd expect a similar profile to that of the groupings, i.e., lots of places where each node only has one MutationObserver associated with it.
Comment 9 Adam Klein 2013-03-05 18:32:35 PST
Created attachment 191633 [details]
WIP: two-phase vector approach, mark 2
Comment 10 Adam Klein 2013-03-06 12:02:44 PST
(In reply to comment #9)
> Created an attachment (id=191633) [details]
> WIP: two-phase vector approach, mark 2

I'm not able to discern any performance difference between these approaches, at least not for "reasonable" cases:

- one observer, one observed node
- one observer, many observed nodes
- many observers, one observed node
- many observers, many observed nodes

So I'm happy to go with whichever you'd prefer. Note that the vector solution could perform quite a bit worse if a single observer somehow manages to have wrappers in different worlds, but that seems quite unlikely.
Comment 11 Adam Barth 2013-03-06 14:47:15 PST
Comment on attachment 191633 [details]
WIP: two-phase vector approach, mark 2

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

> Source/WebCore/bindings/v8/V8GCController.cpp:149
> +            {

Why have these { } ?

> Source/WebCore/bindings/v8/V8GCController.cpp:173
> +            v8::V8::AddImplicitReferences(v8::Persistent<v8::Object>::Cast(parentWrapper), children.data(), children.size());

Should we ASSERT that parentWrapper is really an object before casting it?  (Maybe the cast does that assert for us?

It's a bit odd to have this cast here.  I guess AddImplicitReferences needs an object as the first argument but AddObjectGroup can deal with values?
Comment 12 Adam Barth 2013-03-06 14:48:46 PST
(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=191633) [details] [details]
> > WIP: two-phase vector approach, mark 2
> 
> I'm not able to discern any performance difference between these approaches, at least not for "reasonable" cases:
> 
> - one observer, one observed node
> - one observer, many observed nodes
> - many observers, one observed node
> - many observers, many observed nodes
> 
> So I'm happy to go with whichever you'd prefer.

Thanks for looking at the performance.  I'm not sure it makes a big difference either way.

> Note that the vector solution could perform quite a bit worse if a single observer somehow manages to have wrappers in different worlds, but that seems quite unlikely.

Yes, although that's something we should keep in mind if we start using this mechanism for more kinds of objects.
Comment 13 Adam Klein 2013-03-06 15:17:42 PST
Created attachment 191847 [details]
Patch for landing
Comment 14 Adam Klein 2013-03-06 15:18:02 PST
Comment on attachment 191633 [details]
WIP: two-phase vector approach, mark 2

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

>> Source/WebCore/bindings/v8/V8GCController.cpp:149
>> +            {
> 
> Why have these { } ?

Just to scope the "iter" name, but not needed for now. I'll remove the {}

>> Source/WebCore/bindings/v8/V8GCController.cpp:173
>> +            v8::V8::AddImplicitReferences(v8::Persistent<v8::Object>::Cast(parentWrapper), children.data(), children.size());
> 
> Should we ASSERT that parentWrapper is really an object before casting it?  (Maybe the cast does that assert for us?
> 
> It's a bit odd to have this cast here.  I guess AddImplicitReferences needs an object as the first argument but AddObjectGroup can deal with values?

I believe the Cast will ASSERT for us.

Sadly this is just weirdness in the V8 API. I tried changing most of V8GCController.cpp to deal with Object handles, but AddObjectGroup needs Values.

I've managed to rewrite this with no Casts, though (besides the pre-existing Cast in VisitPersistentHandle).
Comment 15 WebKit Review Bot 2013-03-06 15:58:30 PST
Comment on attachment 191847 [details]
Patch for landing

Clearing flags on attachment: 191847

Committed r144994: <http://trac.webkit.org/changeset/144994>
Comment 16 WebKit Review Bot 2013-03-06 15:58:35 PST
All reviewed patches have been landed.  Closing bug.