1174873 - Remove HeapPtr and rename RelocatablePtr to HeapPtr

Status: RESOLVED INVALID

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: remove_RelocatablePtr-v0.diff

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 1

[Terrence Cole [:terrence]]

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.

Comment 2

[Terrence Cole [:terrence]]

Attachment: remove_RelocatablePtr-v1.diff

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

Comment 3

[Jon Coppeard (:jonco)]

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?

Comment 4

[Terrence Cole [:terrence]]

(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.

Comment 5

[Terrence Cole [:terrence]]

Attachment: remove_RelocatablePtr-v2.diff

Rebased over new prerequisite bug 1175642.

Comment 6

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc07d991ce93

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0e69237df5b

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0e69237df5b

Comment 9

[Terrence Cole [:terrence]]

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.

Comment 10

[Pulsebot]

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

Comment 11

[Terrence Cole [:terrence]]

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