Last Comment Bug 721463 - GC: minimize jsgcmark's interface
: GC: minimize jsgcmark's interface
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Terrence Cole [:terrence]
:
:
Mentors:
Depends on: 727980
Blocks: 712022 720522 721557 726115 726687 726845 727135 727281
  Show dependency treegraph
 
Reported: 2012-01-26 11:19 PST by Terrence Cole [:terrence]
Modified: 2012-02-16 13:33 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Mostly mechanical. (62.46 KB, patch)
2012-01-26 11:19 PST, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v1: updated based on informal feedback (62.05 KB, patch)
2012-01-31 16:17 PST, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v2: With review feedback. (60.47 KB, patch)
2012-02-09 12:00 PST, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review
v3: Re-added opt null check. (60.61 KB, patch)
2012-02-10 10:01 PST, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-01-26 11:19:31 PST
Created attachment 591870 [details] [diff] [review]
Mostly mechanical.

Currently jsgcmark.h exposes way more surface area than we actually use in practice.

We can easily drop the 2-pointer version of Mark.*Range: the only places it is used, it is not the more natural of the two interfaces.

We should also compact the duplicated, strongly-typed wrappers of Mark<>.  There are several non-important differences in these functions (different assertions, or assertion order, etc), that muddy the waters unnecessarily.  There are also differences in which set of operations we expose for each type, also confusing what is going on here.  As an added bonus, if we auto-generate these, we can drop the Markable interface and take HeapPtr<T> directly, tightening the type-checking further.

We can make all but one of the MarkChildren interfaces (5 of 7 of which are currently exposed) internal.

Finally, IsMarked has a JSObject and a Cell version, even though JSObject is always convertible to Cell.

The diffstat:
 13 files changed, 432 insertions(+), 671 deletions(-)
Comment 1 Terrence Cole [:terrence] 2012-01-26 11:21:40 PST
https://tbpl.mozilla.org/?tree=Try&rev=a0d04538602d
Comment 2 Terrence Cole [:terrence] 2012-01-31 16:17:24 PST
Created attachment 593263 [details] [diff] [review]
v1: updated based on informal feedback

Removed the detail namespace that ended up not getting exposed in the header.
Comment 3 Bill McCloskey (:billm) 2012-02-08 18:16:48 PST
Comment on attachment 593263 [details] [diff] [review]
v1: updated based on informal feedback

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

Thanks, this is a nice cleanup!

::: js/src/jsatom.cpp
@@ +385,5 @@
>      JSAtomState *state = &rt->atomState;
>  
>      if (rt->gcKeepAtoms) {
>          for (AtomSet::Range r = state->atoms.all(); !r.empty(); r.popFront()) {
> +            MarkStringRoot(trc, r.front().asPtr(), "locked_atom");

Could you remove the unnecessary braces around this statement while you're here?

::: js/src/jsgc.cpp
@@ +1905,2 @@
>          if (fp->isEvalFrame()) {
> +            MarkScriptRoot(trc, fp->script(), "eval script");

Can you remove the braces around this statement?

::: js/src/jsgcmark.cpp
@@ +52,5 @@
> +/*
> + * PushMarkStack is forward-declared so that it can be called by Mark, and
> + * and the Markers are declared above PushMarkStack so that the we can mark
> + * recursively.
> + */

I don't think we need this comment. It's a bit confusing. Most of the PushMarkStack functions could just have been moved above Mark. I just wanted to put them below because Mark is the most important function.

@@ +74,5 @@
>  
>  static inline void
>  PushMarkStack(GCMarker *gcmarker, types::TypeObject *thing);
>  
> +/* ********** Object Marking ********** */

Please change this to /*** Object marking ***/ so that it resembles the jsapi.h convention. Same for the other separators here.

@@ +130,5 @@
> +}
> +
> +template <typename T>
> +static void
> +Mark(JSTracer *trc, const HeapPtr<T> &thing, const char *name)

I don't think any of these intermediate Mark or MarkUnbarriered functions are needed. You can might as well just do everything in the macro definitions. In the few places where they're called later, it seems like you can just use Mark<Type>Unbarriered. The same goes for MarkRoot, MarkRange, and MarkRootRange below.

@@ +166,5 @@
> +#define DeclMarkerImpl(base, type)                                                                \
> +void                                                                                              \
> +Mark##base##Unbarriered(JSTracer *trc, type *thing, const char *name)                             \
> +{                                                                                                 \
> +    MarkUnbarriered<type>(trc, thing, name);                                              \

Please fix the backslash spacing here and below.

@@ +170,5 @@
> +    MarkUnbarriered<type>(trc, thing, name);                                              \
> +}                                                                                                 \
> +                                                                                                  \
> +void                                                                                              \
> +Mark##base(JSTracer *trc, const HeapPtr<type> &thing, const char *name)                           \

This one should go before Mark##base##Unbarriered now that it doesn't depend on it.

@@ +253,5 @@
> +
> +/* ********** ID Marking ********** */
> +
> +static inline void
> +MarkIdInternal(JSTracer *trc, const jsid &id) {

The brace should always go in a separate line.

::: js/src/jsgcmark.h
@@ +36,5 @@
> + * The cycle collector internally duplicates most of the unexposed logic in
> + * PushMarkStack so that it can special case and ignore edge cases it doesn't
> + * care about.  It uses JS_TraceChildren to iterate children, but has some
> + * important special interfaces that make it a separate path.  See the comment
> + * on MarkCycleCollectorChildren.

To be honest, I found this comment pretty confusing. Comments in a header file should be pretty high-level, describing how stuff is used, rather than how it works. Could you take this out?

@@ +43,4 @@
>  namespace js {
>  namespace gc {
>  
> +/* ********** Object Marking ********** */

Please fix the separators in this file as well.

@@ +52,5 @@
> + * If there was only a  need to track the various GC kinds, we would not need
> + * this.  The problem is that we want to pass a HeapPtr<Foo> to a convertable
> + * Bar type named MarkBar.  As an explicit example, consider that we want to
> + * be able to pass all of JSLinearString, JSFlatString, JSAtom, etc to a
> + * single MarkString function.

I think we need a comment here that describes what functions are actually being exposed and how they should be used. Something like this:

These functions expose marking function for all the different GC things. For each GC thing, there are several variants. As an example, these are the variants generated for JSObject. They are listed from most to least desirable for use:

    MarkObject(JSTracer *trc, const HeapPtr<JSObject> &thing, const char *name);
        This function should be used for marking JSObjects, in preference to all others below. Use it when you have HeapPtr<JSObject>, which automatically implements write barriers.

    MarkObjectRoot(JSTracer *trc, JSObject *thing, const char *name);
        This function is only valid during the root marking phase of GC (i.e., when MarkRuntime is on the stack).

    MarkObjectUnbarriered(JSTracer *trc, JSObject *thing, const char *name);
        Like MarkObject, this function can be called at any time. It is more forgiving, since it doesn't demand a HeapPtr as an argument. Its use should always be accompanied by a comment explaining how write barriers are implemented for the given field.

Additionally, the functions MarkObjectRange and MarkObjectRootRange are defined for marking arrays of object pointers.

@@ +56,5 @@
> + * single MarkString function.
> + */
> +#define DeclMarker(base, type)                                                                    \
> +void Mark##base##Unbarriered(JSTracer *trc, type *thing, const char *name);                       \
> +void Mark##base(JSTracer *trc, const HeapPtr<type> &thing, const char *name);                     \

Please put Mark##base first, since it's the main one.

@@ +78,5 @@
> +DeclMarker(XML, JSXML)
> +#endif
> +
> +inline bool
> +IsMarked(JSContext *cx, Cell *cell)

Please group the Mark and IsMarked function together in a separate section.

@@ +87,5 @@
> +/* ********** Externally Typed Marking ********** */
> +
> +/*
> + * Note: this must only be called by the GC and only when we are tracing through
> + * MarkRoots.  It is explicitly for ConservativeStackMarking and should go away

One space after the period.

@@ -202,5 @@
> - * the type of the object. The static type is used at compile time to link to
> - * the corresponding Mark/IsMarked function.
> - */
> -inline void
> -Mark(JSTracer *trc, const js::HeapValue &v, const char *name)

These Mark functions are used by IonMonkey, so we shouldn't remove them.

@@ +137,3 @@
>  
> +/* TypeNewObject contains a HeapPtr<const Shape> that needs a unique cast. */
> +void MarkShape(JSTracer *trc, HeapPtr<const Shape> &thing, const char *name);

void should go on a separate line.

@@ +138,5 @@
> +/* TypeNewObject contains a HeapPtr<const Shape> that needs a unique cast. */
> +void MarkShape(JSTracer *trc, HeapPtr<const Shape> &thing, const char *name);
> +
> +/* Direct value access used by the write barriers and the methodjit */
> +void MarkValueUnbarriered(JSTracer *trc, const js::Value &v, const char *name);

Here too.

@@ +149,5 @@
> +MarkCrossCompartmentValue(JSTracer *trc, const js::HeapValue &v, const char *name);
> +
> +/*
> + * MarkChildren<JSObject> is exposed solely for preWriteBarrier on
> + * JSObject::TradeGuts.  It should not be considered external interface.

One space after the period.

@@ +157,5 @@
> +
> +/*
> + * Trace through the shape and any shapes it contains to mark
> + * non-shape children.  This is exposed to the JS API as
> + * JS_TraceShapeCycleCollectorChildren.

One space after the period.
Comment 4 Terrence Cole [:terrence] 2012-02-09 12:00:51 PST
Created attachment 595840 [details] [diff] [review]
v2: With review feedback.

(In reply to Bill McCloskey (:billm) from comment #3)
> @@ +130,5 @@
> > +}
> > +
> > +template <typename T>
> > +static void
> > +Mark(JSTracer *trc, const HeapPtr<T> &thing, const char *name)
> 
> I don't think any of these intermediate Mark or MarkUnbarriered functions
> are needed. You can might as well just do everything in the macro
> definitions. In the few places where they're called later, it seems like you
> can just use Mark<Type>Unbarriered. The same goes for MarkRoot, MarkRange,
> and MarkRootRange below.

They were very helpful when pushing the marking indirection up through the stack -- I didn't need to add lines to the macros.  That's done now, so no problem removing them.

> ::: js/src/jsgcmark.h
> @@ +36,5 @@
> > + * The cycle collector internally duplicates most of the unexposed logic in
> > + * PushMarkStack so that it can special case and ignore edge cases it doesn't
> > + * care about.  It uses JS_TraceChildren to iterate children, but has some
> > + * important special interfaces that make it a separate path.  See the comment
> > + * on MarkCycleCollectorChildren.
> 
> To be honest, I found this comment pretty confusing. Comments in a header
> file should be pretty high-level, describing how stuff is used, rather than
> how it works. Could you take this out?

I totally forgot about those: notes that I kept as I worked out how the marker works.  I was planning to remove them before cutting a patch.
 
> I think we need a comment here...

Thanks for writing it for me.

> Please group the Mark and IsMarked function together in a separate section.
&
> These Mark functions are used by IonMonkey, so we shouldn't remove them.

I'm not entirely sure what you mean here.  I added a new "Ion Monkey" section and put these back together.
Comment 5 Bill McCloskey (:billm) 2012-02-09 14:31:57 PST
Comment on attachment 595840 [details] [diff] [review]
v2: With review feedback.

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

Excellent, thanks!

::: js/src/jsgcmark.cpp
@@ -1,1 @@
> -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-

Please keep the mode line for emacs. I guess it should go in a separate comment so as not to affect the license boilerplate.

@@ +205,4 @@
>  }
>  
>  void
> +MarkRootGCThing(JSTracer *trc, void *thing, const char *name)

Based on the other ones, I guess this should now be MarkGCThingRoot.

@@ +313,4 @@
>  }
>  
>  void
> +MarkShape(JSTracer *trc, HeapPtr<const Shape> &thing, const char *name)

For now I think this should be a const HeapPtr<const Shape> &thing.

::: js/src/jsgcmark.h
@@ -1,1 @@
> -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-

Same thing here.

@@ +137,5 @@
>   */
>  void
>  MarkCycleCollectorChildren(JSTracer *trc, const Shape *shape);
>  
> +/*** Ion Monkey ***/

Please give this a different name, like "Generic", and put a comment that they're only to be used in templated code, and that it's better to call the marking functions that include the type in the name when possible.

(What I mean before is that these functions are only called from IonMonkey, so if you delete them, then the IonMonkey repo will get broken on the next merge and David will just have to add them back.)
Comment 6 Terrence Cole [:terrence] 2012-02-09 18:01:22 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/61d58d97747a
Comment 7 Phil Ringnalda (:philor) 2012-02-09 21:06:58 PST
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/967b0dbce7e0 - billm thought my bet on bug 714109 being the source of the GC crashes in browser-chrome wasn't a good bet, and that this was better.
Comment 8 Bill McCloskey (:billm) 2012-02-09 21:08:53 PST
It looks like this is missing some NULL checks on the range marking functions.

Also, now I'm thinking that those intermediate Mark and MarkUnbarriered functions you had may have been nice to have, since they make the code easier to step through in the debugger. Would you mind putting them back?
Comment 9 Terrence Cole [:terrence] 2012-02-10 09:51:19 PST
You are right.  I was just looking at the Id/Value Range functions before I implemented those, so that's probably why I accidentally dropped them.  Adding back those and the intermediate template functions.  Will have a patch in a few minutes.
Comment 10 Terrence Cole [:terrence] 2012-02-10 10:01:03 PST
Created attachment 596080 [details] [diff] [review]
v3: Re-added opt null check.

This re-adds the intermediate functions between the macros and MarkInternal and adds an opt null check to MarkRange.  It does not add a null check to MarkRootRange, since it appears that there was not one before.  We assert that the thing isn't null in debug builds, so I'm not sure why this wasn't dying on debug as well.
Comment 11 Terrence Cole [:terrence] 2012-02-10 10:03:16 PST
Try run: https://tbpl.mozilla.org/?tree=Try&rev=d6db6d1c8a53
Comment 12 Terrence Cole [:terrence] 2012-02-14 16:23:30 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef6208def4d
Comment 13 Marco Bonardo [::mak] 2012-02-15 09:11:32 PST
https://hg.mozilla.org/mozilla-central/rev/5ef6208def4d

Note You need to log in before you can comment on or make changes to this bug.