Bug 100707 – [V8] Garbage collection should use opaque roots rather than implicit references

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

[V8] Garbage collection should use opaque roots rather than implicit references

[Adam Barth]

Created attachment 171325 [details]
Patch

[Adam Barth]

You can find the step-by-step construction of this patch in https://github.com/abarth/webkit/compare/master...newgc

[Adam Barth]

@pfeldman: One concern I have about this patch is whether it has any interaction with the heap profiler.  Does the WebInspector use the RetainedDOMInfo associated with Node object groups?  No LayoutTests seemed to fail when I removed the RetainedDOMInfo from the object group, but I worry that somehow this functionality isn't covered by LayoutTests.  Thanks!

[Kentaro Hara]

Comment on attachment 171325 [details]
Patch

LGTM at a first glance. Will take a detailed look later.

[Kentaro Hara]

Comment on attachment 171325 [details]
Patch

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

The overall approach looks great.

> Source/WebCore/ChangeLog:30
> +        * bindings/scripts/CodeGeneratorV8.pm:

You need to update run-bindings-tests before landing.

> Source/WebCore/bindings/v8/V8GCController.cpp:81
> +public:

private

> Source/WebCore/bindings/v8/V8GCController.cpp:152
> +                m_grouper->keepAlive(wrapper);

I might be wrong: Maybe can you return just after this line?

> Source/WebCore/bindings/v8/V8GCController.cpp:156
> +                m_grouper->keepAlive(wrapper);

Ditto.

> Source/WebCore/bindings/v8/V8GCController.cpp:166
> +// FIXME: This should use opaque GC roots.

Yes, and we want to kill AddImplicitReferences() completely, in a follow-up patch.

> Source/WebCore/bindings/v8/V8GCController.cpp:189
> -static Node* rootForGC(Node* node)
> +void* V8GCController::rootForGarbageCollection(Node* node)

I would prefer rootForGC (or opaqueRootForGC) rather than rootForGarbageCollection, as most of other binding code is using GC for GarbageCollection.

> Source/WebCore/bindings/v8/V8GCController.cpp:223
> +            m_grouper->keepAlive(wrapper);

Ditto. Can't you return just after this line?

> Source/WebCore/bindings/v8/custom/V8NodeListCustom.cpp:63
> +void* V8NodeList::rootForGarbageCollection(void* object, v8::Persistent<v8::Object> wrapper)

wrapper is not used.

BTW, do we need to pass wrapper to rootForGarbageCollection() in the first place?

> Source/WebCore/bindings/v8/custom/V8NodeListCustom.cpp:68
> +    return V8GCController::rootForGarbageCollection(static_cast<DynamicNodeList*>(impl)->ownerNode());

You need to check if static_cast<DynamicNodeList*>(impl)->ownerNode() is 0 or not. If 0, you need to return object.

> Source/WebCore/bindings/v8/custom/V8SpeechRecognitionResultCustom.cpp:41
> -    Document* emma = impl->emma();
> -    v8::Persistent<v8::Value> emmaWrapper = store->domNodeMap().get(emma);
> -    if (!emmaWrapper.IsEmpty())
> -        v8::V8::AddImplicitReferences(wrapper, &emmaWrapper, 1);
> +    return V8GCController::rootForGarbageCollection(impl->emma());

Is this change correct? Here we are intending to add an implicit reference of not emmaWrapper=>wrapper but wrapper=>emmaWrapper.

[Adam Barth]

(In reply to comment #5)
> (From update of attachment 171325 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171325&action=review
> 
> The overall approach looks great.

Thanks!

> > Source/WebCore/ChangeLog:30
> > +        * bindings/scripts/CodeGeneratorV8.pm:
> 
> You need to update run-bindings-tests before landing.

Very true.  Done.

> > Source/WebCore/bindings/v8/V8GCController.cpp:81
> > +public:
> 
> private

Fixed.

> > Source/WebCore/bindings/v8/V8GCController.cpp:152
> > +                m_grouper->keepAlive(wrapper);
> 
> I might be wrong: Maybe can you return just after this line?

We can today, but that might not be future proof.  Although wrapper won't be garbage collected, it's possible that it will share a GC root with another object that isn't an ActiveDOMObject.

Today, that only happens for nodes, but I wanted to make this work the same way for Nodes and non-Nodes in case someone adds something like that in the future.

> > Source/WebCore/bindings/v8/V8GCController.cpp:156
> > +                m_grouper->keepAlive(wrapper);
> 
> Ditto.
>
> > Source/WebCore/bindings/v8/V8GCController.cpp:166
> > +// FIXME: This should use opaque GC roots.
> 
> Yes, and we want to kill AddImplicitReferences() completely, in a follow-up patch.

This is the last use of AddImplicitReferences in WebCore.

> > Source/WebCore/bindings/v8/V8GCController.cpp:189
> > -static Node* rootForGC(Node* node)
> > +void* V8GCController::rootForGarbageCollection(Node* node)
> 
> I would prefer rootForGC (or opaqueRootForGC) rather than rootForGarbageCollection, as most of other binding code is using GC for GarbageCollection.

Done.

> > Source/WebCore/bindings/v8/V8GCController.cpp:223
> > +            m_grouper->keepAlive(wrapper);
> 
> Ditto. Can't you return just after this line?

For this one, we cannot return early.  The problem is HTMLMediaElement, which will end up both in the libe objects group as well as the node group for all the other nodes in the document.

> > Source/WebCore/bindings/v8/custom/V8NodeListCustom.cpp:63
> > +void* V8NodeList::rootForGarbageCollection(void* object, v8::Persistent<v8::Object> wrapper)
> 
> wrapper is not used.
> 
> BTW, do we need to pass wrapper to rootForGarbageCollection() in the first place?

I meant to use it for ASSERTs, but I forgot.  I've added the ASSERTs.

> > Source/WebCore/bindings/v8/custom/V8NodeListCustom.cpp:68
> > +    return V8GCController::rootForGarbageCollection(static_cast<DynamicNodeList*>(impl)->ownerNode());
> 
> You need to check if static_cast<DynamicNodeList*>(impl)->ownerNode() is 0 or not. If 0, you need to return object.
> 
> > Source/WebCore/bindings/v8/custom/V8SpeechRecognitionResultCustom.cpp:41
> > -    Document* emma = impl->emma();
> > -    v8::Persistent<v8::Value> emmaWrapper = store->domNodeMap().get(emma);
> > -    if (!emmaWrapper.IsEmpty())
> > -        v8::V8::AddImplicitReferences(wrapper, &emmaWrapper, 1);
> > +    return V8GCController::rootForGarbageCollection(impl->emma());
> 
> Is this change correct? Here we are intending to add an implicit reference of not emmaWrapper=>wrapper but wrapper=>emmaWrapper.

I'm not sure I quite understand.  You're same that the SpeechRecognitionResult should keep the document alive but the document should not keep the SpeechRecognitionResult alive?  That sounds right.

This code in general seems suspect to me.  I need to look more at what the speech code is trying to do.  It's odd that it needs such special-case treatment.

[Adam Barth]

Created attachment 171532 [details]
Patch

[Adam Barth]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 171325 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171325&action=review
> > 
> > > Source/WebCore/bindings/v8/custom/V8SpeechRecognitionResultCustom.cpp:41
> > > -    Document* emma = impl->emma();
> > > -    v8::Persistent<v8::Value> emmaWrapper = store->domNodeMap().get(emma);
> > > -    if (!emmaWrapper.IsEmpty())
> > > -        v8::V8::AddImplicitReferences(wrapper, &emmaWrapper, 1);
> > > +    return V8GCController::rootForGarbageCollection(impl->emma());
> > 
> > Is this change correct? Here we are intending to add an implicit reference of not emmaWrapper=>wrapper but wrapper=>emmaWrapper.
> 
> I'm not sure I quite understand.  You're same that the SpeechRecognitionResult should keep the document alive but the document should not keep the SpeechRecognitionResult alive?  That sounds right.
> 
> This code in general seems suspect to me.  I need to look more at what the speech code is trying to do.  It's odd that it needs such special-case treatment.

Re-reading <https://bugs.webkit.org/show_bug.cgi?id=91743>, I now remember the issue.  The issue is that the emma property of SpeechRecognitionResult is a document captive on SpeechRecognitionResult, not the document that's displayed in the frame.  That means that the SpeechRecognitionResult and the emma Document should live and die together as a group.

[WebKit Review Bot]

Comment on attachment 171532 [details]
Patch

Attachment 171532 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14660073

New failing tests:
animations/suspend-resume-animation-events.html

[Kentaro Hara]

Comment on attachment 171532 [details]
Patch

Looks OK

[WebKit Review Bot]

Comment on attachment 171532 [details]
Patch

Clearing flags on attachment: 171532

Committed r133044: <http://trac.webkit.org/changeset/133044>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 100888

[Adam Barth]

Created attachment 171726 [details]
Patch for landing

[Adam Barth]

Comment on attachment 171726 [details]
Patch for landing

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:213
> +    ASSERT(!wrapper.IsIndependent());

I added this ASSERT.

> Source/WebCore/bindings/v8/V8GCController.cpp:145
> +        if (wrapper.IsIndependent())
> +            return;

I also added this early exist.  There's no point in trying to group objects that are independent because there's no point in putting them into groups.

[WebKit Review Bot]

Comment on attachment 171726 [details]
Patch for landing

Rejecting attachment 171726 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14677306

[Adam Barth]

Created attachment 171749 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 171749 [details]
Patch for landing

Clearing flags on attachment: 171749

Committed r133113: <http://trac.webkit.org/changeset/133113>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

I think maybe this is/was causing some browser tests to fail; we are going to revert the webkit roll that made this show up downstream ...

http://build.chromium.org/p/chromium.linux/builders/Linux%20%28Precise%29/builds/266/steps/browser_tests/logs/stdio

Annoyingly, it looks like we don't have any debug testers, so we're not going to see the dchecks failing on the canaries. I guess we should fix that :(.

[Adam Barth]

Do you mean the first time this patch landed, or the re-landed version with the fix?  The first landed will definitely hit ASSERTs all over the place.  My understanding is that the second version is clean.

[Dirk Pranke]

(In reply to comment #21)
> Do you mean the first time this patch landed, or the re-landed version with the fix?  The first landed will definitely hit ASSERTs all over the place.  My understanding is that the second version is clean.

first time - r133044.

[Yury Semikhatsky]

(In reply to comment #3)
> @pfeldman: One concern I have about this patch is whether it has any interaction with the heap profiler.  Does the WebInspector use the RetainedDOMInfo associated with Node object groups?  No LayoutTests seemed to fail when I removed the RetainedDOMInfo from the object group, but I worry that somehow this functionality isn't covered by LayoutTests.  Thanks!

Adam what was the reason for removing RetainedDOMInfo parameter from AddObjectGroup? It broke heap profiler: now each node in a detached dom tree is shown in its own group. There were two types of RetainedDOMInfo:
1) Created for a DOM group with its root node passed as a parameter;
2) Created for DOM nodes that were not part of any DOM group with >1 elements, in which case the node itself was passed as a parameter to RetainedDOMInfo. It seems that when we started to traverse all persistent handles the second case should have been eliminated, but not the first.

[Adam Barth]

> Adam what was the reason for removing RetainedDOMInfo parameter from AddObjectGroup?

I'm sorry, but I don't remember.  This patch changed the model for how we do GC grouping.  The issue might have been that we lost the type for the "root" object.  Previously we knew it was a Node* but now we know only that it's a void*.

I suspect if worked a bit more on the new code we could get the RetainedDOMInfo back.  There weren't any tests that failed, so I didn't know if it was needed.

> It broke heap profiler: now each node in a detached dom tree is shown in its own group. There were two types of RetainedDOMInfo:
> 1) Created for a DOM group with its root node passed as a parameter;
> 2) Created for DOM nodes that were not part of any DOM group with >1 elements, in which case the node itself was passed as a parameter to RetainedDOMInfo. It seems that when we started to traverse all persistent handles the second case should have been eliminated, but not the first.

Ok.  Should we open a new bug an try for a fix?

[Adam Barth]

Oh, I see bug 107819 is that bug.

[Yury Semikhatsky]

(In reply to comment #24)
> > Adam what was the reason for removing RetainedDOMInfo parameter from AddObjectGroup?
> 
> I'm sorry, but I don't remember.  This patch changed the model for how we do GC grouping.  The issue might have been that we lost the type for the "root" object.  Previously we knew it was a Node* but now we know only that it's a void*.
> 
> I suspect if worked a bit more on the new code we could get the RetainedDOMInfo back.  There weren't any tests that failed, so I didn't know if it was needed.
> 
Yes, it is sad that there are no tests for that functionality. I'll try to write one for it.