Closed Bug 1244365 Opened 8 years ago Closed 8 years ago

Remove JS::Traceable

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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 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+
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.