Closed
Bug 1244365
Opened 8 years ago
Closed 8 years ago
Remove JS::Traceable
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: terrence, Assigned: terrence)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
35.83 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
With the ability to map T to a RootKind, plus the existence of GCPolicy<T>, we now we longer need JS::Traceable to detect and trace traceable structures. Now that PersistentRooted and Rooted are sharing most mechanisms, swapping over to auto-detection of traceable structures becomes as easy a changing a couple types and deleting JS::Traceable. In more detail: creating Rooted<NotAPointer>, uses MapTypeToTraceKind<NotAPointer>::kind, which yields the default, JS::RootKind::Traceable, which we match on to select DispatchWrapper storage instead of POD storage, which stores GCPolicy<NotAPointer>::trace in front of NotAPointer, which, if not overridden, will produce StructGCPolicy<NotAPointer>::trace, i.e. NotAPointer::trace. Even though the above is a long and complicated chain of logic, each individual step is a simple and well-supported GC API mechanism. The consumer needs no weird templates, no awkward base mixin, no editing enums in a different part of the tree. You can simply put any class with a trace method in a Rooted and the right thing happens. I want to stress this point: our new GC primitives, when combined, make the simplest usage the right usage. They did not accidentally delete barriers, or delete your hash keys, as has happened in the past. I wasn't even aiming to remove Traceable, it just kinda sloughed off when it became redundant. I feel like we're moving in the right direction here. Now that I've expressed optimism, I expect this try run to be completely red: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1935aebeaf23
Attachment #8713905 -
Flags: review?(sphink)
Comment 1•8 years ago
|
||
Comment on attachment 8713905 [details] [diff] [review] 2.3_remove_Traceable-v0.diff Review of attachment 8713905 [details] [diff] [review]: ----------------------------------------------------------------- This cleaned up really well! ::: js/src/gc/RootMarking.cpp @@ +49,5 @@ > +// the type field. We use the following type as a compatible stand-in. No > +// actual methods from ConcreteTraceable type are actually used at runtime -- > +// the real trace function has been stored inline in the DispatchWrapper. > +struct ConcreteTraceable { > + ConcreteTraceable() { MOZ_CRASH("instantiation of ConcreteTraceable"); } Do you need to have a constructor at all? Would this work if it were deleted?
Attachment #8713905 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 2•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f322e2cc221c68c128c30a5211b7c883c835013 Bug 1244365 - Remove Traceable; r=sfink
Comment 3•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1f322e2cc221
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•