The new exact GC rooting has no replacement for AutoArrayRooter and friends. This should be relatively easy to add and more efficient as well. Unfortunately, until we actually turn on exact rooting, we cannot replace these for anything that isn't rooted in some other way. For AutoArrayRooters that are rooting stack stored arrays, this should be fine, but I have no idea how many others there are. It would probably not be wise to add a RootedArray<T> to the API that doesn't actually root. For the moment, RootedArray<T> could create an AutoArrayRooter under the covers until after we turn on exact rooting. It would probably be best to just wait until after exact rooting is available.

Comment on attachment 681631 [details] [diff] [review] Implement RootedVector<Value> Thinking about it, this won't work with the conservative GC. Cancelling review.

That was pretty much my top comment in my in-progress review. :-) I think we /must/ make the RootedArray make its own storage. Further, RootedVector is going to start with inline storage (if we use js/Vector.h). I think it would be swell if we could combine these two facts and only provide a RootedVector. We'll have to tie into the existing rooting mechanisms (and add another SkipRoot) when we transition to out-of-line storage, but it would make things so much nicer to only have a single canonical way to root all linear collections.

Is there any reason we can't continue to use the existing AutoRooters for arrays?

(In reply to Terrence Cole [:terrence] from comment #5) > I think we /must/ make the RootedArray make its own storage. Why? Without relying on features that I didn't think we could use yet, the loss of initializer lists results in clumsier syntax. > Further, RootedVector is going to start with inline storage (if we use js/Vector.h). Yes, and it should be able to. > I think it would be swell if we could combine these two facts and only > provide a RootedVector. Hm. I see the appeal. I made an attempt at a RootedArrayN earlier, but ran into a lot of problems. > We'll have to tie into the existing rooting > mechanisms (and add another SkipRoot) when we transition to out-of-line > storage, Why a SkipRoot? If it's on the heap, it won't be poisoned by the rootanalysis. We just need to trace it for both exact and conservative.

(In reply to Bill McCloskey (:billm) from comment #6) > Is there any reason we can't continue to use the existing AutoRooters for > arrays? They don't do exact rooting. We use a SkipRoot to prevent that from messing up the analysis. But yes, there's no fundamental difference between AutoArrayRooter and RootedArray. I could have just as well added exact rooting to AutoArrayRooter. I don't like the old names, though, and would prefer to rename at the same time.

(In reply to Steve Fink [:sfink] from comment #8) > (In reply to Bill McCloskey (:billm) from comment #6) > > Is there any reason we can't continue to use the existing AutoRooters for > > arrays? > > They don't do exact rooting. We use a SkipRoot to prevent that from messing > up the analysis. I'm not sure what you mean. We mark everything they find in MarkRuntime, and they're compatible with moving collection AFAIK. The SkipRoot is just necessary to ensure that the rooting analysis doesn't wrongly poison their values. > But yes, there's no fundamental difference between AutoArrayRooter and > RootedArray. I could have just as well added exact rooting to > AutoArrayRooter. I don't like the old names, though, and would prefer to > rename at the same time. The problem with renaming them is that they have different semantics than Rooted<T>. A Rooted<T> is expected to hold the storage for the thing being rooted, while AutoArrayRooter is passed the thing being rooted in the constructor. Using Brian's old naming convention, this should properly be called a RootArray<T>, I guess.

Comment on attachment 681610 [details] [diff] [review] Implement RootedArray<Value> Unflagging review, since we've decided not to go this route yet.

(In reply to Bill McCloskey (:billm) from comment #9) > (In reply to Steve Fink [:sfink] from comment #8) > > (In reply to Bill McCloskey (:billm) from comment #6) > > > Is there any reason we can't continue to use the existing AutoRooters for > > > arrays? > > > > They don't do exact rooting. We use a SkipRoot to prevent that from messing > > up the analysis. > > I'm not sure what you mean. We mark everything they find in MarkRuntime, and > they're compatible with moving collection AFAIK. The SkipRoot is just > necessary to ensure that the rooting analysis doesn't wrongly poison their > values. Oh. Sadly, I did not realize that. We really need to comment our SkipRoots. I've been assuming they're all essentially "TODO" markers, other than a funky one in jstypedarray.cpp that I commented. Why was AutoArrayRooter necessary in the conservative scanning world? > > But yes, there's no fundamental difference between AutoArrayRooter and > > RootedArray. I could have just as well added exact rooting to > > AutoArrayRooter. I don't like the old names, though, and would prefer to > > rename at the same time. > > The problem with renaming them is that they have different semantics than > Rooted<T>. A Rooted<T> is expected to hold the storage for the thing being > rooted, while AutoArrayRooter is passed the thing being rooted in the > constructor. Using Brian's old naming convention, this should properly be > called a RootArray<T>, I guess. ...and I didn't realize that either. I mean, I know all current Rooted<T> contain the pointer being rooted, but I didn't realize it was a fundamental property. I thought it was just that nobody had a need for rooting anything external to a Rooted<> yet. (For single values, it's hard to come up with an example where that would be safe enough to be useful; you'd need to go through the Rooted<> to access it anyway.) Hm, ok. So, I still see a couple of existing problems that I'd like to solve: 1. I really don't like the pile of names. Quick, what do each of these do: AutoValueRooter RootedValue AutoValueArrayRooter AutoValueVector AutoValueArray AutoGCRooter<Value> AutoVectorRooter<Value> jsvalRoot Ok, 2 of those aren't totally fair: one doesn't actually exist, but shows up in the comments right where you'd look to pick the right thing; and one is a necessary base class (though maybe it should be hidden somewhere other than jsapi.h, since it sounds too plausible.) I'd at least like to get this down to something like RootedValue RootedVector ArrayRoot<Value> The name of the 3rd is arguable, and as terrence points out, it would be nice to eliminate it entirely and only use RootedVector. 2. The implementation of the rooting seems unnecessarily different. It would be nice to use thingGCRooters for everything with stack lifetime (including heap allocations with stack lifetime, like today's AutoValueVector.) We'd still need to do custom stuff for non-exact rooting. I was thinking of having thingGCRooters on unconditionally, but only use the few necessary ones when both exact rooting and root analysis are off. What do you guys think?

I agree, but given the complexity I think we should probably just live with the bad names and awkward differences for awhile longer. In a couple of months we'll have gotten the truly necessary grunt work out of the way and we can revisit this.

Six months on and my thinking on this has changed a bit. We have two basically identical methods, but there is a difference: marking performance. Since there are only a handful of Rooted, the switch is tiny and since they all have a single inline slot, the case contents are trivial and similarly sized. AutoRooter, on the other hand, have tons of cases and complex marking in each. Since there are tons of Rooted on the stack at any time but few AutoRooters, this current split actually makes total sense. What makes a little less sense is the lack of a Handle equivalent for AutoRooter. However, since the storage is out-of-line generally, I think it is okay to use const& for these, as long as we can make the analysis aware of the pattern. AutoValueVector - gold standard for arrays of Values RootedValue - gold standard for single Value AutoValueRooter - gone AutoValueArrayRooter - not familiar, assuming gone AutoValueArray - replace with AutoValueVector<N> in AutoGCRooter<Value> - should be hidden in namespace detail AutoVectorRooter<Value> - should be hidden in a namespace detail jsvalRoot - jsd1? not really a concern The work for most of the above is now going on in Bug 965830, so I'm duping this over to it.