1188197 - Allow PersistentRooted to store DynamicTraceable

Status: RESOLVED INVALID

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: support_dynamictraceable_in_persistentrooted-v0.diff

Comment 1

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

Comment on attachment 8639631 [details] [diff] [review]
support_dynamictraceable_in_persistentrooted-v0.diff

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

Please update the comment just above DynamicTraceable:

// If a class containing GC pointers has (or can gain) a vtable, then it can be
// trivially used with Rooted/Handle/MutableHandle by deriving from
// DynamicTraceable and overriding |void trace(JSTracer*)|.

::: js/public/RootingAPI.h
@@ +994,5 @@
>                           private mozilla::LinkedListElement<PersistentRooted<T>>
>  {
> +    static_assert(!mozilla::IsConvertible<T, StaticTraceable*>::value &&
> +                  !mozilla::IsConvertible<T, DynamicTraceable*>::value,
> +                  "Rooted takes pointer or Traceable types but not Traceable* type");

Thanks, that's an easy mistake to make.

::: js/src/gc/RootMarking.cpp
@@ +301,2 @@
>  
> +    template <TraceFunction<T> TraceFn = TraceNullableRoot>

Ugh. Now you have TraceFunction, TraceFunc, and TraceFn. And I don't see where TraceFunc is even used. I guess it's handy to expose as part of the class? Would it work as

  using TraceFunc = TraceFunction<T>;

?

How about s/TraceFn/TraceFuncT/? And

  template <TraceFunc TraceFnT = TraceNullableRoot>

maybe?

@@ +327,5 @@
>                                               "PersistentRooted<Value>");
> +
> +    PersistentRootedMarker<ConcreteTraceable>::markChain<MarkDynamicTraceable>(trc,
> +        reinterpret_cast<mozilla::LinkedList<JS::PersistentRooted<ConcreteTraceable>>&>(
> +            roots.heapRoots_[THING_ROOT_DYNAMIC_TRACEABLE]),

Yeah, now I'm thinking root.typedHeapRoots<ConcreteTraceable>(THING_ROOT_DYNAMIC_TRACEABLE) would be worth it. (Is there some way of shrinking that down? Doesn't ConcreteTraceable imply THING_ROOT_DYNAMIC_TRACEABLE?)

::: js/src/jsapi-tests/testGCExactRooting.cpp
@@ +276,5 @@
>      CHECK(CheckMyHashMap(cx, map));
>  
> +    // Unfortunately, the type of get() needs to be non-const ref, so
> +    // we need to explicitly Move the storage.
> +    JS::PersistentRooted<MyHashMap> heapMap(cx, mozilla::Move(map.get()));

I don't really understand the comment (aren't there already both const and non-const get()'s defined? What needs to be a non-const ref, again?) But it doesn't really matter

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54a082b01174

Comment 3

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

Backed out with the other patches in that push for various Spidermonkey and Cpp test failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=12273235&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=12277586&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/35e191320db8

Comment 4

[Terrence Cole [:terrence]]

See bug 1189809.