1459577 - GCPolicy defaulting to StructGCPolicy is a footgun

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jan de Mooij [:jandem]]

If I put this in StructGCPolicy:

    static_assert(!mozilla::IsPointer<T>::value, ...);

I get a lot of failures because we instantiate StructGCPolicy for TypedArrayObject* and other types in asserts like this one:

    MOZ_ASSERT(GCPolicy<T>::isValid(*ptr));

StructGCPolicy::isValid always returns true.

We could fix this by adding TypedArrayObject et al to the list in gc/Policy.h, but that doesn't scale and I'm worried about it bloating our code because we use that also to define various out-of-line template functions (I actually noticed this because I was looking into that).

Comment 1

[Jan de Mooij [:jandem]]

I can try something but I'm not great at this template stuff so I may throw it back if I get nowhere :)

Comment 2

[Jan de Mooij [:jandem]]

Having T* default to GCPointerPolicy seems to work in the shell and is an improvement, but not sure if that's what we want..

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #2)
I'm having trouble understanding why e.g. JS::MapTypeToTraceKind<TypedObject*>::kind is TraceKind::Object (correct) but JS::GCPolicy<TypedObject*> derives from StructGCPolicy rather than GCPointerPolicy.

Because of the former we are tracing Rooted things correctly even when the derived GC thing type is not in the list in gc/Policy.h, but we are missing the assertion in GCPointerPolicy::isValid.

I agree that adding everything to the list is not scalable.  I'll try and think how best to make this work.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #3)
Oh, MapTypeToTraceKind<T> gets the trace kind from T::TraceKind if T doesn't match one of the base GC thing types.

Comment 5

[Jon Coppeard (:jonco)]

I think we should introduce a base class for traceable structs and reimplement this in terms of function overloading rather than template specialisation.  That should remove the bloat and the need to list all derived classes explicitly.  I haven't tried this yet though.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #5)
This was harder than I expected.  Function overloading is a pain because we have double pointers here and F(A**) doesn't match for a call to F(B**) where B is derived from A.  I think we may be able to move the template functions to the header and make them inline though, which would reduce the bloat at least.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1459577-gc-policy

This patch keep StructGCPolicy as the default policy and adds an assertion that it's not used with pointer types.

Inside the engine, InternalGCPointerPolicy is used as the default policy for all pointer types (so not using the list of types).  This also adds an assertion that it's only used with derived GC thing pointers.

I had to add an include to gc/Rooting.h to pull in gc/Policy.h for a couple of things to see the internal specialisation, but after that everything worked fine.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8980528 [details] [diff] [review]
bug1459577-gc-policy

Review of attachment 8980528 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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