1186609 - Implement a TraceableVector for use with Rooted

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: impl_traceable_vector-v0.diff

Not so bad after having implemented TraceableHashMap.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8637431 [details] [diff] [review]
impl_traceable_vector-v0.diff

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

::: js/public/TraceableVector.h
@@ +15,5 @@
> +
> +template <typename> struct DefaultTracer;
> +
> +// A TraceableVector is a Vector with an additional trace method that knows
> +// how to visi all of the items stored in the Vector. For vectors that contain

visi -> visit

Comment 2

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

Comment on attachment 8637431 [details] [diff] [review]
impl_traceable_vector-v0.diff

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

::: js/public/TraceableVector.h
@@ +26,5 @@
> +// available to handle the custom type.
> +//
> +// Note that although this Vector's trace will deal correctly with moved items,
> +// it does not itself know when to barrier or trace items. To function properly
> +// it must either be used with Rooted, or barriered and traced manually.

I'm just starting to read through this, but that comment makes me wonder if there should be a HeapVector<T>

@@ +49,5 @@
> +    }
> +
> +    void trace(JSTracer* trc) override {
> +        for (size_t i = 0; i < this->length(); ++i)
> +            TraceFunc::trace(trc, &Base::operator[](i), "vector element");

You prefer &Base::operator[](i) to &(*this)[i] ?

@@ +98,5 @@
> +    void clearAndFree() { vec().clearAndFree(); }
> +    template<typename U> bool append(U&& aU) { return vec().append(mozilla::Forward<U>(aU)); }
> +    template<typename... Args> bool emplaceBack(Args&&... aArgs) {
> +        return vec().emplaceBack(mozilla::Forward<Args...>(aArgs...));
> +    }

whoa, there's an emplaceBack?

@@ +129,5 @@
> +};
> +
> +template <typename A, size_t B, typename C, typename D>
> +class RootedBase<TraceableVector<A,B,C,D>>
> +  : public MutableTraceableVectorOperations<JS::Rooted<TraceableVector<A,B,C,D>>, A,B,C,D>

Ok, I don't know how crazy you want to go, but at least in my simplified test program that I wrote to explore all this stuff you're doing, it worked to make this something like

template <typename... TArgs>
class RootedBase<TraceableVector<TArgs...>>
  : public MutableTraceableVectorOperations<JS::Rooted<TraceableVector<TArgs...>>, TArgs...>
{ ... continue to replace <A,B,C,D> with <TArgs...> ... }

::: js/src/jsapi-tests/testGCExactRooting.cpp
@@ +265,5 @@
> +        CHECK(shape);
> +
> +    return true;
> +}
> +END_TEST(testGCRootedVector)

Nice test! Though perhaps you should add a comment somewhere in here

  // Construct a unique property name to ensure that the object creates a new shape.

Comment 3

[Terrence Cole [:terrence]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #1)
> Comment on attachment 8637431 [details] [diff] [review]
> impl_traceable_vector-v0.diff
> 
> Review of attachment 8637431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/TraceableVector.h
> @@ +15,5 @@
> > +
> > +template <typename> struct DefaultTracer;
> > +
> > +// A TraceableVector is a Vector with an additional trace method that knows
> > +// how to visi all of the items stored in the Vector. For vectors that contain
> 
> visi -> visit

vidi vici

Thanks!

Comment 4

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink, :s:] from comment #2)
> Comment on attachment 8637431 [details] [diff] [review]
> impl_traceable_vector-v0.diff
> 
> Review of attachment 8637431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/TraceableVector.h
> @@ +26,5 @@
> > +// available to handle the custom type.
> > +//
> > +// Note that although this Vector's trace will deal correctly with moved items,
> > +// it does not itself know when to barrier or trace items. To function properly
> > +// it must either be used with Rooted, or barriered and traced manually.
> 
> I'm just starting to read through this, but that comment makes me wonder if
> there should be a HeapVector<T>

Oooh. I was thinking you'd just have a TraceableVector<Heap<T>> so that you get barriers on access to the underlying pointers, but maybe we could do a Heap<TraceableVector<T>> that puts the entire thing in the store buffer? In fact, BufferableRef is identical to a DynamicTraceable so we could even make the generic buffer take any DynamicTraceable. Hurm, I'll need to give this some serious thought.

> @@ +49,5 @@
> > +    }
> > +
> > +    void trace(JSTracer* trc) override {
> > +        for (size_t i = 0; i < this->length(); ++i)
> > +            TraceFunc::trace(trc, &Base::operator[](i), "vector element");
> 
> You prefer &Base::operator[](i) to &(*this)[i] ?

I seem to recall that that didn't compile because the operator[] is hidden -- the same thing as needing to use this-> to call things on the base.

> @@ +129,5 @@
> > +};
> > +
> > +template <typename A, size_t B, typename C, typename D>
> > +class RootedBase<TraceableVector<A,B,C,D>>
> > +  : public MutableTraceableVectorOperations<JS::Rooted<TraceableVector<A,B,C,D>>, A,B,C,D>
> 
> Ok, I don't know how crazy you want to go, but at least in my simplified
> test program that I wrote to explore all this stuff you're doing, it worked
> to make this something like
> 
> template <typename... TArgs>
> class RootedBase<TraceableVector<TArgs...>>
>   : public
> MutableTraceableVectorOperations<JS::Rooted<TraceableVector<TArgs...>>,
> TArgs...>
> { ... continue to replace <A,B,C,D> with <TArgs...> ... }

Yeah, that works... on clang and gcc; not so much on msvc unfortunately. Check out bug 1186450 for the full explanation and probably more than you wanted to know about the intersection of template template args and template varargs. template.

> ::: js/src/jsapi-tests/testGCExactRooting.cpp
> @@ +265,5 @@
> > +        CHECK(shape);
> > +
> > +    return true;
> > +}
> > +END_TEST(testGCRootedVector)
> 
> Nice test! Though perhaps you should add a comment somewhere in here
> 
>   // Construct a unique property name to ensure that the object creates a
> new shape.

Done!

Comment 5

[Pulsebot]

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

Comment 6

[Carsten Book [:Tomcat]]

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