1232417 - Use a strongly typed Variant as the key for the CrossCompartmentWrapperMap

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 1_variant_CCW_Key-v0.diff

Right now this is a tagged Cell* with optional JSObject* debugger: ideal for conversion to a mozilla::Variant. The core problem is that to get any benefit from the static types here, we need to convert all of our existing switch(tag) { cast; use; break; } logic into Variant::match. This is generally straightforward, but quite verbose and difficult to grok, given the lack of builtin tuple syntax.

Instead, what I've done is wrapped the underlying Variant's match in an abstraction that matches over the wrapped, or the debugger so that we don't have to deal with the tuple directly. The upside is significantly less and easier to read code, but there is a downside as well. Specifically, since C++ support neither template lambdas nor even function local structs with template members, we have to move the actual action above the function into an anonymous namespace. This results in a small code size increase over the type-unsafe version: 8 files changed, 310 insertions(+), 237 deletions(-).

I still think this is worth taking: in particular, I was able to remove a handful of assertions that guard against states that are simply impossible to express at all now.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 8698145 [details] [diff] [review]
1_variant_CCW_Key-v0.diff

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

::: js/src/gc/Tracer.cpp
@@ +241,5 @@
> +                    void operator()(JSScript** tp) {
> +                        if (!zones_.has((*tp)->zone()))
> +                            return;
> +                        TraceManuallyBarrieredEdge(trc_, tp, "cross-compartment script");
> +                    }

The duplication here is annoying, but we need to special case String* somehow and this is the pattern we're already using to do that.

::: js/src/jscompartment.cpp
@@ +231,5 @@
> +        TraceFunctor(JSTracer *trc, const char* name) : trc_(trc), name_(name) {}
> +
> +        using ReturnType = void;
> +        template <class T> void operator()(T* t) { TraceManuallyBarrieredEdge(trc_, t, name_); }
> +    };

This should all go away when we move to stable hashing.

@@ +369,5 @@
>      }
>  
>      /* Check the cache. */
>      RootedValue key(cx, StringValue(str));
> +    if (WrapperMap::Ptr p = crossCompartmentWrappers.lookup(CrossCompartmentKey::fromValue(key))) {

Fixed this ugliness. See the note on the declaration.

@@ +565,5 @@
>      MOZ_ASSERT(!zone()->isCollecting() || trc->runtime()->gc.isHeapCompacting());
>  
>      for (WrapperMap::Enum e(crossCompartmentWrappers); !e.empty(); e.popFront()) {
>          Value v = e.front().value().unbarrieredGet();
> +        if (e.front().key().is<JSObject*>()) {

This syntax at least is an improvement.

@@ +762,5 @@
>  
> +namespace {
> +struct NeedsSweepUnbarrieredFunctor {
> +    using ReturnType = bool;
> +    template <class T> bool operator()(T* t) const { return IsAboutToBeFinalizedUnbarriered(t); }

I wasn't able to figure out any great way to share this with the similar functor in Marking.cpp. The protocol is just ever so slightly different. I might take a swing at reifying this and exposing a functor for all of our marking functions at some point.

@@ +778,5 @@
>      /* Remove dead wrappers from the table. */
>      for (WrapperMap::Enum e(crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +        CrossCompartmentKey copy(e.front().key());
> +        bool keyDying = copy.applyToWrapped(NeedsSweepUnbarrieredFunctor()) ||
> +                        copy.applyToDebugger(NeedsSweepUnbarrieredFunctor());

Luckily, not all cases are bad -- this one gets quite a bit cleaner.

::: js/src/jscompartment.h
@@ +73,5 @@
> +
> +    explicit CrossCompartmentKey(JSObject* obj) : wrapped(obj) { MOZ_RELEASE_ASSERT(obj); }
> +    explicit CrossCompartmentKey(JSString* str) : wrapped(str) { MOZ_RELEASE_ASSERT(str); }
> +    explicit CrossCompartmentKey(NativeObject* debugger, JSObject* obj, DebuggerObjectKind kind)
> +        : wrapped(DebuggerAndObject(debugger, obj, kind))

Whoops! Fixed the spacing locally.

@@ +79,5 @@
> +        MOZ_RELEASE_ASSERT(debugger);
> +        MOZ_RELEASE_ASSERT(obj);
> +    }
> +    explicit CrossCompartmentKey(NativeObject* debugger, JSScript* script)
> +        : wrapped(DebuggerAndScript(debugger, script))

And here.

@@ +85,5 @@
> +        MOZ_RELEASE_ASSERT(debugger);
> +        MOZ_RELEASE_ASSERT(script);
> +    }
> +
> +    static CrossCompartmentKey fromValue(const Value& v) {

Oh, heh, I was thinking that we couldn't construct this directly because Variant has no default and we need to call different constructors somehow. Actually we can just construct WrappedType for /each/ and copy into wrapped. Will upload a new patch.

@@ +123,5 @@
> +            ReturnType match(const DebuggerAndScript& tpl) const { return f_(mozilla::Get<1>(tpl)); }
> +            ReturnType match(const DebuggerAndObject& tpl) const { return f_(mozilla::Get<1>(tpl)); }
> +        } matcher(f);
> +        return wrapped.match(matcher);
> +    }

Unfortunately, I had to have both a const and non-const version of each of the apply functions. We need a non-const version for tracers and other places where we update the pointer. Without a const_version, we'd need to const_cast in a bunch of places. It also simplifies most callees significantly to avoid the double indirection. This is a bunch of almost-duplication, but it seemed better to reduce the complexity in the callers and the callees.

@@ +167,5 @@
> +        };
> +        return applyToWrapped(GetCompartmentFunctor());
> +    }
> +
> +    struct Hasher

Hurm. Deriving from DefaultHasher<CrossCompartmentKey> gives us Lookup and rekey, which I guess turned out to be identical. Guess I'll pull those back in to save some lines.

@@ +180,5 @@
> +            }
> +        };
> +        static HashNumber hash(const CrossCompartmentKey& key) {
> +            static_assert(sizeof(HashNumber) == sizeof(uint32_t),
> +                          "subsequent code assumes a four-byte hash");

I've no idea at all where or why since this has always just passed through to DefaultHasher.

@@ -90,5 @@
> -    {
> -        MOZ_RELEASE_ASSERT(wrappedArg.isString() || wrappedArg.isObject());
> -        MOZ_RELEASE_ASSERT(wrapped);
> -    }
> -    explicit CrossCompartmentKey(const RootedValue& wrappedArg)

I was quite happy to find that everything still built when I deleted this constructor.

@@ -98,5 @@
> -    {
> -        MOZ_RELEASE_ASSERT(wrappedArg.isString() || wrappedArg.isObject());
> -        MOZ_RELEASE_ASSERT(wrapped);
> -    }
> -    CrossCompartmentKey(Kind kind, JSObject* dbg, js::gc::Cell* wrapped)

Verified that this sharp edge was actually only called from the intended 3 places in the debugger.

@@ +590,5 @@
>  
>      bool putWrapper(JSContext* cx, const js::CrossCompartmentKey& wrapped, const js::Value& wrapper);
>  
>      js::WrapperMap::Ptr lookupWrapper(const js::Value& wrapped) const {
> +        return crossCompartmentWrappers.lookup(js::CrossCompartmentKey::fromValue(wrapped));

Fixed locally.

::: js/src/jscompartmentinlines.h
@@ +104,5 @@
>      JS::RootedObject cacheResult(cx);
>  #endif
>      JS::RootedValue v(cx, vp);
> +    auto key = js::CrossCompartmentKey::fromValue(v);
> +    if (js::WrapperMap::Ptr p = crossCompartmentWrappers.lookup(key)) {

This changeset is gone.

::: js/src/jsfriendapi.cpp
@@ +601,5 @@
> +    void operator()(T t) const {
> +        if (t->isTenured() && t->asTenured().isMarked(gc::GRAY))
> +            callback_(closure_, JS::GCCellPtr(t));
> +    }
> +};

It's annoying to have to copy the data all out-of-line here. Maybe we can express all callbacks as a functor and just specialize this one?

Comment 2

[Terrence Cole [:terrence]]

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

Comment 3

[Terrence Cole [:terrence]]

Attachment: 1_variant_CCW_Key-v1.diff

Rebased and applied my self-review feedback. Jon, please see the comments on the previous patch for notes on difficulties and design decisions.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8698172 [details] [diff] [review]
1_variant_CCW_Key-v1.diff

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

I think this is a good idea, but I don't like how much extra code we have to add for all the matchers and functors.  I wonder if we can improve that.  I'm going to cancel review for now and we can talk about it at the meeting later.

Also, it didn't build for me.  I got lots of errors related to equality of tuples, like this:

../../dist/include/mozilla/Variant.h:174:36: error: invalid operands to binary expression ('const mozilla::Tuple<js::NativeObject *, JSScript *>' and 'const mozilla::Tuple<js::NativeObject *, JSScript *>')
      return aLhs.template as<T>() == aRhs.template as<T>();
             ~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~

::: js/src/jscompartment.cpp
@@ -264,5 @@
> -        CrossCompartmentKey key = e.front().key();
> -        CheckGCThingAfterMovingGC(key.debugger);
> -        CheckGCThingAfterMovingGC(key.wrapped);
> -        CheckGCThingAfterMovingGC(
> -                static_cast<Cell*>(e.front().value().unbarrieredGet().toGCThing()));

The value check seems to have got lost.

@@ -785,5 @@
> -                  reinterpret_cast<JSScript**>(&key.wrapped));
> -              break;
> -          default:
> -              MOZ_CRASH("Unknown key kind");
> -        }

Yay for fewer reinterpret_casts!

::: js/src/jscompartment.h
@@ +97,5 @@
> +    template <typename T> bool is() const { return wrapped.is<T>(); }
> +    template <typename T> const T& as() const { return wrapped.as<T>(); }
> +
> +    template <typename F>
> +    auto applyToWrapped(F f) -> decltype(f(static_cast<JSObject**>(nullptr))) {

Can we say F::ReturnType here instead of decltype(...)?

@@ +173,5 @@
> +            using ReturnType = HashNumber;
> +            template <class T> HashNumber operator()(T t) const { return DefaultHasher<T>::hash(t); }
> +        };
> +        static HashNumber hash(const CrossCompartmentKey& key) {
> +            return key.applyToWrapped(HashFunctor());

This isn't quite the same as the original, which included key.kind.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #4)
> Also, it didn't build for me.  I got lots of errors related to equality of
> tuples

Oh right, I need the patch from bug 1232418 too.

Comment 6

[Terrence Cole [:terrence]]

Attachment: 1_variant_CCW_Key-v2.diff

This is a big improvement over the previous version. We're down to 58 extra lines. Would be less if I had been able to land bug 1232686 first, but that's not an option yet, given the uncertainty around vs2013 still. Also, it turns out we need to compile with C++14 to get generic lambdas, even once compiler support is universal, so that's also not something we can block on, unfortunately.

Comment 7

[Terrence Cole [:terrence]]

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

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 8752952 [details] [diff] [review]
1_variant_CCW_Key-v2.diff

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

Looks good, except for the issue with debuggers and zone groups below.

::: js/src/gc/Tracer.cpp
@@ +137,5 @@
> +    template <typename T>
> +    ReturnType operator()(T tp) {
> +        if (!compartments_.has((*tp)->compartment()))
> +            return;
> +        TraceManuallyBarrieredEdge(trc_, tp, "cross-compartment object");

The name should be "cross-compartment wrapper" not "cross-compartment object"

::: js/src/jscompartment.cpp
@@ +293,5 @@
>      }
>  
> +    if (const_cast<CrossCompartmentKey&>(wrapped).applyToWrapped(IsInsideNurseryFunctor()) ||
> +        const_cast<CrossCompartmentKey&>(wrapped).applyToDebugger(IsInsideNurseryFunctor()))
> +    {

Const cast is a bit awkward, but whatever :)

::: js/src/jscompartment.h
@@ +153,5 @@
> +
> +    struct Hasher : public DefaultHasher<CrossCompartmentKey>
> +    {
> +         struct HashFunctor {
> +             using ReturnType = HashNumber;

Indentation of previous two lines is off.

::: js/src/jsfriendapi.cpp
@@ +599,5 @@
>  
> +namespace JS {
> +template <> struct MapTypeToTraceKind<js::NativeObject> {
> +    static const JS::TraceKind kind = JS::TraceKind::Object;
> +};

What is this for?

::: js/src/jsgc.cpp
@@ +4772,5 @@
>  JSCompartment::findOutgoingEdges(ComponentFinder<JS::Zone>& finder)
>  {
>      for (js::WrapperMap::Enum e(crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +        MOZ_ASSERT(!e.front().key().is<JSString*>());
> +        e.front().mutableKey().applyToWrapped(AddOutgoingEdgeFunctor(finder));

This is not the same as the original.  For debugger wrappers we always add an edge regardless of whether the wrapped thing is marked, to ensure debugger and debuggee objects are swept in the same group.  This is a simplification to make sure sweeping works correctly when the debugger is in use.  We could probably improve this but that should be done in a separate bug.

@@ +4943,5 @@
> +                AssertNotOnGrayList(&e.front().value().unbarrieredGet().toObject());
> +        }
> +    }
> +#endif
> +}

Thanks for factoring this out.

Comment 9

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #8)
> ::: js/src/jscompartment.cpp
> @@ +293,5 @@
> >      }
> >  
> > +    if (const_cast<CrossCompartmentKey&>(wrapped).applyToWrapped(IsInsideNurseryFunctor()) ||
> > +        const_cast<CrossCompartmentKey&>(wrapped).applyToDebugger(IsInsideNurseryFunctor()))
> > +    {
> 
> Const cast is a bit awkward, but whatever :)

These two lines are the one and only reason that I had the second const version of applyTo on the CCKey. I guess this is probably clearer than making the CCKey 40 lines longer and having a second ever-so-slightly incompatible version.

> ::: js/src/jsfriendapi.cpp
> @@ +599,5 @@
> >  
> > +namespace JS {
> > +template <> struct MapTypeToTraceKind<js::NativeObject> {
> > +    static const JS::TraceKind kind = JS::TraceKind::Object;
> > +};
> 
> What is this for?

Ah, that's orphaned now. An earlier version of the patch used NativeObject* to disambiguate a normal object wrappers from a debugger object wrappers (which are always NativeObject). This was a terrible idea for several reasons and it's been gone since the very earliest iterations. Must have missed this remnant. 

> ::: js/src/jsgc.cpp
> @@ +4772,5 @@
> >  JSCompartment::findOutgoingEdges(ComponentFinder<JS::Zone>& finder)
> >  {
> >      for (js::WrapperMap::Enum e(crossCompartmentWrappers); !e.empty(); e.popFront()) {
> > +        MOZ_ASSERT(!e.front().key().is<JSString*>());
> > +        e.front().mutableKey().applyToWrapped(AddOutgoingEdgeFunctor(finder));
> 
> This is not the same as the original.  For debugger wrappers we always add
> an edge regardless of whether the wrapped thing is marked, to ensure
> debugger and debuggee objects are swept in the same group.  This is a
> simplification to make sure sweeping works correctly when the debugger is in
> use.  We could probably improve this but that should be done in a separate
> bug.

Oh, wow, that's really subtle. Great catch! I think we can do something like:
bool needsEdge = key.is<JSObject*>() ? (!isMarked(BLACK) || isMarked(GRAY) : true;
And pass the result into the functor.

> @@ +4943,5 @@
> > +                AssertNotOnGrayList(&e.front().value().unbarrieredGet().toObject());
> > +        }
> > +    }
> > +#endif
> > +}
> 
> Thanks for factoring this out.

:-)

Comment 10

[Terrence Cole [:terrence]]

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

Comment 11

[Terrence Cole [:terrence]]

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

Comment 12

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fcaad4111c43ef3bbab08f66f5c3511a2af67a5
Bug 1232417 - Use a Variant to represent the CrossCompartmentWrapperMap key; r=jonco

Comment 13

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3eb07c249c423d3ff2ee258665be51480a404be
Bug 1232417 - Followup to fix unexpected S bustage on a CLOSED TREE; r=meow

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/0fcaad4111c4
https://hg.mozilla.org/mozilla-central/rev/a3eb07c249c4