Remove AddFooRoot API in favour of PersistentRootedFoo

RESOLVED FIXED in mozilla38

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

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.
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)
We can also give PersistentRooted<Value> value operations to make it easier to use.
Attachment #8553114 - Flags: review?(terrence)
Use of Maybe<PersistentRooted<>>> can be replaced with use of two-phase construction.
Attachment #8553116 - Flags: review?(terrence)
Remove nsAutoJSValHolder and replace its use with PersistentRooted.
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)
Replace use of Add/RemoveRoot API with PersistentRooted in jsapi tests.
Attachment #8553128 - Flags: review?(terrence)
Replace use of Add/RemoveRoot API with PersistentRooted in JSExceptionState.
Attachment #8553129 - Flags: review?(terrence)
Remove public Add/RemoveRoot API.
Attachment #8553130 - Flags: review?(terrence)
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+
Duplicate of this bug: 962176
And fix build error due to missing explicit keyword on single-arg constructor:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b70cf25865d
(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.
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)
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.