Closed
Bug 1107639
Opened 10 years ago
Closed 9 years ago
Remove AddFooRoot API in favour of PersistentRootedFoo
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(8 files)
8.96 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
4.10 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
11.60 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
9.67 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
2.79 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
13.85 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
2.31 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
15.49 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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•9 years ago
|
||
We can also give PersistentRooted<Value> value operations to make it easier to use.
Attachment #8553114 -
Flags: review?(terrence)
Assignee | ||
Comment 3•9 years ago
|
||
Use of Maybe<PersistentRooted<>>> can be replaced with use of two-phase construction.
Attachment #8553116 -
Flags: review?(terrence)
Assignee | ||
Comment 4•9 years ago
|
||
Remove nsAutoJSValHolder and replace its use with PersistentRooted.
Assignee | ||
Comment 5•9 years ago
|
||
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•9 years ago
|
||
Replace use of Add/RemoveRoot API with PersistentRooted in jsapi tests.
Attachment #8553128 -
Flags: review?(terrence)
Assignee | ||
Comment 7•9 years ago
|
||
Replace use of Add/RemoveRoot API with PersistentRooted in JSExceptionState.
Attachment #8553129 -
Flags: review?(terrence)
Assignee | ||
Comment 8•9 years ago
|
||
Remove public Add/RemoveRoot API.
Attachment #8553130 -
Flags: review?(terrence)
Assignee | ||
Updated•9 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 10•9 years ago
|
||
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 11•9 years ago
|
||
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 12•9 years ago
|
||
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 13•9 years ago
|
||
Comment on attachment 8553122 [details] [diff] [review] 4-replace-nsAutoJSValHolder Review of attachment 8553122 [details] [diff] [review]: ----------------------------------------------------------------- \o/
Updated•9 years ago
|
Attachment #8553128 -
Flags: review?(terrence) → review+
Updated•9 years ago
|
Attachment #8553129 -
Attachment is patch: true
Comment 14•9 years ago
|
||
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 15•9 years ago
|
||
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 16•9 years ago
|
||
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 | ||
Comment 18•9 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•9 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•9 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.
Comment 21•9 years ago
|
||
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
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 22•9 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•7 years 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.
Description
•