1181799 - Allow Rooted to root any non-virtual struct with a compatible trace method

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: rooted_of_arbitrary_static_types-v1.diff

With bug 1181279, we have the ability to use Rooted with any class with a virtual trace method. Unfortunately, the virtual adds a full extra word to everything we might want to use it with. In many cases this is fine, but for a large number of SpiderMonkey containers, we expect the container to live normally on the Heap. Adding an extra word to, for example, Bindings or TypeSet::Type would add an extra word to JSScript or double the typical overhead for ObjectGroup. This is totally unacceptable.

However, with a bit of care, it turns out we can have our cake and eat it too. Instead of storing an extra word in every single instance, Steve observed that we only need to be able to do dynamic dispatch when the thing is on the stack. Specifically, if we can store the "dynamic" bits needed to dispatch to the right tracing inline in the list somehow, we only have to eat the dynamic overhead when the thing is on the stack.

And this actually works pretty well. The main complexity is in how we get the Rooted implementation to only store the extra word when create Rooted<StaticTraceable>. We can easily use IsBaseOf and Conditional to swap out the type, but this doesn't work with the storage class, so we can't just drop in a thing with 0-size. Moreover, once storage classes are out of the picture, there is no zero-sized type (for fairly esoteric C reasons). So we basically have to end up conditionally wrapping T, rather than extending the class. This, however, isn't so bad, as C++ gives us the tools to easily pass through a member transparently.

Even with a new, non-pointer wrapper type dropped in for |ptr|, no additional changes were required to Rooted, Handle, or MutableHandle. The dynamic dispatch is accomplished by storing the address of the trace function. Since we don't know the type at call time, this has to be static. Boilerplate in the user becomes switching this-> to self-> in trace, or adding a wrapper function to dispatch to self->trace().

Comment 1

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8631278 [details] [diff] [review]
rooted_of_arbitrary_static_types-v1.diff

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

This turned out way better than I had expected. Nice going!

::: js/public/RootingAPI.h
@@ +648,5 @@
> +{
> +    static_assert(mozilla::IsBaseOf<JS::StaticTraceable, T>::value,
> +                  "DispatchWrapper is intended only for usage with a StaticTraceable");
> +
> +    using TraceFn = void (*)(T*, JSTracer*);

Oh, wow, I've never used |using| for that. I've always done

  typedef void (*TraceFn)(T*, JSTracer*);

but even if that doesn't work for some obscure reason, I think I like the |using| version better.

@@ +653,5 @@
> +    TraceFn tracer;
> +    T storage;
> +
> +  public:
> +    // Mimic a pointer types, so that we can drop into Rooted.

*type

@@ +664,5 @@
> +    // Trace the contained storage (of unknown type) using the trace function
> +    // we set aside when we did know the type.
> +    static void TraceWrapped(JSTracer* trc, JS::StaticTraceable* thingp, const char* name) {
> +        auto wrapper = reinterpret_cast<DispatchWrapper*>(uintptr_t(thingp) - sizeof(TraceFn));
> +        wrapper->tracer(&wrapper->storage, trc);

This is awesome!

But could it be done with this?:

  auto wrapper = reinterpret_cast<DispatchWrapper*>(uintptr_t(thingp) - offsetof(DispatchWrapper, storage));

Gotta be ready for those architectures with 16-bit function pointers and 32-bit field alignment, y'know. :-)

@@ +795,5 @@
> +    using MaybeWrapped = typename mozilla::Conditional<
> +        mozilla::IsBaseOf<StaticTraceable, T>::value,
> +        js::DispatchWrapper<T>,
> +        T>::Type;
> +    MaybeWrapped ptr;

That's a mouthful, but surprisingly enough it actually makes sense. You probably ought to update the comment to exclude your wrapped monstrosities, though, since casting them to Rooted<void*> would NOT work.

Comment 2

[Terrence Cole [:terrence]]

Oh, wow, that comment is referring to the /dynamic/ rooting analysis. I'll just replace it with something relevant then.

Comment 3

[Terrence Cole [:terrence]]

Ironically, Steve, you were quite correct about the alignment issue. On 32bit, we still sometimes require CellSize (8 byte) alignment, so I had to add padding.

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

Comment 4

[Terrence Cole [:terrence]]

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

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55b4ef462fde

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/55b4ef462fde