I spent some quality time with `man grep` today and figured out exactly how to make the exact rooting measurement script recognize what we want and filter out what we don't want. I used BaseShape as a benchmark, since it started with the smallest number of roots. By the time I had all the edge cases worked out I was able to get us to 0 incorrectly rooted |BaseShape*|'s.

Comment on attachment 679867 [details] [diff] [review] v0 Review of attachment 679867 [details] [diff] [review]: ----------------------------------------------------------------- A couple of questions, but nothing that I think is necessary to change. The Return<> followed by AutoAssertNoGC dance is little funky, but I guess it makes sense. ::: js/src/gc/Marking.cpp @@ +874,2 @@ > do { > + MarkCycleCollectorChildren(trc, shape->base().unsafeGet(), &prevParent); unsafeGet() always makes me think "what's unsafe about it?" and "why is it safe here?" Consider using an AutoAssertNoGC. ::: js/src/gc/Root.h @@ +393,5 @@ > template <typename T> > class Return > { > friend class Rooted<T>; > + typedef void (Return<T>::* ConvertibleToBool)(); I could probably dig this up on the web, but why such a funky type for ConvertibleToBool? Why can't this just be void****** or something like in jscntxtinlines.h? ::: js/src/jsfriendapi.h @@ +289,5 @@ > */ > namespace shadow { > > struct TypeObject { > + RawObject proto; I know RawObject is all we have right now for this, but this seems a little weird. It makes sense from the perspective of measuring progress by counting pointers, but not if you're using RawObject to mean "does not need to be rooted." Unless the plan is to revert this one after we're done? ::: js/src/jsgcinlines.h @@ -502,5 @@ > > inline JSObject * > js_NewGCObject(JSContext *cx, js::gc::AllocKind kind) > { > - AssertCanGC(); Are you thinking this is just unnecessary noise now, or what?

(In reply to Steve Fink [:sfink] from comment #1) > ::: js/src/gc/Marking.cpp > @@ +874,2 @@ > > do { > > + MarkCycleCollectorChildren(trc, shape->base().unsafeGet(), &prevParent); > > unsafeGet() always makes me think "what's unsafe about it?" and "why is it > safe here?" > > Consider using an AutoAssertNoGC. Yeah, that would definitely be clearer. > ::: js/src/gc/Root.h > @@ +393,5 @@ > > template <typename T> > > class Return > > { > > friend class Rooted<T>; > > + typedef void (Return<T>::* ConvertibleToBool)(); > > I could probably dig this up on the web, but why such a funky type for > ConvertibleToBool? Why can't this just be void****** or something like in > jscntxtinlines.h? I just copied what was in HashTable.h. I think the upshot is that it's entirely impossible to define a template that depends on a member function of the templatized class, whereas, the way we're going it's possible someone could theoretically request a Return<JSObject******> and accidentally get the wrong behavior. To be honest, I found the ConvertibleToBool trick far easier to read: the first time I saw it I interpreted the ::* as "Here There Be C++ Dragons, probably related to Boolean." On the other hand, the first time I saw ******* I thought "Gee, I hope I don't run into the use case that required 6 levels of indirection... probably has something to do with TI." > ::: js/src/jsfriendapi.h > @@ +289,5 @@ > > */ > > namespace shadow { > > > > struct TypeObject { > > + RawObject proto; > > I know RawObject is all we have right now for this, but this seems a little > weird. It makes sense from the perspective of measuring progress by counting > pointers, but not if you're using RawObject to mean "does not need to be > rooted." > > Unless the plan is to revert this one after we're done? I don't think we can have RawObject mean anything other than that at this point. If we do need more specialized semantics, I think we are going to have to break out Bare<T>, Naked<T>, etc. > ::: js/src/jsgcinlines.h > @@ -502,5 @@ > > > > inline JSObject * > > js_NewGCObject(JSContext *cx, js::gc::AllocKind kind) > > { > > - AssertCanGC(); > > Are you thinking this is just unnecessary noise now, or what? Yeah, I was overzealous when I added these: to say these functions call an allocator unconditionally is a bit of an understatement. We don't really gain anything by having the assertion happen here rather than 1 statement later.

Pushed at: https://hg.mozilla.org/integration/mozilla-inbound/rev/08fd9aa0d6e1 Green try at: https://tbpl.mozilla.org/?tree=Try&rev=00b022cd2386

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1b0226622e94 - oddly, unaccountably, this turned bug 728834 (yes, I do mean a reftest failure) permaorange on Linux32 debug.

These exact roots got integrated into the Return/Unrooted merge patch.