Closed Bug 1186609 Opened 6 years ago Closed 6 years ago

Implement a TraceableVector for use with Rooted

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Not so bad after having implemented TraceableHashMap.
Attachment #8637431 - Flags: review?(sphink)
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 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.
Attachment #8637431 - Flags: review?(sphink) → review+
(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!
(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!
https://hg.mozilla.org/mozilla-central/rev/f7b0d8d11e31
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.