If we're really C++ing the JS API, one thing that has been on my target list for a while is JSTracer. It has all sorts of C-crustiness, like having to call JS_TracerInit instead of a real constructor, JS_SET_TRACING_DETAILS to set fields on it, etc.

The first patch gives JSTracer a constructor, and switches code over to use that, in the process eliminating JS_TracerInit and TracerInit. The current method is annoying and error prone, and there's the weird thing where sometimes you also have to set the weak map tracer. It also fixes a few classes that, rather than inheriting from JSTracer, have a JSTracer field "base". It makes a number of the tracing methods into private static methods, to avoid forward declarations. I also have some patches to get rid of the weird macros in Tracer.h, as those are ugly and lack type safety, but I want to think a little more about how to distill the essence of the variants that are used. The further patches are less tedious than this one. There are also a bunch of functions like JS_Call*Tracer and JS_GetTraceThingInfo that could be turned into methods, but I think they are fine as is, so I'll probably just leave it alone to avoid code churn. I may move some of the functions to "stringify" a tracer into the JSTracer class, as they are surprisingly spread around, and that might enable making some of these fields private.

Feedback is welcome.

Comment on attachment 815908 [details] [diff] [review] part 1: give JSTracer a constructor (WIP) Review of attachment 815908 [details] [diff] [review]: ----------------------------------------------------------------- f=yesplease,youareawesome ::: js/public/Tracer.h @@ +48,5 @@ > > struct JSTracer { > + JSTracer(JSRuntime *rt, JSTraceCallback callback, WeakMapTraceKind eagerlyTrace = TraceWeakMapValues); > + > + void Reset(JSRuntime *rt, JSTraceCallback callback); This is a really awkward interface, particularly as it forces eagerlyTraceWeakmaps to an fixed value. I think we should call this setCallback and have it only update |callback|: this should be adequate for where it is currently used. ::: js/src/builtin/TestingFunctions.cpp @@ +625,5 @@ > typedef HashSet<void *, PointerHasher<void *, 3>, SystemAllocPolicy> VisitedSet; > > +struct JSCountHeapTracer : public JSTracer { > + JSCountHeapTracer(JSRuntime *rt, JSTraceCallback callback) > + : JSTracer(rt, callback) Two space indent before : @@ +637,3 @@ > > static void > CountHeapNotify(JSTracer *trc, void **thingp, JSGCTraceKind kind) Any reason you did not make this a static member, like the other tracers? ::: js/src/gc/Marking.cpp @@ +1598,4 @@ > } > > static void > UnmarkGrayChildren(JSTracer *trc, void **thingp, JSGCTraceKind kind); You should be able to move UnmarkGrayChildren into UnmarkGrayTracer and avoid this forward declaration. @@ +1613,1 @@ > } {}, as for JSDumpingTracer. @@ +1619,1 @@ > } {} ::: js/src/gc/StoreBuffer.cpp @@ +133,5 @@ > trc->edges->put(thingp); > } > > public: > + AccumulateEdgesTracer(JSRuntime *rt, EdgeSet *edgesArg) Whoa, I totally forgot to remove this when I switched the post barrier verifier to use the real nursery. Thanks for the reminder: filed bug 925863 to track. ::: js/src/gc/Verifier.cpp @@ +391,5 @@ > * This function builds up the heap snapshot by adding edges to the current > * node. > */ > +// static > +void We normally write these as: /* static */ void And the style here is /**/ anyway. ::: js/src/jsapi.cpp @@ +1947,5 @@ > char buffer[200]; > + > +private: > + static void > + DumpNotify(JSTracer *trc, void **thingp, JSGCTraceKind kind); All on one line for in-class declarations. @@ +1951,5 @@ > + DumpNotify(JSTracer *trc, void **thingp, JSGCTraceKind kind); > +}; > + > +// static > +void /* static */ void ::: js/src/jsfriendapi.cpp @@ +764,5 @@ > JS_GetTraceEdgeName(dtrc, buffer, sizeof(buffer))); > } > > +// static > +void /* static */ void ::: js/src/jsgc.cpp @@ +2672,5 @@ > Zone *zone; > JSCompartment *compartment; > +private: > + static void > + CheckCompartmentCallback(JSTracer *trcArg, void **thingp, JSGCTraceKind kind); One line. @@ +2726,5 @@ > return nullptr; > } > > +// static > +void /* static */ void ::: js/xpconnect/src/nsXPConnect.cpp @@ +367,2 @@ > static void > VerifyTraceXPCGlobalCalled(JSTracer *trc, void **thingp, JSGCTraceKind kind) Could we just move this full method inline into the tracer? @@ +373,5 @@ > + > +struct VerifyTraceXPCGlobalCalledTracer : public JSTracer > +{ > + VerifyTraceXPCGlobalCalledTracer(JSRuntime *rt) > + : JSTracer(rt, VerifyTraceXPCGlobalCalled), ok(false) Two space indent for :

Thanks for the comments! Good idea about Reset(). I was pretty unhappy with that, given that it was only used in one place, EndVerifyPreBarriers, but I wasn't sure what was the right way to go. Yeah // static is browser-style, and I figured JS had another one, but I was too lazy to look for my WIP. For some reason, Emacs really wanted to indent before : and I tried to catch where it had done it. I really need to disable the electric indenting.

Yeah, vim also has trouble with those. Sometimes I feel like our indenting rules are "as many as possible" rather than something sane.

I haven't tested these at all yet, but I think they should at least compile.

Comment on attachment 816357 [details] [diff] [review] part 2 - Use a method instead of JS_SET_TRACING_DETAILS Review of attachment 816357 [details] [diff] [review]: ----------------------------------------------------------------- f=me

Comment on attachment 816358 [details] [diff] [review] part 3 - Use a method instead of JS_SET_TRACING_INDEX and JS_SET_TRACING_NAME Review of attachment 816358 [details] [diff] [review]: ----------------------------------------------------------------- f=me

(In reply to Andrew McCreight [:mccr8] from comment #5) > Thanks for the comments! > > Good idea about Reset(). I was pretty unhappy with that, given that it was > only used in one place, EndVerifyPreBarriers, but I wasn't sure what was the > right way to go. > > Yeah // static is browser-style, and I figured JS had another one, but I was > too lazy to look for my WIP. > > For some reason, Emacs really wanted to indent before : and I tried to catch > where it had done it. I really need to disable the electric indenting. I don't know if this'll help if this indentation rule is spidermonkey-specific, but I just added the 'member-init-intro' case to my config to fix this. My current settings are: (c-offsets-alist . ((substatement-open . 0) (case-label . /) (member-init-intro . *) (innamespace . 0) (func-decl-cont . 0) (access-label . /))) I don't really know what all those do, but everything but 'substatement-open' and 'func-decl-cont' look good to me wrt handling SM-specific indentation funkiness.

Looks like the latest version of these patches is causing a bunch of JS test failures, plus a Windows linking failure. The latter should be easy to fix once I look up the right linking macro incantation, but I'm not sure about the former. One of the tests I looked at was for a patch that involved implementing some trace stuff, so maybe I need to fix up another tracer in some way that didn't show up as a compilation error. https://tbpl.mozilla.org/?tree=Try&rev=fca3d41476ef

The test failures are segfaults in the shell, so those should also be trivial to track down. It's probably just an assertion that is running at slightly the wrong time now.

Enough changed here that I didn't end up re-using the original patches. This is very similar, however.

Comment on attachment 8407685 [details] [diff] [review] make_tracer_cpp-browser-v0.diff Review of attachment 8407685 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this! ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +395,2 @@ > if (MOZ_UNLIKELY(tracer->mCb.WantDebugInfo())) { > // based on DumpNotify in jsapi.c While you are here, please replace "jsapi.c" with "jsapi.cpp". I suppose it is thematically related to updating the code to make it more C++-like.

Forgot to attach the new Tracer.h to the patch. Try was not happy. New run up at... well I'll post it when the push finishes.

Do you need to change the structs that inherit from JSTracer to classes now that JSTracer is a class?

https://tbpl.mozilla.org/?tree=Try&rev=877281172f3b (In reply to Andrew McCreight [:mccr8] from comment #19) > Do you need to change the structs that inherit from JSTracer to classes now > that JSTracer is a class? Well, clang and gcc didn't complain, so I didn't think to take that step. Previous try push got cancelled by a power flicker, so lets see what the new one above says.

This just moves GCMarker to gc/Tracer.h. Will follow up with cleanup once I figure out how to cleanly separate IS_GC_MARKING_TRACER and callback==NULL.

Gah! Different unified build chunking caught me out on a few missing includes. https://tbpl.mozilla.org/?tree=Try&rev=ecf866e2b0b6

Comment on attachment 8407723 [details] [diff] [review] make_tracer_cpp-js-v1.diff Review of attachment 8407723 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, and it feels good to get rid of all those macros! ::: js/public/TracingAPI.h @@ +108,5 @@ > + // Return the weak map tracing behavior set on this tracer. > + WeakMapTraceKind eagerlyTraceWeakMaps() const { return eagerlyTraceWeakMaps_; } > + > + // Update the trace callback. > + void setTraceCallback(JSTraceCallback traceCallback); Improvement: can we make this protected? It doesn't feel like it should be part of the public interface. ::: js/src/builtin/TestingFunctions.cpp @@ -682,5 @@ > }; > > typedef HashSet<void *, PointerHasher<void *, 3>, SystemAllocPolicy> VisitedSet; > > -typedef struct JSCountHeapTracer { We could make this class derive from JSTracer now too. ::: js/src/gc/RootMarking.cpp @@ +468,5 @@ > case OBJOBJHASHMAP: { > AutoObjectObjectHashMap::HashMapImpl &map = static_cast<AutoObjectObjectHashMap *>(this)->map; > for (AutoObjectObjectHashMap::Enum e(map); !e.empty(); e.popFront()) { > MarkObjectRoot(trc, &e.front().value(), "AutoObjectObjectHashMap value"); > + trc->setTracingLocation((void *)&e.front().key()); Is the void* cast necessary? ::: js/src/gc/StoreBuffer.h @@ +60,5 @@ > Key prior = key; > typename Map::Ptr p = map->lookup(key); > if (!p) > return; > + trc->setTracingLocation((void*)&*p); I think we can remove the cast here too. ::: js/src/gc/Tracer.cpp @@ +241,5 @@ > > +JSTracer::JSTracer(JSRuntime *rt, JSTraceCallback traceCallback, > + WeakMapTraceKind weakTraceKind /* = TraceWeakMapValues */) > + : callback(traceCallback) > + , runtime_(rt) I don't think the ", intializer" format is part of our coding style. ::: js/src/jsapi.cpp @@ +1636,5 @@ > }; > > typedef HashSet<void *, PointerHasher<void *, 3>, SystemAllocPolicy> VisitedSet; > > +class DumpingTracer Can we make this derive JSTracer? ::: js/src/jsinfer.cpp @@ +3793,5 @@ > {} > > void mark(JSTracer *trc) { > JSObject *prior = proto; > + trc->setTracingLocation((void *)&*prior); Remove cast.

Comment on attachment 8407889 [details] [diff] [review] move_gcmarker_to_tracer-v1.diff Review of attachment 8407889 [details] [diff] [review]: ----------------------------------------------------------------- Great! One thing to be aware of though is that the changes in bug 995657 touched the GCMarker code that this moves, so you may need to manually reapply those changes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf31dec6639a Managed to trip a couple ugly MSVC bugs and dll linkage needed JS_PUBLIC_API, but I finally got a green try run from windows: https://tbpl.mozilla.org/?tree=Try&rev=744d34af4040 (In reply to Jon Coppeard (:jonco) from comment #23) > Comment on attachment 8407723 [details] [diff] [review] > make_tracer_cpp-js-v1.diff > ::: js/public/TracingAPI.h > @@ +108,5 @@ > > + // Return the weak map tracing behavior set on this tracer. > > + WeakMapTraceKind eagerlyTraceWeakMaps() const { return eagerlyTraceWeakMaps_; } > > + > > + // Update the trace callback. > > + void setTraceCallback(JSTraceCallback traceCallback); > > Improvement: can we make this protected? It doesn't feel like it should be > part of the public interface. Not easily. Maybe in a followup. > ::: js/src/builtin/TestingFunctions.cpp > @@ -682,5 @@ > > }; > > > > typedef HashSet<void *, PointerHasher<void *, 3>, SystemAllocPolicy> VisitedSet; > > > > -typedef struct JSCountHeapTracer { > > We could make this class derive from JSTracer now too. Followup. The patch series is long and complicated enough that it's bounced on try 3 times now. > ::: js/src/gc/RootMarking.cpp > @@ +468,5 @@ > > case OBJOBJHASHMAP: { > > AutoObjectObjectHashMap::HashMapImpl &map = static_cast<AutoObjectObjectHashMap *>(this)->map; > > for (AutoObjectObjectHashMap::Enum e(map); !e.empty(); e.popFront()) { > > MarkObjectRoot(trc, &e.front().value(), "AutoObjectObjectHashMap value"); > > + trc->setTracingLocation((void *)&e.front().key()); > > Is the void* cast necessary? gc/RootMarking.cpp:472:37: error: cannot initialize a parameter of type 'void *' with an rvalue of type 'JSObject *const *' > ::: js/src/gc/StoreBuffer.h > @@ +60,5 @@ > > Key prior = key; > > typename Map::Ptr p = map->lookup(key); > > if (!p) > > return; > > + trc->setTracingLocation((void*)&*p); > > I think we can remove the cast here too. Huh, this one is indeed unnecessary. > ::: js/src/gc/Tracer.cpp > @@ +241,5 @@ > > > > +JSTracer::JSTracer(JSRuntime *rt, JSTraceCallback traceCallback, > > + WeakMapTraceKind weakTraceKind /* = TraceWeakMapValues */) > > + : callback(traceCallback) > > + , runtime_(rt) > > I don't think the ", intializer" format is part of our coding style. It is when we have to use an #ifdef block around the last field in the struct. There's no other elegant way to get the comma on the second-to-last field to be conditionally present. I'm pretty sure I've seen at least one other class use this initializer form for this reason. > ::: js/src/jsapi.cpp > @@ +1636,5 @@ > > }; > > > > typedef HashSet<void *, PointerHasher<void *, 3>, SystemAllocPolicy> VisitedSet; > > > > +class DumpingTracer > > Can we make this derive JSTracer? Follow-up. > ::: js/src/jsinfer.cpp > @@ +3793,5 @@ > > {} > > > > void mark(JSTracer *trc) { > > JSObject *prior = proto; > > + trc->setTracingLocation((void *)&*prior); > > Remove cast. Nice!

And a green try run on linux, just incase: https://tbpl.mozilla.org/?tree=Try&rev=5a10e755377a

And of course the build fails with the /one/ compiler I didn't try with: https://hg.mozilla.org/integration/mozilla-inbound/rev/c26b2de39a80

And the build fails on ancient GCC because of imperfect forwarding of nullptr. https://hg.mozilla.org/integration/mozilla-inbound/rev/fce80dedac0b

This moves all the stuff in MarkStack/GCMarker that doesn't need to be inline out-of-line, removes the totally-unused templating from MarkStack, and hides the symbols that are only used internally, where possible.

https://hg.mozilla.org/mozilla-central/rev/cf31dec6639a https://hg.mozilla.org/mozilla-central/rev/c26b2de39a80 https://hg.mozilla.org/mozilla-central/rev/fce80dedac0b

Comment on attachment 8409201 [details] [diff] [review] move_infrequent_code_outofline-v0.diff Review of attachment 8409201 [details] [diff] [review]: ----------------------------------------------------------------- Nice tidyup.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/84b4cf605262 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/098f943ba58d https://tbpl.mozilla.org/?tree=Try&rev=09b4fe6211ac

https://hg.mozilla.org/mozilla-central/rev/84b4cf605262 https://hg.mozilla.org/mozilla-central/rev/098f943ba58d