Bug 111382 – [V8] Use implicit references instead of object groups to keep registered MutationObservers alive

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

[Adam Klein]

[V8] Use implicit references instead of object groups to keep registered MutationObservers alive

[Adam Klein]

Created attachment 191357 [details]
Patch

[Adam Barth]

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?

[Adam Klein]

Created attachment 191534 [details]
WIP: two-phase approach

[Adam Barth]

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

[Adam Klein]

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.

[Adam Barth]

OK.  :)

For the object group case, I think we had a large number of groups with only one entry.

[Adam Klein]

Created attachment 191609 [details]
WIP: two-phase with vector

[Adam Klein]

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

[Adam Klein]

Created attachment 191633 [details]
WIP: two-phase vector approach, mark 2

[Adam Klein]

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

[Adam Barth]

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?

[Adam Barth]

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

[Adam Klein]

Created attachment 191847 [details]
Patch for landing

[Adam Klein]

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

[WebKit Review Bot]

Comment on attachment 191847 [details]
Patch for landing

Clearing flags on attachment: 191847

Committed r144994: <http://trac.webkit.org/changeset/144994>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.