Remove AddFooRoot API in favour of PersistentRootedFoo

RESOLVED FIXED in mozilla38

Status

()

Core
JavaScript: GC
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla38
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8553113 [details] [diff] [review]
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.
Assignee: nobody → jcoppeard
Attachment #8553113 - Flags: review?(terrence)
(Assignee)

Comment 2

3 years ago
Created attachment 8553114 [details] [diff] [review]
2-give-persistent-rooted-value-operations

We can also give PersistentRooted<Value> value operations to make it easier to use.
Attachment #8553114 - Flags: review?(terrence)
(Assignee)

Comment 3

3 years ago
Created attachment 8553116 [details] [diff] [review]
3-use-delayed-init-for-maybe-persistent-rooteds

Use of Maybe<PersistentRooted<>>> can be replaced with use of two-phase construction.
Attachment #8553116 - Flags: review?(terrence)
(Assignee)

Comment 4

3 years ago
Created attachment 8553122 [details] [diff] [review]
4-replace-nsAutoJSValHolder

Remove nsAutoJSValHolder and replace its use with PersistentRooted.
(Assignee)

Comment 5

3 years ago
Created attachment 8553126 [details] [diff] [review]
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.
Attachment #8553126 - Flags: review?(terrence)
(Assignee)

Comment 6

3 years ago
Created attachment 8553128 [details] [diff] [review]
6-use-persistent-rooted-in-jsapi-tests

Replace use of Add/RemoveRoot API with PersistentRooted in jsapi tests.
Attachment #8553128 - Flags: review?(terrence)
(Assignee)

Comment 7

3 years ago
Created attachment 8553129 [details] [diff] [review]
7-use-persistent-rooted-for-JSExceptionState

Replace use of Add/RemoveRoot API with PersistentRooted in JSExceptionState.
Attachment #8553129 - Flags: review?(terrence)
(Assignee)

Comment 8

3 years ago
Created attachment 8553130 [details] [diff] [review]
8-remove-add-remove-root-api

Remove public Add/RemoveRoot API.
Attachment #8553130 - Flags: review?(terrence)
(Assignee)

Updated

3 years ago
Attachment #8553122 - Flags: review?(bent.mozilla)
Comment on attachment 8553122 [details] [diff] [review]
4-replace-nsAutoJSValHolder

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

\o/
Attachment #8553122 - Flags: review?(bent.mozilla) → review+
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.
Attachment #8553113 - Flags: review?(terrence) → review+
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.
Attachment #8553114 - Flags: review?(terrence) → review+
Comment on attachment 8553116 [details] [diff] [review]
3-use-delayed-init-for-maybe-persistent-rooteds

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

Nice!
Attachment #8553116 - Flags: review?(terrence) → review+
Comment on attachment 8553122 [details] [diff] [review]
4-replace-nsAutoJSValHolder

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

\o/
Attachment #8553128 - Flags: review?(terrence) → review+
Attachment #8553129 - Attachment is patch: true
Comment on attachment 8553129 [details] [diff] [review]
7-use-persistent-rooted-for-JSExceptionState

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

Woot!
Attachment #8553129 - Flags: review?(terrence) → review+
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.
Attachment #8553130 - Flags: review?(terrence) → review+
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?
Attachment #8553126 - Flags: review?(terrence) → review+
(Assignee)

Updated

3 years ago
Duplicate of this bug: 962176
(Assignee)

Comment 18

3 years ago
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
(Assignee)

Comment 19

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

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

Comment 20

3 years ago
(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.
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Comment 22

3 years ago
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.
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 23

6 months ago
Filed bug 1370831 for the former.  The latter doesn't apply any more.
Flags: needinfo?(jcoppeard)
You need to log in before you can comment on or make changes to this bug.