1107639 - Remove AddFooRoot API in favour of PersistentRootedFoo

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

There are a whole load of functions in jsapi.h to add/remove GC roots which aren't really used much any more.

We should replace use of these functions with the simpler (and safer) PersistentRooted objects, and remove these APIs.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: 1-allow-delayed-init-for-persistent-rooteds

To allow PersistentRooteds to be more easily used as global objects, this patch adds two-phase construction by means of an init() method called after the default constructor.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: 2-give-persistent-rooted-value-operations

We can also give PersistentRooted<Value> value operations to make it easier to use.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: 3-use-delayed-init-for-maybe-persistent-rooteds

Use of Maybe<PersistentRooted<>>> can be replaced with use of two-phase construction.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: 4-replace-nsAutoJSValHolder

Remove nsAutoJSValHolder and replace its use with PersistentRooted.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: 5-remove-roots-before-final-gc

We can remove persistent roots and the contents of the roots hash map before we do the final GC when destroying a runtime.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: 6-use-persistent-rooted-in-jsapi-tests

Replace use of Add/RemoveRoot API with PersistentRooted in jsapi tests.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: 7-use-persistent-rooted-for-JSExceptionState

Replace use of Add/RemoveRoot API with PersistentRooted in JSExceptionState.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: 8-remove-add-remove-root-api

Remove public Add/RemoveRoot API.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8553122 [details] [diff] [review]
4-replace-nsAutoJSValHolder

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

\o/

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 8553113 [details] [diff] [review]
1-allow-delayed-init-for-persistent-rooteds

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

Nice! We've had other requests for this, but sadly I don't remember who.

::: js/public/RootingAPI.h
@@ +1098,5 @@
>   * marked when the object itself is marked.
>   */
>  template<typename T>
>  class PersistentRooted : private mozilla::LinkedListElement<PersistentRooted<T> > {
> +    typedef mozilla::LinkedListElement<PersistentRooted<T> > Base;

>> can be used to close nested templates now, so please use the nicer syntax here and in the line above.

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 8553114 [details] [diff] [review]
2-give-persistent-rooted-value-operations

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

This also solves bug 962176, so please dupe that here when you land.

::: js/public/RootingAPI.h
@@ +1101,5 @@
>   * marked when the object itself is marked.
>   */
>  template<typename T>
> +class PersistentRooted : public js::PersistentRootedBase<T>,
> +                         private mozilla::LinkedListElement<PersistentRooted<T> > {

>> to close and { on newline.

::: js/public/Value.h
@@ +1854,5 @@
> + * Augment the generic PersistentRooted<T> interface when T = Value with type-querying,
> + * value-extracting, and mutating operations.
> + */
> +template <>
> +class PersistentRootedBase<JS::Value> : public MutableValueOperations<JS::PersistentRooted<JS::Value> >

>> to close.

@@ +1856,5 @@
> + */
> +template <>
> +class PersistentRootedBase<JS::Value> : public MutableValueOperations<JS::PersistentRooted<JS::Value> >
> +{
> +    friend class ValueOperations<JS::PersistentRooted<JS::Value> >;

Ditto.

@@ +1861,5 @@
> +    const JS::Value * extract() const {
> +        return static_cast<const JS::PersistentRooted<JS::Value>*>(this)->address();
> +    }
> +
> +    friend class MutableValueOperations<JS::PersistentRooted<JS::Value> >;

Ditto.

Comment 12

[Terrence Cole [:terrence]]

Comment on attachment 8553116 [details] [diff] [review]
3-use-delayed-init-for-maybe-persistent-rooteds

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

Nice!

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 8553122 [details] [diff] [review]
4-replace-nsAutoJSValHolder

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

\o/

Comment 14

[Terrence Cole [:terrence]]

Comment on attachment 8553129 [details] [diff] [review]
7-use-persistent-rooted-for-JSExceptionState

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

Woot!

Comment 15

[Terrence Cole [:terrence]]

Comment on attachment 8553130 [details] [diff] [review]
8-remove-add-remove-root-api

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

::: js/src/gc/GCRuntime.h
@@ +290,5 @@
>      inline bool upcomingZealousGC();
>      inline bool needZealousGC();
>  
> +    bool addRoot(Value *vp, const char *name);
> +    void removeRoot(Value *vp);

The manual rooting of ErrorResult::mJSException is absolutely terrifying: we should definitely make that PersistentRooted too and kill off this last vestige.

Comment 16

[Terrence Cole [:terrence]]

Comment on attachment 8553126 [details] [diff] [review]
5-remove-roots-before-final-gc

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

It looks like a bunch of what's currently in ~JSRuntime would fit in finishRoots. I guess the plan is to gradually move things to finishRoots with the goal of making finishRoots mirror markRoots?

Comment 18

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d39d7e6ac6b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/71097bb1ddcc
https://hg.mozilla.org/integration/mozilla-inbound/rev/852a970ae0d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/de33f69c43fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/b60af7b7317c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7427476ad3d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9dec52a4bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/572c6cb0e79c

Comment 19

[Jon Coppeard (:jonco)]

And fix build error due to missing explicit keyword on single-arg constructor:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b70cf25865d

Comment 20

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #16)

> It looks like a bunch of what's currently in ~JSRuntime would fit in
> finishRoots. I guess the plan is to gradually move things to finishRoots
> with the goal of making finishRoots mirror markRoots?

I hadn't thought of that but it sounds like a good idea.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d39d7e6ac6b8
https://hg.mozilla.org/mozilla-central/rev/71097bb1ddcc
https://hg.mozilla.org/mozilla-central/rev/852a970ae0d2
https://hg.mozilla.org/mozilla-central/rev/de33f69c43fb
https://hg.mozilla.org/mozilla-central/rev/b60af7b7317c
https://hg.mozilla.org/mozilla-central/rev/7427476ad3d4
https://hg.mozilla.org/mozilla-central/rev/0f9dec52a4bd
https://hg.mozilla.org/mozilla-central/rev/572c6cb0e79c
https://hg.mozilla.org/mozilla-central/rev/9b70cf25865d

Comment 22

[Florian Bender]

Have bugs been filed for these: 

(In reply to Terrence Cole [:terrence] from comment #15)
> The manual rooting of ErrorResult::mJSException is absolutely terrifying: we
> should definitely make that PersistentRooted too and kill off this last
> vestige.

(In reply to Jon Coppeard (:jonco) from comment #20)
> > It looks like a bunch of what's currently in ~JSRuntime would fit in
> > finishRoots. I guess the plan is to gradually move things to finishRoots
> > with the goal of making finishRoots mirror markRoots?
> 
> I hadn't thought of that but it sounds like a good idea.

Comment 23

[Jon Coppeard (:jonco)]

Filed bug 1370831 for the former.  The latter doesn't apply any more.