Closed Bug 1169086 Opened 5 years ago Closed 5 years ago

Make CallbackTracer use virtual dispatch instead of a function pointer

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

I've named the there-is-an-edge-here observer callback "onEdge". I made sure all of the overrides are marked properly as "override", so changing this name should be trivial if there are objections.

Caveat #1: I needed a way to implement hasCallback without using dynamic_cast or trawling through the vtbl. I ended up using |virtual uintptr_t getUID() const { return 0; }|. Users that don't care can ignore it and users that need to provide an IsFooTracer(JSTracer*) for assertions can return the address of a static variable to get a UID without a registry.

Caveat #2: I did not attempt to simplify anything. There are a couple places where the obvious implementation is an inline definition. I kept these defs out-of-line so that the namespace change is easy to review. I plan to move these inline in a followup, where appropriate.
Attachment #8611544 - Flags: review?(jcoppeard)
And the gecko bits.

I plan to make the main callback pass a GCCellPtr soon; for now I've kept the wrapper.
Attachment #8611545 - Flags: review?(continuation)
Comment on attachment 8611545 [details] [diff] [review]
gecko_use_virtual_dispatch_for_callbacktracer-v0.diff

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

Hopefully at some point GCThingCallback can get turned into a regular object, too.

This getuid scheme is a little scary, but jonco is reviewing the part that adds it, so if part 1 is okay with him, then this is okay with me.
Attachment #8611545 - Flags: review?(continuation) → review+
Comment on attachment 8611544 [details] [diff] [review]
sm_use_virtual_dispatch_for_callbacktracer-v0.diff

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

I like the way this simplifies the trace callback functions.

I don't like the getUID() thing though.  How about we have a TracerKind enum for the ones we care about and add a member for this?  getUID() could be replaced with getTracerKind().

::: js/public/TracingAPI.h
@@ +103,5 @@
>      };
>      bool isMarkingTracer() const { return tag_ == TracerKindTag::Marking; }
>      bool isTenuringTracer() const { return tag_ == TracerKindTag::Tenuring; }
>      bool isCallbackTracer() const { return tag_ == TracerKindTag::Callback; }
> +    inline JS::CallbackTracer* asCallbackTracer() const { return asCallbackTracer(); }

Doesn't this just iloop?

@@ +133,5 @@
>          contextName_(nullptr), contextIndex_(InvalidIndex), contextFunctor_(nullptr)
>      {}
>  
> +    // Override this method to receive notification when an edge is visited.
> +    virtual void onEdge(void **thing, JS::TraceKind kind) = 0;

My preference would just be to call this method 'trace'.
Attachment #8611544 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #4)
> Comment on attachment 8611544 [details] [diff] [review]
> sm_use_virtual_dispatch_for_callbacktracer-v0.diff
> 
> Review of attachment 8611544 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the way this simplifies the trace callback functions.
> 
> I don't like the getUID() thing though.  How about we have a TracerKind enum
> for the ones we care about and add a member for this?  getUID() could be
> replaced with getTracerKind().

I wanted to avoid a global registry since it's used outside SpiderMonkey, but the use is niche enough that it probably doesn't make sense to provide it for other embedders.

> ::: js/public/TracingAPI.h
> @@ +103,5 @@
> >      };
> >      bool isMarkingTracer() const { return tag_ == TracerKindTag::Marking; }
> >      bool isTenuringTracer() const { return tag_ == TracerKindTag::Tenuring; }
> >      bool isCallbackTracer() const { return tag_ == TracerKindTag::Callback; }
> > +    inline JS::CallbackTracer* asCallbackTracer() const { return asCallbackTracer(); }
> 
> Doesn't this just iloop?

Gcc and clang are smart enough to call the non-const variant below. MSVC is not, however, so I am going to have to change it.

> @@ +133,5 @@
> >          contextName_(nullptr), contextIndex_(InvalidIndex), contextFunctor_(nullptr)
> >      {}
> >  
> > +    // Override this method to receive notification when an edge is visited.
> > +    virtual void onEdge(void **thing, JS::TraceKind kind) = 0;
> 
> My preference would just be to call this method 'trace'.

I like that the English double-meaning of "on edge" is "tense or nervous", which I feel gives an entirely appropriate sense of foreboding to trace function implementors. |trace| is probably better though as a response to TraceChildren.
(In reply to Terrence Cole [:terrence] from comment #5)
> > Doesn't this just iloop?
> 
> Gcc and clang are smart enough to call the non-const variant below. MSVC is
> not, however, so I am going to have to change it.

Unless I'm missing something |this| is a const pointer so I don't see how that's legal.

> I like that the English double-meaning of "on edge" is "tense or nervous",
> which I feel gives an entirely appropriate sense of foreboding to trace
> function implementors. 

Heh, I missed that pun.  Entirely appropriate, as you say.
You are quite right! I guess clang and gcc are just DCEing it before it gets to that analysis.
This was missing an 'override' annotation (on struct VerifyTraceProtoAndIfaceCacheCalledTracer, method "trace", which overrides a pure virtual method defined elsewhere in the patch).

This causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+).

I pushed a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/334776f16570
https://hg.mozilla.org/mozilla-central/rev/31b85f5bb71c
https://hg.mozilla.org/mozilla-central/rev/334776f16570
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.