Strongly type the CallbackTracer dispatch function

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks 1 bug)

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments)

This changes |void trace(void**, JS::TraceKind)| to |void onEdge(T**)|. The default implementation of onEdge calls |void onChild(const GCCellPtr&)|, which is =0 in CallbackTracer. This forces an implementation to override onEdge for all layout types, or to implement onChild, which will be called automatically by CallbackTracer's default onEdge implementations. Optionally, it is possible to have onChild with default processing, plus any subset of onEdge calls overridden with extra or different processing.

The Design Space:
=================
    (or why I chose this mechanism and those names)

The obvious, and by far the simplest thing to do would have been to convert |trace| to take a GCCellPtr* and let implementations deal. This would be suboptimimal, however. First, the vast majority of tracers only care about processing the children of a thing and do not care at all about the edge leading to the child. Forcing all tracers to deal with the edgeness of the argument is usually annoying. Secondly, since the underlying storage is not a GCCellPtr, we'd incur a performance penalty to box, unbox, and writeback in all cases.

So, for performance, we need to have the ability to take the raw, unboxed edge pointers, but we also need the common case to have a single boxed override. That means we need to choose the implementation dynamically, which leads us to the second virtual call, but only in the slow case. Making the default of the typed interface call the boxed interface allows us to get the best of both worlds here.

|virtual void trace(const GCCellPtr&)| has a different signature from |virtual void trace(T**)|, so why chose to change the names from trace to onEdge/onChild. Mostly for my sanity. I mean, try reading that first paragraph again, but calling both "trace". It made error message nigh-on inscrutable. I'd be fine with changing one of them back to trace at this point, but I'm going to put my foot down on calling them both "trace". Down that road led madness and changing it back made interpreting error messages straightforward.

I like the onEdge vs onChild naming because it explains what you are looking at when you get the callback. Trace, on the other hand, is a verb. The thing I find confusing about it being a verb is the question of whether it describes what is currently happening or what you should be doing there. In this case it's the first and doing the later will rapidly run you out of stack space.

On the other hand, the interface has been called trace forever and it's still called trace in the similar construct in gecko. I'd be willing to switch onChild back to trace, but I don't think there's any more compelling reason to do so other than the ones I've listed here.
Attachment #8621116 - Flags: review?(jcoppeard)
And the gecko half. I think some of these would make ideal candidates for transitioning over to onEdge, but I didn't want to do too much refactoring in this patch. In particular, please see what I've done with MovingTracer in the prior patch. I think something like that could be useful here too.
Attachment #8621117 - Flags: review?(continuation)
Comment on attachment 8621117 [details] [diff] [review]
gecko_typed_tracer_dispatch-v0.diff

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

::: js/public/TracingAPI.h
@@ -7,5 @@
>  #ifndef js_TracingAPI_h
>  #define js_TracingAPI_h
>  
>  #include "jsalloc.h"
> -#include "jspubtd.h"

Is this intentional?
Attachment #8621117 - Flags: review?(continuation) → review+
Yes. We do not need jspubtd.h in TracingAPI.h. I'm not sure what used to depend on it, but it's not needed now.
Comment on attachment 8621116 [details] [diff] [review]
sm_typed_tracer_dispatch-v0.diff

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

It's great to see this being made into a typed API.

::: js/public/TracingAPI.h
@@ +113,5 @@
> +        onChild(JS::GCCellPtr(*lazyp, JS::TraceKind::LazyScript));
> +    }
> +
> +    // Override this method to receive notification when the child of the
> +    // currently traced thing is visited.

'child of the currently traced thing' sounds a bit clunky - the comment on onEdge() talks about visiting edges, so I think we could just talk about visiting nodes in the GC graph here, or GC things or whatever.

@@ +114,5 @@
> +    }
> +
> +    // Override this method to receive notification when the child of the
> +    // currently traced thing is visited.
> +    virtual void onChild(const JS::GCCellPtr& thing) = 0;

Maybe give this a default implementation that does nothing rather than make clients that override onEdge() override this too?

::: js/src/gc/Tracer.cpp
@@ -50,2 @@
>      JS::AutoTracingName ctx(trc, name);
> -    trc->trace(reinterpret_cast<void**>(thingp), kind);

\o/
Attachment #8621116 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #5)
> Comment on attachment 8621116 [details] [diff] [review]
> sm_typed_tracer_dispatch-v0.diff
> 
> Review of attachment 8621116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's great to see this being made into a typed API.

I believe JS_TraceChildren is the last thing using void* now.

> ::: js/public/TracingAPI.h
> @@ +113,5 @@
> > +        onChild(JS::GCCellPtr(*lazyp, JS::TraceKind::LazyScript));
> > +    }
> > +
> > +    // Override this method to receive notification when the child of the
> > +    // currently traced thing is visited.
> 
> 'child of the currently traced thing' sounds a bit clunky - the comment on
> onEdge() talks about visiting edges, so I think we could just talk about
> visiting nodes in the GC graph here, or GC things or whatever.

Fixed.

> @@ +114,5 @@
> > +    }
> > +
> > +    // Override this method to receive notification when the child of the
> > +    // currently traced thing is visited.
> > +    virtual void onChild(const JS::GCCellPtr& thing) = 0;
> 
> Maybe give this a default implementation that does nothing rather than make
> clients that override onEdge() override this too?

I think silently dropping edges would be the wrong choice. In particular, I'm worried that people who have not been paying attention will use the old "trace" name and be quite confused when their tracer simply does nothing and then have a very hard time figuring out why.

> ::: js/src/gc/Tracer.cpp
> @@ -50,2 @@
> >      JS::AutoTracingName ctx(trc, name);
> > -    trc->trace(reinterpret_cast<void**>(thingp), kind);
> 
> \o/

Try is looking green now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ea102de8fc
(In reply to Terrence Cole [:terrence] from comment #6)
> I think silently dropping edges would be the wrong choice. In particular,
> I'm worried that people who have not been paying attention will use the old
> "trace" name and be quite confused when their tracer simply does nothing and
> then have a very hard time figuring out why.

Yeah, fair enough.
https://hg.mozilla.org/mozilla-central/rev/d498daf4f845
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.