Make JSTracer a real C++ class with a constructor and methods

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: mccr8, Assigned: terrence)

Tracking

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

Reporter

Description

7 years ago
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.
Reporter

Updated

6 years ago
Assignee: general → continuation
Reporter

Comment 2

6 years ago
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.
Reporter

Comment 3

6 years ago
Feedback is welcome.
Assignee

Comment 4

6 years ago
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 :
Attachment #815908 - Flags: feedback+
Reporter

Comment 5

6 years ago
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.
Assignee

Comment 6

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

Comment 7

6 years ago
Attachment #815908 - Attachment is obsolete: true
Reporter

Comment 9

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

Comment 10

6 years ago
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
Attachment #816357 - Flags: feedback+
Assignee

Comment 11

6 years ago
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
Attachment #816358 - Flags: feedback+
(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.
Reporter

Comment 13

6 years ago
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
Assignee

Comment 14

6 years ago
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.
Assignee

Comment 15

5 years ago
Posted patch make_tracer_cpp-js-v0.diff (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=f2262977a112
Assignee: continuation → terrence
Attachment #816356 - Attachment is obsolete: true
Attachment #816357 - Attachment is obsolete: true
Attachment #816358 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8407684 - Flags: review?(jcoppeard)
Assignee

Comment 16

5 years ago
Enough changed here that I didn't end up re-using the original patches. This is very similar, however.
Attachment #8407685 - Flags: review?(continuation)
Reporter

Comment 17

5 years ago
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.
Attachment #8407685 - Flags: review?(continuation) → review+
Assignee

Comment 18

5 years ago
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.
Attachment #8407684 - Attachment is obsolete: true
Attachment #8407684 - Flags: review?(jcoppeard)
Attachment #8407723 - Flags: review?(jcoppeard)
Reporter

Comment 19

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

Comment 20

5 years ago
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.
Assignee

Comment 21

5 years ago
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.
Attachment #8407880 - Flags: review?(jcoppeard)
Assignee

Comment 22

5 years ago
Gah! Different unified build chunking caught me out on a few missing includes.

https://tbpl.mozilla.org/?tree=Try&rev=ecf866e2b0b6
Attachment #8407880 - Attachment is obsolete: true
Attachment #8407880 - Flags: review?(jcoppeard)
Attachment #8407889 - Flags: review?(jcoppeard)
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.
Attachment #8407723 - Flags: review?(jcoppeard) → review+
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.
Attachment #8407889 - Flags: review?(jcoppeard) → review+
Assignee

Comment 25

5 years ago
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!
Assignee

Comment 26

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

Comment 27

5 years ago
And of course the build fails with the /one/ compiler I didn't try with:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c26b2de39a80
Assignee

Comment 28

5 years ago
And the build fails on ancient GCC because of imperfect forwarding of nullptr.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fce80dedac0b
Assignee

Updated

5 years ago
Keywords: leave-open
Assignee

Comment 29

5 years ago
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.
Attachment #8409201 - Flags: review?(jcoppeard)
Comment on attachment 8409201 [details] [diff] [review]
move_infrequent_code_outofline-v0.diff

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

Nice tidyup.
Attachment #8409201 - Flags: review?(jcoppeard) → review+
Assignee

Updated

5 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/84b4cf605262
https://hg.mozilla.org/mozilla-central/rev/098f943ba58d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.