And after some fairly extensive measurements, our HeapPtr optimizations (e.g. not handling moving), adds about 1B Irefs to a run of Typescript + EarleyBoyer (by far the heaviest allocators). I guess this shouldn't be so surprising: anything that's heavily used goes though WholeCellBuffer, since that's what the JITs target. The remaining code isn't going to have much impact and removing HeapPtr halves the amount of pointer wrapper code we have to deal with. The upshot is a very slight reduction in ICache misses and a tremendous reduction in complexity. A simple |template <typename T> using RelocatablePtr = HeapPtr<T>;| seems to build fine, so I'd like to do the actual conversion of the rest of the engine (about 50 references) in a second patch to ease backout, if this turns out poorly.

Comment on attachment 8622665 [details] [diff] [review] remove_RelocatablePtr-v0.diff Ah hem. At least it passed jit-tests on Friday. Looks like something new in ObjectGroup landed this weekend that's depending on more stuff working with tagged pointers. A moment while I suss out what's going on.

RelocatablePtr did not have an init method, so we were falling through to PreBarriered::init, which does nothing.

Comment on attachment 8623198 [details] [diff] [review] remove_RelocatablePtr-v1.diff Review of attachment 8623198 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: js/public/RootingAPI.h @@ +662,5 @@ > MOZ_ASSERT_IF(v, JS::GCCellPtr(v).kind() == JS::TraceKind::Object); > #elif defined(DEBUG) && defined(EXPORT_JS_API) > CheckTypeHasObjectBase<Derived*>(); > #endif > + return uintptr_t(v) > 32 && gc::IsInsideNursery(reinterpret_cast<gc::Cell*>(v)); How about using IsNullTaggedPointer() here rather than duplicating the magical constant 32? ::: js/src/gc/Barrier.h @@ +418,5 @@ > if (GCMethods<T>::needsPostBarrier(this->value)) > relocate(); > } > > + void init(T v) { While we're here, can we add an assert that this->value == GCMethods<T>::initial() at this point?

(In reply to Jon Coppeard (:jonco) from comment #3) > ::: js/public/RootingAPI.h > @@ +662,5 @@ > > MOZ_ASSERT_IF(v, JS::GCCellPtr(v).kind() == JS::TraceKind::Object); > > #elif defined(DEBUG) && defined(EXPORT_JS_API) > > CheckTypeHasObjectBase<Derived*>(); > > #endif > > + return uintptr_t(v) > 32 && gc::IsInsideNursery(reinterpret_cast<gc::Cell*>(v)); > > How about using IsNullTaggedPointer() here rather than duplicating the > magical constant 32? Yeah, it's pretty ugly, but I think we can get rid of this entire method in a couple patches. At the moment IsNullTaggedPointer is only internal and I'd prefer not to expose it if at all possible. > ::: js/src/gc/Barrier.h > @@ +418,5 @@ > > if (GCMethods<T>::needsPostBarrier(this->value)) > > relocate(); > > } > > > > + void init(T v) { > > While we're here, can we add an assert that this->value == > GCMethods<T>::initial() at this point? I actually did exactly that, but had to remove it! Turns out there are a number of sites that are still mallocing their data, rather than newing it, so the initial value is sometimes the malloc poison. Something to fix, certainly, but that's going to have to come later.

Rebased over new prerequisite bug 1175642.

Backed out for now -- the fix regressed octane on windows, and I don't have time to make a better fix while paying attention to work-week presentations.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/21a80f803b46

I'm fairly certain that this cannot actually happen with our current architecture.