1466979 - Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Kris Wright :KrisWright]

JS::ubi::EdgeVectorTracer[1] and JS::ubi::SimpleEdgeRange[2] need to be separated into a utils header file where they can be further accessed in other locations. Also, SimpleEdgeRange needs to be updated to attach single edges that do not come from the GC.

[1] https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/js/src/vm/UbiNode.cpp#226-286
[2] https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/js/src/vm/UbiNode.cpp#289-310

Comment 1

[Kris Wright :KrisWright]

Attachment: Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h

Separated the classes into the new UbiNodeUtils.h, updated build files, changed SimpleEdgeRange::init to SimpleEdgeRange::addTracerEdges, created bool addEdge to add new edges to the vector.

Comment 2

[Kris Wright :KrisWright]

Attachment: Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h

Separated the classes into the new UbiNodeUtils.h, updated build files, changed SimpleEdgeRange::init to SimpleEdgeRange::addTracerEdges, created bool addEdge to add new edges to the vector.

Comment 3

[Kris Wright :KrisWright]

Attachment: Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h

Separated the classes into the new UbiNodeUtils.h, updated build files, changed SimpleEdgeRange::init to SimpleEdgeRange::addTracerEdges, created bool addEdge to add new edges to the vector.

Comment 4

[Kris Wright :KrisWright]

Attachment: Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h

Separated the classes into the new UbiNodeUtils.h, updated build files, changed SimpleEdgeRange::init to SimpleEdgeRange::addTracerEdges, created bool addEdge to add new edges to the vector.

Comment 5

[Kris Wright :KrisWright]

Attachment: Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h

Separated the classes into the new UbiNodeUtils.h, updated build files, changed SimpleEdgeRange::init to SimpleEdgeRange::addTracerEdges, created bool addEdge to add new edges to the vector.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8989573 [details] [diff] [review]
Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h

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

This looks fine overall. I have some minor changes I would like; see the comments. I've cleared the review flag, indicating the patch should not land as-is; but set the "feedback+" flag, which indicates agreement with the overall approach.

I noticed that this patch doesn't apply to current mozilla-central; I think this is because someone else went through and removed all uses of mozilla::Move, since we can just use std::move now. (It seems like you correctly dropped the use of Move in `EdgeVectorTracer::onChild`, where it was redundant.)

If you can rebase the patch series so that it applies cleanly, we can just have the tree sheriffs land the patch for us once it's approved, which will be convenient.

::: js/public/UbiNodeUtils.h
@@ +23,5 @@
> +namespace ubi {
> +
> +// A JS::CallbackTracer subclass that adds a Edge to a Vector for each
> +// edge on which it is invoked.
> +class EdgeVectorTracer : public JS::CallbackTracer {

Isn't EdgeVectorTracer used only within addTracerEdges? Since that method's definition is still in UbiNode.cpp, it would be nice to keep this this implementation detail isolated to that file. So if possible, let's keep EdgeVectortracer in UbiNode.cpp.

::: js/src/vm/UbiNode.cpp
@@ +473,5 @@
> +    }
> +}
> +
> +bool
> +SimpleEdgeRange::addTracerEdges(JSRuntime* rt, void* thing, JS::TraceKind kind, bool wantNames = true) {

Since the default value for wantNames is given in the method declaration in the header file, we don't need to reiterate it here.

Comment 7

[Kris Wright :KrisWright]

Attachment: Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h

Separated SimpleEdgeRange into the new UbiNodeUtils.h, updated build files, changed SimpleEdgeRange::init to SimpleEdgeRange::addTracerEdges, created bool addEdge to add new edges to the vector.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 8990860 [details] [diff] [review]
Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h

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

Looks good.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/caab8817cbf2
Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h r=jimb

Comment 10

[Arthur Iakab [arthur_iakab]]

Backed out changeset caab8817cbf2 (bug 1466979)For build bustages on js/src/vm/UbiNode.cpp CLOSED TREE
Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/8a34a37ea1f4d0994e82af2db4d1df5e09ef4a9e
Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=caab8817cbf2bf777fdc3684d4d3cfb64f5725f5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Link to the log: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=caab8817cbf2bf777fdc3684d4d3cfb64f5725f5&selectedJob=187455658

Comment 11

[Kris Wright :KrisWright]

Attachment: Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h

Separated SimpleEdgeRange into the new UbiNodeUtils.h, updated build files, changed SimpleEdgeRange::init to SimpleEdgeRange::addTracerEdges, created bool addEdge to add new edges to the vector.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/383714d5acad
Separate JS::ubi::EdgeVectorTracer and JS::ubi::SimpleEdgeRange into UbiNodeUtils.h r=jimb

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/383714d5acad