1102541 - JS::ubi::RootList should have a method for manually adding ubi::Nodes as roots

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

If some ubi::Node isn't technically a GC root (like a debuggee global), we might want to pretend it is anyways, and it would be nice if RootList had an API to support that.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: add-root-to-root-list.patch

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8534542 [details] [diff] [review]
add-root-to-root-list.patch

Review of attachment 8534542 [details] [diff] [review]:
-----------------------------------------------------------------

More straightforward, nice.

::: js/src/vm/Debugger.cpp
@@ +3575,5 @@
> +            /*
> +             * Ensure that each of our debuggee globals are in the root list.
> +             */
> +            for (GlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) {
> +                if (!rootList.addRoot(JS::ubi::Node(static_cast<JSObject *>(r.front()))))

Do you need the static_cast?

@@ +3578,5 @@
> +            for (GlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) {
> +                if (!rootList.addRoot(JS::ubi::Node(static_cast<JSObject *>(r.front()))))
> +                    return false;
> +            }
> +

Hm, why not put this logic into RootList::init(HandleObject)? That already expects a Debugger object and operates on its debuggee zones, so might as well just add the globals. I don't feel that strongly though.

::: js/src/vm/DebuggerMemory.cpp
@@ +779,5 @@
>              return false;
>  
> +        // Add all debuggee globals as roots.
> +        for (GlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) {
> +            if (!rootList.addRoot(JS::ubi::Node(static_cast<JSObject *>(r.front()))))

Ditto on if you need this static_cast.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Shu-yu Guo [:shu] from comment #2)
> ::: js/src/vm/Debugger.cpp
> @@ +3575,5 @@
> > +            /*
> > +             * Ensure that each of our debuggee globals are in the root list.
> > +             */
> > +            for (GlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) {
> > +                if (!rootList.addRoot(JS::ubi::Node(static_cast<JSObject *>(r.front()))))
> 
> Do you need the static_cast?

Yup, I get compiler errors without it here and in the other location.

> @@ +3578,5 @@
> > +            for (GlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) {
> > +                if (!rootList.addRoot(JS::ubi::Node(static_cast<JSObject *>(r.front()))))
> > +                    return false;
> > +            }
> > +
> 
> Hm, why not put this logic into RootList::init(HandleObject)? That already
> expects a Debugger object and operates on its debuggee zones, so might as
> well just add the globals. I don't feel that strongly though.

Good idea, will do.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: add-root-to-root-list.patch

Updated based on review comments. Carrying over shu's r+.

Tree is closed right now, so can't land it yet.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0064d8949f0c

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/93ede5de8bd3 for build bustage:


https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4597464&repo=mozilla-inbound

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I don't understand why the compiler on linux doesn't like this...

It is complaining about GlobalObjectSet:

> 12:39:47 INFO - /builds/slave/m-in-lx-d-00000000000000000000/build/src/js/src/vm/UbiNode.cpp: In member function 'bool JS::ubi::RootList::init(JS::HandleObject)':
> 12:39:47 INFO - /builds/slave/m-in-lx-d-00000000000000000000/build/src/js/src/vm/UbiNode.cpp:330:10: error: 'GlobalObjectSet' has not been declared 

But in GlobalObject.h GlobalObjectSet gets defined (http://dxr.mozilla.org/mozilla-central/source/js/src/vm/GlobalObject.h?from=GlobalObjectSet#809):

> typedef HashSet<GlobalObject *, DefaultHasher<GlobalObject *>, SystemAllocPolicy> GlobalObjectSet;

And in UbiNode.cpp we include that header (http://dxr.mozilla.org/mozilla-central/source/js/src/vm/UbiNode.cpp?from=UbiNode.cpp#24):

> #include "vm/GlobalObject.h"

Any idea what's up here, shu?

Comment 8

[Jan de Mooij [:jandem]]

GlobalObjectSet is in the js namespace, so maybe js::GlobalObjectSet?

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> GlobalObjectSet is in the js namespace, so maybe js::GlobalObjectSet?

Sigh, thanks >_<

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4d0e37abcb38

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: add-root-to-root-list.patch

Updated patch from the green try push.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9ce705adaf

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b9ce705adaf