Objects will not always have an arena header available to look at, so redirect the access to the shape.

This is happening for zones anyway.

Ah, thanks! Fixed the title.

Actually, I think I'm just going to broaden this bug and post all 4 patches here, since they'll probably be easier to review like that.

This adds an isTenured() and calls it for all access to the arenaHeader. At the moment it just returns true, but the intent should be clear. Since access to the JSRuntime has to happen out of the header to avoid a circular dependency, it is added as #ifdef DEBUG to ensure that it is only used by assertions. When creating this series, I noticed that Cell::chunk and Cell::address are only used internally to Cell and subclasses. I made these protected in this patch since I was already touching the Cell definition.

This is the main patch in the series.

Sorry for the delay, I wanted to test GGC on top of the last two patches in the series to make sure they wouldn't need any further tweaking. It turns out we only need a non-tenured version of getAllocKind for JSFunction and JSObject. JSObject::getAllocKind is a fairly complex function, unfortunately, but it seems to pass all tests.

It is important not to call sizeOfThis() on an uninitialized object. This should also make the cached object creation path a bit faster.

With the patch from Bug 843337, neither SS or v8 show a significant perf regressions on shookfoil. I've thrown up a talos run at: https://tbpl.mozilla.org/?tree=Try&rev=82ef8e5a1d63

Comment on attachment 713619 [details] [diff] [review] v6: 2 of 4 - Specialize zone and compartment on JSObject. Review of attachment 713619 [details] [diff] [review]: ----------------------------------------------------------------- Please don't land this until zones lands. The zones patch already adds a compartment() method to each GC thing type and fixes up the Cell uses, so you won't have to make any of the tenuredCompartment changes. ::: js/src/gc/Barrier-inl.h @@ +42,5 @@ > #endif > } > > +/* static */ JS_ALWAYS_INLINE JS::Zone * > +EncapsulatedValue::zone(const Value &value) This is an odd place to put this function, especially since it would be nice to be able to use it outside the barrier code. I would just create a function called ZoneOfValue or something and put it somewhere central. Or else you could try to add it as a method of Value, although that might cause problems. ::: js/src/gc/Marking.cpp @@ +1614,5 @@ > return; > > UnmarkGrayGCThing(thing); > > + JSRuntime *rt = static_cast<Cell *>(thing)->tenuredZone()->rt; Better just to convert this to thing->runtime(). ::: js/src/jswrapper.cpp @@ +1030,5 @@ > Value old = wrapper->getSlot(slot); > if (old.isMarkable()) { > + Zone *zone = old.isObject() > + ? old.toObject().zone() > + : static_cast<Cell *>(old.toGCThing())->tenuredZone(); You could use ZoneOfValue here. ::: js/src/vm/ObjectImpl-inl.h @@ +191,5 @@ > return getSlot(slot); > } > > +static JS_ALWAYS_INLINE JSCompartment * > +ValueCompartment(const js::Value &value) This function doesn't appear to be used. Also, it's incompatible with the zones work (because strings won't have a compartment).

Comment on attachment 715695 [details] [diff] [review] v0: rename getAllocKind to tenuredGetAllocKind Review of attachment 715695 [details] [diff] [review]: ----------------------------------------------------------------- Just waiting for an answer about ObjectImpl::getAllocKind... ::: js/src/jsfun.h @@ +298,5 @@ > bool singleton = false); > > + /* GC support. */ > + js::gc::AllocKind getAllocKind() const { > + if (isExtended()) Can you add an assertion here, like the one above, that if it's tenured then we're returning the right alloc kind? ::: js/src/jsgcinlines.h @@ +56,3 @@ > JS_ASSERT(thing); > const Cell *cell = reinterpret_cast<const Cell *>(thing); > + return MapAllocToTraceKind(cell->tenuredGetAllocKind()); GetGCThingTraceKind is really called only for tenured stuff? It looks like the browser calls it, but I guess those can be fixed up later somehow. Maybe we could check if it's in the range of the nursery. ::: js/src/vm/ObjectImpl-inl.h @@ +19,5 @@ > #include "js/TemplateLib.h" > > #include "ObjectImpl.h" > > +#include "jsgcinlines.h" What's this for? ::: js/src/vm/ObjectImpl.cpp @@ +317,5 @@ > } > } > > +gc::AllocKind > +js::ObjectImpl::getAllocKind() const Who uses this? It seems a little dangerous to be exposing it at so low a level. If it's only needed for the nursery collection, then it would be better to make it a static function in jsgc.cpp. ::: js/src/vm/ObjectImpl.h @@ +1067,5 @@ > "shadow placeholder must match actual elements"); > } > > JSObject * asObjectPtr() { return reinterpret_cast<JSObject *>(this); } > + const JSObject * asConstObjectPtr() const { return reinterpret_cast<const JSObject *>(this); } It's okay to call this asObjectPtr(), I believe. The "const" qualifier should be enough to distinguish them.

(In reply to Bill McCloskey (:billm) from comment #11) > ::: js/src/jsgcinlines.h > @@ +56,3 @@ > > JS_ASSERT(thing); > > const Cell *cell = reinterpret_cast<const Cell *>(thing); > > + return MapAllocToTraceKind(cell->tenuredGetAllocKind()); > > GetGCThingTraceKind is really called only for tenured stuff? It looks like > the browser calls it, but I guess those can be fixed up later somehow. Maybe > we could check if it's in the range of the nursery. This is by far the weakest link of the series. My rough plan is to have the browser pass us the runtime and/or a trace kind so we can do the right thing here. > ::: js/src/vm/ObjectImpl-inl.h > @@ +19,5 @@ > > #include "js/TemplateLib.h" > > > > #include "ObjectImpl.h" > > > > +#include "jsgcinlines.h" > > What's this for? It appears orphaned -- I had tried inlining getAllocKind, but it didn't work out for other reasons. > ::: js/src/vm/ObjectImpl.cpp > @@ +317,5 @@ > > } > > } > > > > +gc::AllocKind > > +js::ObjectImpl::getAllocKind() const > > Who uses this? It seems a little dangerous to be exposing it at so low a > level. If it's only needed for the nursery collection, then it would be > better to make it a static function in jsgc.cpp. I see 5 main classes of user when building without it present: 1) Checking whether to call the finalizer in ::finalize, which Jon and I both have patches out to remove. 2) Basically every object subclass asserts that it got the right thing kind or is looking at the right thing kind in various places. These are nice, but not vital. 3) The NewObjectCache uses getAllocKind on the template -- I actually have a patch I've been meaning to ask you or Brian to review that switches this to using the kind that is cached with the object. 4) jsobj.cpp calls it on |this| all over the place when creating and modifying shapes. 5) JM and Ion call getAllocKind on the template object to select the AllocKind to bake into the inline NewObject paths. 6) The minor collection needs it. 1 - 3 we can kill easily, but I'm not sure how we'd handle 4 & 5.

This patch assumes we're going to keep getAllocKind on ObjectImpl. If you can think of a good solution, I'll re-roll it.

Comment on attachment 719643 [details] [diff] [review] 3 of 4: v1 Review of attachment 719643 [details] [diff] [review]: ----------------------------------------------------------------- OK. Can you add an assertion that ObjectImpl::getAllocKind returns the same as tenuredGetAllocKind as long as the object isn't in the nursery?

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba3337f676f https://hg.mozilla.org/integration/mozilla-inbound/rev/035af5615fb4

https://hg.mozilla.org/mozilla-central/rev/035af5615fb4 https://hg.mozilla.org/mozilla-central/rev/6ba3337f676f

Green try run at: https://tbpl.mozilla.org/?tree=Try&rev=82ef8e5a1d63 Pushed at: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e0347b9a4e4 https://hg.mozilla.org/integration/mozilla-inbound/rev/085337fa9658

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/79f5f64f33b1 for complete scorched-earth failure.

Re-pushing the tenured zone patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/7e658b893b5e Green try run at: https://tbpl.mozilla.org/?tree=Try&rev=75f8b09adb0a

Followup fix for a zone() call I missed in the root analysis build: https://hg.mozilla.org/integration/mozilla-inbound/rev/f447e09d5a0c

https://hg.mozilla.org/mozilla-central/rev/7e658b893b5e https://hg.mozilla.org/mozilla-central/rev/f447e09d5a0c

Comment on attachment 719643 [details] [diff] [review] 3 of 4: v1 Thanks for pushing this!