892643 - SpiderMonkey needs a root type with unrestricted lifetime that can be copied and assigned

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

SpiderMonkey has no kind of rooted reference that has unrestricted lifetime, and can be copied and assigned with the usual C++ operators.

We should have a template like Rooted, perhaps called DynamicRooted, that uses mfbt/LinkedList.h to add its instances to and remove them from a doubly-linked list, owned by the runtime. LinkedList is an 'intrusive' linked list template, meaning that the links live in the list elements themselves; insertion and deletion require no allocation, and are infallible.

The copy constructor for DynamicRooted could simply insert itself in the same linked list as the original; because it's a doubly-linked list, it doesn't need a pointer to the JSRuntime.

The assignment operator, obviously, would just change the value being rooted, and leave the links alone.

If you look for where gcRootsHash (the table that JS_AddObjectRoot works with) gets dealt with, those are the places that the linked list would need to be dealt with, too. There aren't many.

To handle all the different kinds of roots, you'd probably want to have a ThingRootKind element to say what kind of thing it was, and then a switch resembling the 'if' chain for gcRootsHash in js::gc::MarkRuntime (redundant structure, *sigh*), rather than a virtual function. (Perhaps you could pull out the switch from MarkExactStackRoot into its own function, and just used that...)

Then, CompileOptions could simply use DynamicRooted<JSObject *>.

The name 'DynamicRooted' has been bikeshedded in bug 887077 comment 3. Massive off-topic renames have been suggested in bug 887039 comment 3. My personal strategy would be to ignore both of those and see how far you can get. :)

Comment 1

[Jim Blandy :jimb]

It's not clear that SpiderMonkey *does* need such a type any more. CompileOptions was the motivating use case, and CompileOptions probably needs to be a MOZ_STACK_CLASS type anyway, so using a HandleObject in there should be fine.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Proposed solution

Since I'm running into this problem with CompileOptions, I've cooked up a potential solution. I'd like your feedback on my approach before I continue.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 775662 [details] [diff] [review]
Proposed solution

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

This is pretty clean, but I'd like two changes:

1) Include a user with the patch so that we can see how much benefit we're getting by adding this instead of an in-place hack.

2) Simplify the list structure as indicated below.

::: js/src/gc/GCInternals.h
@@ +142,5 @@
>      AutoStopVerifyingBarriers(JSRuntime *, bool) {}
>  };
>  #endif /* JS_GC_ZEAL */
>  
> +void 

Trailing space.

::: js/src/gc/RootMarking.cpp
@@ +793,5 @@
> +js::gc::MarkDynamicRoots(JSTracer *trc)
> +{
> +    JSRuntime *rt = trc->runtime;
> +    js::DynamicRooted *node = rt->gcRootsList;
> +    if (node) {

while (node && node != rt->gcRootsList)

Don't bother lifting the NULL check: it adds non-zero cognitive overhead and the virtual dispatch is going to dominate the performance anyway. Also, |node| is basically guaranteed to be in a register so it's not going to add memory traffic.

Also, see later.

::: js/src/jsgc.cpp
@@ +5085,5 @@
> +        next_->prev_ = this;
> +    if (prev_)
> +        prev_->next_ = this;
> +
> +    rt->gcRootsList = this;

I think this list implementation is more advanced than we need: e.g. we're not using the circularity, just the double-linkedness. How about we switch to gcRootsList just being a  head pointer?

insert:
next_ = rt->gcRootsList;
if (next_)
    next_->prev_ = this;
rt->gcRootsList = this;

remove:
if (next_)
    next_->prev_ = prev_;
if (prev_)
    prev_->next_ = next_;
if (rt->gcRootsList == this)
    rt->gcRootsList = next_;

Then the iteration is just:
while (node) {
    node->trace(trc);
    node = node->next_;
}

::: js/src/vm/Runtime.h
@@ +888,5 @@
>      js::gc::Chunk       *gcUserAvailableChunkListHead;
>      js::gc::ChunkPool   gcChunkPool;
>  
>      js::RootedValueMap  gcRootsHash;
> +    js::DynamicRooted   *gcRootsList;

I think gcDynamicRootsList, as we have tons of other GC roots lists as well.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 775662 [details] [diff] [review]
Proposed solution

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

Looks all right to me, but I have some larger concerns that I'll post separately.

::: js/public/RootingAPI.h
@@ +1002,5 @@
> +    void remove();
> +
> +    JSContext *cx_;
> +    DynamicRooted *next_;
> +    DynamicRooted *prev_;

We have a really sweet doubly-linked list template (that stores the links in the element, as you're doing here) in mfbt/LinkedList.h. We should just use that instead of giving readers Yet Another Doubly Linked List to read and wonder if it's what they expect (and reviewers an excuse to bikeshed you! :) )

Comment 9

[Jim Blandy :jimb]

As I said in comment 1, I'm not sure this whole bug *should* be solved any more.

1) I now think CompileOptions should be restricted to be used on the stack only. If it's going to live in the heap, then not only does it need to properly root element and elementProperty, but it needs to own its own copies of the string arguments. For almost all its uses --- stacklike --- that's unnecessary complexity and overhead.

The only non-stack-like use of CompileOptions is in an asm.js failure case (where it decides it can't generate super-fast code for it). That use can be replaced by custom code.

2) Even if we did want to use a nice root type like the one proposed in this bug in CompileOptions, the use in asm.js would still be a bug, because asm.js is using CompileOptions in a structure owned by a JavaScript function (the asm.js 'module'). You can't put roots in objects owned by JS objects; you must only trace them when the owning JS object is traced. Otherwise, when a GC begins, the referents of the CompileOptions are treated as live (because they're *roots*), even if the owning JS object is dead. If there is any path from those referents back to the JS object, the whole blob of objects will never be freed.

So the immediate motivation for this type is gone (i.e., CompileOptions must be stack-like, so a HandleObject should be okay there); and the long-term motivation looks shaky (these root types make it too easy to introduce leaks).

Comment 10

[Eddy Bruel [:ejpbruel]]

(In reply to Jim Blandy :jimb from comment #9)
> As I said in comment 1, I'm not sure this whole bug *should* be solved any
> more.
> 
> 1) I now think CompileOptions should be restricted to be used on the stack
> only. If it's going to live in the heap, then not only does it need to
> properly root element and elementProperty, but it needs to own its own
> copies of the string arguments. For almost all its uses --- stacklike ---
> that's unnecessary complexity and overhead.
> 
> The only non-stack-like use of CompileOptions is in an asm.js failure case
> (where it decides it can't generate super-fast code for it). That use can be
> replaced by custom code.
> 
> 2) Even if we did want to use a nice root type like the one proposed in this
> bug in CompileOptions, the use in asm.js would still be a bug, because
> asm.js is using CompileOptions in a structure owned by a JavaScript function
> (the asm.js 'module'). You can't put roots in objects owned by JS objects;
> you must only trace them when the owning JS object is traced. Otherwise,
> when a GC begins, the referents of the CompileOptions are treated as live
> (because they're *roots*), even if the owning JS object is dead. If there is
> any path from those referents back to the JS object, the whole blob of
> objects will never be freed.
> 
> So the immediate motivation for this type is gone (i.e., CompileOptions must
> be stack-like, so a HandleObject should be okay there); and the long-term
> motivation looks shaky (these root types make it too easy to introduce
> leaks).

Fair enough. However, according to https://bugzilla.mozilla.org/show_bug.cgi?id=887077#c17, off thread parses creates CompileOptions on the heap as well. How are we going to deal with those?

Comment 11

[Jeff Walden [:Waldo]]

(In reply to Jim Blandy :jimb from comment #9)
> You can't put roots in objects owned by JS objects; you must only trace them
> when the owning JS object is traced. Otherwise, when a GC begins, the
> referents of the CompileOptions are treated as live (because they're
> *roots*), even if the owning JS object is dead. If there is any path from
> those referents back to the JS object, the whole blob of objects will never
> be freed.

Excellent, concise summary.  I've been missing one of these in this discussion prior to now.

> and the long-term motivation looks shaky (these root types make it too easy
> to introduce leaks).

I think this may be viewing the situation through Gecko-tinted glasses.  In the Gecko world, pretty much everything is owned by something whose ownership can be tied to a JS object.  I'm not certain this is the case for other embedders (although it doubtless is for some).  We should ask them about it, before deciding this is too risky to introduce.  They can -- and, given the state of our JS_Add*Root documentation:

https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_AddRoot

...probably do -- have dynamic roots managed by C++ objects owned by JS objects now, with the resultant potential for leaks.  The idea proposed here introduces nothing they couldn't do before, at small cost in efficiency.  I think it likely that they *do* hand-roll this from the add/remove-root primitives (which come with no warnings about misuse) seemingly at their disposal now.

Comment 12

[Jim Blandy :jimb]

It is certainly true that anywhere that JS_Add*Root is correct, the types described here would be acceptable to use. And if we are smart enough not to misuse JS_Add*Root, then we can probably be trusted not to misuse this.

But "Let's provide a better front end for JS_Add*Root" wasn't the motivation. All the motivations I, at least, had in opening this bug have been dispelled.

Comment 13

[Jim Blandy :jimb]

(In reply to Eddy Bruel [:ejpbruel] from comment #10)
> Fair enough. However, according to
> https://bugzilla.mozilla.org/show_bug.cgi?id=887077#c17, off thread parses
> creates CompileOptions on the heap as well. How are we going to deal with
> those?

<voice genre="gangster movie, Boston, 1930's">Right, so here's what we gotta do. It's a classic C++ move: make the types reflect the characteristics.

There's gonna be a common base class, CompileOptions, that *consumers* of CompileOptions can use. It has complete implementations for all the members with boring storage characteristics: bools, ints, etc., members and setters, just like now. It also has *abstract virtual* setters and getters for the members with interesting storage characteristics.

This base has two derived classes:

One derived class, AutoCompileOptions, is a stack-only class that uses HandleObject for element, doesn't addref the principals, just holds pointers to the strings, and so on. It implements the base class's setters and getters as appropriate for its representation.

The other derived class, TracedCompileOptions can be used in the heap. It has a 'trace' method. It has HeapPtr<JSObject> members (or whatever). It owns its own copies of the strings. And it also implements the base class's getters and setters. This has a copy constructor that can take an AutoCompileOptions.

All the uses in jsapi.cpp and elsewhere that are creating option sets use AutoCompileOptions. All the uses in off-thread parsing and asm.js recompilation use TracedCompileOptions. And all the consumers --- just the bytecode compiler, right? --- use CompileOptions, just as they do now.

Comment 14

[Jim Blandy :jimb]

... and the objects owning TracedCompileOptions instances would be responsible for calling their 'trace' method at the appropriate times.

Comment 15

[Jim Blandy :jimb]

Comment on attachment 775662 [details] [diff] [review]
Proposed solution

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

Looks all right to me, but I have some larger concerns that I'll post separately.

::: js/public/RootingAPI.h
@@ +1002,5 @@
> +    void remove();
> +
> +    JSContext *cx_;
> +    DynamicRooted *next_;
> +    DynamicRooted *prev_;

We have a really sweet doubly-linked list template (that stores the links in the element, as you're doing here) in mfbt/LinkedList.h. We should just use that instead of giving readers Yet Another Doubly Linked List to read and wonder if it's what they expect (and reviewers an excuse to bikeshed you! :) )

Comment 16

[Jim Blandy :jimb]

I don't know why Splinter is re-posting random reviews...

Comment 17

[Jim Blandy :jimb]

As noted in comment 10, with off-main-thread compilation, the need for non-stack-allocated CompileOptions has appeared again.

Comment 18

[Jim Blandy :jimb]

Attachment: The JSContext accessors GetRuntime, GetContextCompartment, and GetContextZone should be available for inlining by non-friend code.

Comment 19

[Jim Blandy :jimb]

Attachment: Implement AutoRooted<T>, an unrestricted-lifetime rooting type.

This is an alternative to Eddy's patch. While Eddy's patch provides a general rooting base class that anyone can subclass, and that uses virtual dispatch to actually do the tracing, this patch defines a class template, AutoRooted<T>, which is as similar to Rooted<T> as possible, except that it has an arbitrary lifetime.

It's probably not very important, but AutoRooted<T> doesn't incur any virtual function dispatch overhead.

Comment 20

[Jim Blandy :jimb]

Attachment: Implement AutoRooted<T>, an unrestricted-lifetime rooting type.

Now, with more tests! (And a fix for a bug that said tests caught.)

Comment 21

[Jim Blandy :jimb]

Attachment: Implement AutoRooted<T>, an unrestricted-lifetime rooting type.

More tests. Sorry for the noise.

Comment 22

[Jason Orendorff [:jorendorff]]

Comment on attachment 820119 [details] [diff] [review]
The JSContext accessors GetRuntime, GetContextCompartment, and GetContextZone should be available for inlining by non-friend code.

Bouncing review to jonco, who's more familiar with this part of the API. Maybe terrence would be better, I don't really know... anyway, not me :)

Comment 23

[Jon Coppeard (:jonco)]

Comment on attachment 821397 [details] [diff] [review]
Implement AutoRooted<T>, an unrestricted-lifetime rooting type.

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

Yay for testcode!  And the implementation looks fine.  

I have a slight concern with the name - we already have AutoRooters for various things, and these are stack-only roots.  These AutoRooteds seem to be very similar except that they can be used on the heap as well.

Terrence, what do you think?

(We also seem to keep adding new ways of rooting things - maybe we could unify these and the existing AutoRooters in the future)

::: js/public/RootingAPI.h
@@ +1160,5 @@
> +typedef AutoRooted<JSFunction*>                 AutoRootedFunction;
> +typedef AutoRooted<JSScript*>                   AutoRootedScript;
> +typedef AutoRooted<JSString*>                   AutoRootedString;
> +typedef AutoRooted<jsid>                        AutoRootedId;
> +typedef AutoRooted<JS::Value>                   AutoRootedValue;

Do these typedefs need to be here as well as TypeDecls.h?

Comment 24

[Terrence Cole [:terrence]]

Comment on attachment 821397 [details] [diff] [review]
Implement AutoRooted<T>, an unrestricted-lifetime rooting type.

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

We absolutely should not call these Auto. I'm cancelling the review for that reason alone, otherwise this is pretty nice. Currently, Auto *always* means stack lifetime for RAII guards. In fact, we strongly considered calling Rooted AutoRooted.

I'm also a bit concerned about the explosion of rooting mechanisms, but I think that's an inevitable short-term consequence of us completely changing how rooting works in JSAPI. The GC currently exposes a bunch of different interfaces:

1) Whole GC concepts in js/public/GCAPI.h and js/src/jsapi.h.
2) Heap layout minutia for inlining specific things in js/public/HeapAPI.h.
3) Anchors *shudder* in js/public/Anchor.h.
4) Tracing is currently spread among js/src/jsapi.h, js/public/Tracer.h, js/public/RootingAPI.h, and js/src/gc/Barrier.h.
5) StackRooting in js/public/RootingAPI.h.
6) AutoRooters in js/src/jsapi.h.
7) Heap<T> in js/public/RootingAPI.h.
8) JS_AddFooRoot/JS_RemoveFooRoot in js/src/jsapi.h.
9) And now NotAutoRooted in js/public/RootingAPI.h.
And lets not even get started on the internal "structures": a hodge-podge of fields spread out all over Runtime, Context, and Compartment, Zones now intersecting all of them at a weird angle.

I don't know what everyone finds so confusing! I'm sure this has nothing to do with people's perception of SpiderMonkey's GC as a horrible all-consuming monster-gremlin that will eat all of your precious baby objects at whim.

Here's a split that I think makes more sense, although this is quite a bit more conservative with naming than I'd like to be:
1) GCAPI.h: whole GC controls.
2) HeapAPI.h: internal fiddly heap details exposed for inlining.
3) StackRootingAPI.h: Rooted/Handle/MutableHandle
4) TracingAPI.h: Heap/Tracer
5) NotAutoRootAPI.h: The new root type added here, with a sane name.

AddRoot/RemoveRoot should be subsumed by NotAutoRooted. We should also merge the existing AutoRooters into NotAutoRooted. It's a bit weird to change these from Auto to NotAuto, but it is a strict superset of the existing lifetime and performance is not a huge issue since Rooted is handling that niche. Also we definitely need a better name than NotAutoRooted.

Also, a note on the API suffix. Bill did it this way so that you can have js/public/FooAPI.h and js/src/gc/Foo.h open in an editor at the same time without confusion. I agree that this is a pressing use case, however I think we should migrate it in the other direction: i.e. js/public/Foo.h and js/src/gc/FooInternals.h.

I've been putting off this sort of code churn work because everyone just includes jsapi.h anyway, "it's just naming", and don't fix what isn't broken. I think this may have been a mistake. Not only is it confusing where to find things, but we've been incredibly inconsistent with the distinction of (Stack)Rooting vs (Perma)Rooting vs Tracing (i.e. is not rooting, but is marking!); Tracing vs Rooting vs Marking vs IsAboutToBeFinalized; Heap vs. Handle; and probably other things I'm not even aware of. Without even having to touch the documentation, we could do so much better just by moving our code to places that accurately reflect the expected usage.

::: js/src/gc/RootMarking.cpp
@@ +683,5 @@
> +    // Mark the AutoRooted chains of types that are never null.
> +    for (JS::AutoRootedId *r = rt->idAutoRooteds.getFirst(); r != nullptr; r = r->getNext())
> +        MarkIdRoot(trc, r->address(), "AutoRooted<jsid>");
> +    for (JS::AutoRootedValue *r = rt->valueAutoRooteds.getFirst(); r != nullptr; r = r->getNext())
> +        MarkValueRoot(trc, r->address(), "AutoRooted<Value>");

MarkRuntime is already a sweltering hog of a method: could you move this to a submethod? Maybe MarkAutoRootedChains.

Comment 25

[Steve Fink [:sfink] [:s:]]

Amen, brother, and hallelujah!

I totally agree with everything terrence said above.

Naming ideas for AutoRootedFrog:

  RootFrog
  ExternalRootFrog
  PermaRootedFrog
  TracedFrog

I think I prefer these in ascending order -- I like TracedFrog best. I think. It's still a bit messy, because a RootedFrog gets traced too.

Er... or is that the distinction you want to make between tracing and rooting? Argh.

Is having TracedFrog autoconvert to HandleFrog ok? It's easier to make a Handle that outlives its root with TracedFrog than with eg RootedFrog, since you will always be obliterating TracedFrogs at random times unlike the RAII scope of RootedFrog. But I guess it's better than making a .handle() or .toHandle(), since it will be so commonly used. Maybe if we poison TracedFrogs when destroying them, and check for poison in the Handle conversion so the error is close to the source, it'll help?

If we explicitly defined semantics for "mark", "trace", and "root", what would they be?

  mark: visit a node during tracing or root scanning. Includes setting the mark bit and recursing to children, explicitly or via a mark stack.
  mark: visit a node during tracing or root scanning. Includes setting the mark bit. Does not include recursing on children.
  mark: setting the mark bit, only
  mark: ensure a node pointer is updated during moving GC
  trace: traversing the object graph, in general
  trace: recursing to a node's children
  root: ensure an object gets its mark bit set even if nothing else live points to it
  root: ensure an object gets recursively traversed even if nothing else live points to it
  root: ensure a pointer is updated during moving GC

or combinations and intersections of the above. And do you root JSObject, JSObject*, or JSObject**?

Here are the definitions from The Garbage Collection Handbook:

marking: recording that an object is live, often by setting a mark bit
tracing: visiting the reachable objects by traversing all or part of an object graph
root: a reference that is *directly* accessible to the mutator without going through other objects
root object: an object in the heap referred to directly by a root

Comment 26

[Jim Blandy :jimb]

(In reply to Jon Coppeard (:jonco) from comment #23)
> ::: js/public/RootingAPI.h
> @@ +1160,5 @@
> > +typedef AutoRooted<JSFunction*>                 AutoRootedFunction;
> > +typedef AutoRooted<JSScript*>                   AutoRootedScript;
> > +typedef AutoRooted<JSString*>                   AutoRootedString;
> > +typedef AutoRooted<jsid>                        AutoRootedId;
> > +typedef AutoRooted<JS::Value>                   AutoRootedValue;
> 
> Do these typedefs need to be here as well as TypeDecls.h?

Not at all. I was imitating Rooted, which does have its bevy of typedefs in RootingAPI.h; and I accidentally added them in both places. It looks like TypeDecls.h is the best place for these, so I'll take out the copy in RootingAPI.h, and move Rooted's typedefs there as well.

Comment 27

[Jim Blandy :jimb]

(In reply to Terrence Cole [:terrence] from comment #24)
> Comment on attachment 821397 [details] [diff] [review]
> Implement AutoRooted<T>, an unrestricted-lifetime rooting type.
> 
> Review of attachment 821397 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We absolutely should not call these Auto. I'm cancelling the review for that
> reason alone, otherwise this is pretty nice. Currently, Auto *always* means
> stack lifetime for RAII guards. In fact, we strongly considered calling
> Rooted AutoRooted.

In my own defense, I asked about that exact distinction re: the "Auto" prefix in #jsapi, and was told otherwise. But it's good to choose, one way or the other, so I'm happy to change this.

> 3) Anchors *shudder* in js/public/Anchor.h.

Once conservative GC is gone, these should be able to be deleted, right?

> 6) AutoRooters in js/src/jsapi.h.

I both forgot about these and missed these. Our chosen meaning for "Auto" aside, that clash is sufficient to make the name bad.

> MarkRuntime is already a sweltering hog of a method: could you move this to
> a submethod? Maybe MarkAutoRootedChains.

Sure.

Comment 28

[Jim Blandy :jimb]

(In reply to Steve Fink [:sfink] from comment #25)
>   RootFrog

I don't think "Rooted" vs. "Root" is helpful.

>   ExternalRootFrog

"External" to what? The stack?

>   PermaRootedFrog

This is my favorite. It's similar to what Terrence suggested, too.

>   TracedFrog

But all live objects are "traced".

How about NonStackRooted?

> Is having TracedFrog autoconvert to HandleFrog ok? It's easier to make a
> Handle that outlives its root with TracedFrog than with eg RootedFrog, since
> you will always be obliterating TracedFrogs at random times unlike the RAII
> scope of RootedFrog.

Should we just have people use fromMarkedLocation? That would make it easier to search for such issues.

> But I guess it's better than making a .handle() or
> .toHandle(), since it will be so commonly used. Maybe if we poison
> TracedFrogs when destroying them, and check for poison in the Handle
> conversion so the error is close to the source, it'll help?

If we really wanted to kill that kind of bug, we should have Handle adjust reference counts in a HashMap<void *, int> somewhere, and have Rooted and PermaRooted assert that their reference counts are zero.

Comment 29

[Jason Orendorff [:jorendorff]]

I have a string of stupid questions, but maybe they will help actually (one can hope):

js/public/RootingAPI.h says:
> * The Heap<T> class is a C/C++ heap-stored reference to a JS GC thing.

What does this mean? Is a Heap<JSObject*> not a root edge into the GC digraph? It looks like it is. Barriers fire.

Maybe Heap<T> and the new thing could be given contrasting names to help explain both of them? Is the only difference that the new thing is copyable? CopyableHeap<T> then?

Comment 30

[Jim Blandy :jimb]

(In reply to Terrence Cole [:terrence] from comment #24)
> 3) Anchors *shudder* in js/public/Anchor.h.

If anchors make you shudder, then conservative GC should leave you incoherent with terror.

Comment 31

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #29)
> What does this mean? Is a Heap<JSObject*> not a root edge into the GC
> digraph? It looks like it is.

No, it is not. It is just an edge from one object to another.

> Barriers fire.

Yes, but that's irrelevant. Roots are things that are axiomatically alive; barriers are about informing the GC of changes to the graph, and don't inevitably relate to what is alive and what isn't.

We've decided to use snapshot-at-the-beginning semantics, and under such algorithms the barriers do mark the outgoing edge's target as live] - so looking at the implementation, it may indeed seem that barriers are like rooting. But barriers fire only when edges *change*; if nobody touches the object and the next GC cycle begins, the barriers won't fire at all, and won't play any role in their referents' liveness.

> Maybe Heap<T> and the new thing could be given contrasting names to help
> explain both of them? Is the only difference that the new thing is copyable?
> CopyableHeap<T> then?

No; the new thing makes its referent axiomatically alive.

I think we should have StackRooted (nee Rooted) and NonStackRooted (what this patch introduces).

I hereby volunteer to do the rename.

Comment 32

[Jason Orendorff [:jorendorff]]

Well, I did say they were stupid...

Filed bug 931446 as penance.

Comment 33

[Jim Blandy :jimb]

/me looks through jsapi.h again

It looks like Eddy's original patch is identical to CustomAutoRooter.

Comment 34

[Jim Blandy :jimb]

(In reply to Terrence Cole [:terrence] from comment #24)
> We should also merge
> the existing AutoRooters into NotAutoRooted. It's a bit weird to change
> these from Auto to NotAuto, but it is a strict superset of the existing
> lifetime and performance is not a huge issue since Rooted is handling that
> niche. Also we definitely need a better name than NotAutoRooted.

The perf impact is not even going to be that big; it's a singly-linked list vs. a doubly-linked list.

As a separate simplification, a lot of the AutoFooRooters could become typedefs instead of separate classes, because they differ only in the tag value used. If we simply exposed an overloaded Mark function, and used virtual function dispatch instead of the tag switch, then we could eliminate the tags altogether and simplify the code. The overloaded mark function wouldn't even need to be public if we left the virtual function's definition in a .cpp file and used explicit template instantiation.

(I can't volunteer to do this; I'm already feeling really dragged down by the time I've spent on AutoRooted - not because of this discussion! Just the actual implementation of it.)

Comment 35

[Jim Blandy :jimb]

First patch only: https://hg.mozilla.org/integration/mozilla-inbound/rev/0277be165fb3

Comment 36

[Jim Blandy :jimb]

Attachment: Implement PersistentRooted<T>, an unrestricted-lifetime rooting type.

Revised per comments and IRC conversation. New type is now called PersistentRooted.

Comment 37

[Jim Blandy :jimb]

https://tbpl.mozilla.org/?tree=Try&rev=c5e9c16872df

Comment 38

[Terrence Cole [:terrence]]

Comment on attachment 823632 [details] [diff] [review]
Implement PersistentRooted<T>, an unrestricted-lifetime rooting type.

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

Thank you for taking the time to implement this, despite our unreasonable demands for perfection! r=me

::: js/public/RootingAPI.h
@@ +486,5 @@
>  class MOZ_STACK_CLASS MutableHandle : public js::MutableHandleBase<T>
>  {
>    public:
>      inline MutableHandle(Rooted<T> *root);
> +    inline MutableHandle(PersistentRooted<T> *root);

I don't see an implementation of this method?

@@ +577,5 @@
> +     */
> +    template<typename R>
> +    InternalHandle(const JS::PersistentRooted<R> &root, T *field)
> +      : holder((void**)root.address()), offset(uintptr_t(field) - uintptr_t(root.get()))
> +    {}

Actually, lets not provide an InternalHandle constructor for this: it's extra, untested code and that will very likely never get used.

@@ -797,5 @@
> -typedef Rooted<JSFunction*>                 RootedFunction;
> -typedef Rooted<JSScript*>                   RootedScript;
> -typedef Rooted<JSString*>                   RootedString;
> -typedef Rooted<jsid>                        RootedId;
> -typedef Rooted<JS::Value>                   RootedValue;

\o/

@@ +1084,5 @@
> + * Firefox is owned by some JS object or another, so using PersistentRooted in
> + * such objects would introduce leaks. For these kinds of edges, Heap<T> or
> + * TenuredHeap<T> would be better types. It's up to the implementor of the type
> + * containing Heap<T> or TenuredHeap<T> members to make sure their referents get
> + * marked when the object itself is marked.

Great!

::: js/src/jsapi-tests/testPersistentRooted.cpp
@@ +94,5 @@
> +BEGIN_TEST(test_PersistentRootedNull)
> +{
> +    BarkWhenTracedClass::reset();
> +
> +    Kennel kennel(cx);

We'll need to teach the analysis that it is okay to have PersistentRooted on the stack across a GC, please file a follow-up bug and CC myself and Steve.

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0277be165fb3

Comment 40

[Jim Blandy :jimb]

Grr, that try push was against M-C, which didn't have the prerequisite patch (yesterday). Let's try again:

https://tbpl.mozilla.org/?tree=Try&rev=03629c3483a3

Comment 41

[Jim Blandy :jimb]

(In reply to Terrence Cole [:terrence] from comment #38)
> Thank you for taking the time to implement this, despite our unreasonable
> demands for perfection! r=me

I didn't see it that way at all; I just would have liked to keep the conversation more focused. Thanks for pointing out the problems with the names.

> I don't see an implementation of this method?

Added --- thanks.

> Actually, lets not provide an InternalHandle constructor for this: it's
> extra, untested code and that will very likely never get used.

Dropped.

> Great!

I am *very* sympathetic to your concerns about this being misused. People will just cargo-cult it anyway and never see the comment, but we tried. :)

> We'll need to teach the analysis that it is okay to have PersistentRooted on
> the stack across a GC, please file a follow-up bug and CC myself and Steve.

Filed bug 932384.

Cancelled previous try push; here's one with all the revisions:
https://tbpl.mozilla.org/?tree=Try&rev=a535031bd8cf

Comment 42

[Jim Blandy :jimb]

Two minor changes:

- MSVC wasn't able to infer the 'Referent' template argument type for MarkPersistentRootedChain in time to choose the right overload of MarkObjectRoot and MarkStringRoot to take the address of. I reordered the arguments, moving 'list' earlier, so it infers 'Referent' from that first, and it's happy now.

- The conservative GC was finding references to the barker in jsapi-tests/testPersistentRooted.cpp when compiled with optimization on 32-bit Linux; I added a JS_NEVER_INLINE declaration, and now it's fine. :(

A final try push, with those changes:
https://tbpl.mozilla.org/?tree=Try&rev=b0ad9cb4698d

Comment 43

[Terrence Cole [:terrence]]

(In reply to Jim Blandy :jimb from comment #42)
> - The conservative GC was finding references to the barker in
> jsapi-tests/testPersistentRooted.cpp when compiled with optimization on
> 32-bit Linux; I added a JS_NEVER_INLINE declaration, and now it's fine. :(

That is conservative GC for you. The major GC entry points are never-inline for the same reason.

Comment 44

[Jim Blandy :jimb]

In the end I had to disable the test unless exact rooting was in use. I tested with exact rooting enabled, and it passed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/48577a74a5a1

Comment 45

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

https://hg.mozilla.org/mozilla-central/rev/48577a74a5a1

Comment 46

[Terrence Cole [:terrence]]

I think the checked-in solution looks good and seems to work well in practice.