1135985 - Common up and Inline all the marking paths

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

In Marking.cpp, there are almost as many mechanisms as there are types of things to mark:

  1) Directly inlined in processMarkStackTop. This is objects, slot/element arrays, and now, unboxed representations too. These have a complete, highly optimized unrolled, copy of markChildren embedded under a label. The purpose here is to avoid calls, even direct calls, for the super, super hot stuff. PushMarkStack defers to this implementation by pushing to the mark stack.

  2) Indirectly inlined in processMarkStackTop via an always-inlined "call". This is for String and Symbol. It is similar to (1) except that PushMarkStack shares the implementation via a call to ScanString/ScanSymbol -- an implementation of markChildren, but specialized for GCMarker so that it can use the mark stack directly, when needed.

  3&4) Out of line, but still via the mark stack, in processMarkStackOther. This is for ObjectGroup, JitCode, and "saved value arrays". Presumably these are not as hot, but must still be marked through the mark stack because they can recurse. JitCode just dispatches to trace (what every other type calls markChildren), whereas ObjectGroup has a duplicate implementation of PushMarkStackTop and MarkChildren, which are basically identical with different types for the marker.

  5) Ignoring the mark stack entirely. JSScript and LazyScript's implementation of PushMarkStack dispatch directly to markChildren, going out of line and fully abstract immediately.

Comment 1

[Terrence Cole [:terrence]]

Attachment: 1_simplify_pushmarkstack-v0.diff

This simplifies and removes a bunch of unnecessary indirection without changing any semantics.

12 files changed, 193 insertions(+), 275 deletions(-)

The next step is to start removing mechanisms, so further work might have performance impacts.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8568689 [details] [diff] [review]
1_simplify_pushmarkstack-v0.diff

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

I think I follow everything happening here, and it's a nice removal of some excess layering.

::: js/src/gc/Marking.cpp
@@ +1625,5 @@
>          } else if (v.isObject()) {
>              JSObject *obj2 = &v.toObject();
> +            MOZ_ASSERT(obj->compartment() == obj2->compartment());
> +            if (mark(obj2)) {
> +                // Save the rest of this value array for later and start scanning obj2's children.N

s/N$//

::: js/src/gc/Tracer.h
@@ +160,5 @@
> +    // Mark the given GC thing and trace its children at some point.
> +    void trace(JSObject *thing) { markAndPush(ObjectTag, thing); }
> +    void trace(ObjectGroup *thing) { markAndPush(GroupTag, thing); }
> +    void trace(jit::JitCode *thing) { markAndPush(JitCodeTag, thing); }
> +    // The following trace methods trace immediately, go out-of-line to do so.

", but" or something?

@@ -229,5 @@
>      enum StackTag {
>          ValueArrayTag,
>          ObjectTag,
>          GroupTag,
> -        XmlTag,

Wow

@@ +273,5 @@
> +            thing->markChildren(this);
> +    }
> +
> +    // Mark the given GC thing, but do not trace its children. Return true
> +    // if the thing became marked.

You don't want to call this markIfUnmarked? I guess that would make it different from the others, and markAndTraverseIfUnmarked is a bit of a mouthful.

Comment 3

[Terrence Cole [:terrence]]

Attachment: 2_dont_use_marker_for_buffering_gray_roots-v0.diff

So we have a clear distinction between callback tracers and marking tracers /except/ for the oddity of BufferGrayRoots. For bufferGrayRoots, we implement a special mode on GCMarker -- not with RAII or anything, manual calls to start and end -- to do the buffering. In this special mode we set a callback on the gcmarker so that it will act like a JSTracer in MarkInternal, but will still act like a GCMarker anywhere that tests for IS_GC_MARKING_TRACER.

Of course, because this is the one and only exception to "GCMarker does not have a callback," everything in our marking paths, JSTracer *and* GCMarker, has to know about and work around this weird special case. It's a total pain. I've avoided touching it until now because I don't want to have to untangle the massive complexity that this path requires. I mean, obviously, it must need full access to the mark stack and all the special cutouts to normal marking that IS_GC_MARKING_TRACER entails. There's no other possible reason you'd take on all that complexity unless you really needed it, right?

...

Hahahaha. Just kidding. The actual tracer is trivial and can be swapped out for a JSTracer trivially. This saves about 30 lines up front and I'm sure there's a ton more stuff I'm missing.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8568911 [details] [diff] [review]
2_dont_use_marker_for_buffering_gray_roots-v0.diff

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

Great, that's much better.

Comment 5

[Terrence Cole [:terrence]]

Attachment: 3_bifurcate_callback_and_marking_tracers-v0.diff

With bufferGrayRoots out of the picture, implementing CallbackTracer as a sibling of GCMarker, instead of a parent, becomes trivial.

Comment 6

[Terrence Cole [:terrence]]

Attachment: 3.1_browser_bits-v0.diff

And the browser bits.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8569313 [details] [diff] [review]
3_bifurcate_callback_and_marking_tracers-v0.diff

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

Now I think about it that never made any sense.

I also like that this gets rid of the IS_GC_MARKING_TRACER macro.

An idea for further improvement - add JSTracer::asMarkingTracer() and asCallbackTracer() to consolidate all those static_casts in one place.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8569315 [details] [diff] [review]
3.1_browser_bits-v0.diff

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

I wonder how many of the other places in the browser could be given this more specific type.  Though it may be a little tricky.  mozilla::dom::TraceBlackJS() probably only takes GC tracers, but doesn't really care about their additional functionality.

::: dom/bindings/BindingUtils.h
@@ +507,5 @@
>  }
>  
>  #ifdef DEBUG
>  void
> +VerifyTraceProtoAndIfaceCacheCalled(JS::CallbackTracer *trc, void **thingp,

While you are here, please move these * to the left to match browser style.

@@ +526,5 @@
>  {
>    MOZ_ASSERT(js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL);
>  
>  #ifdef DEBUG
> +  if (trc->isCallbackTracer()) {

Some kind of asCallbackTracer() that returns null if it isn't one might be a little cleaner, though maybe it isn't widespread enough of a problem to be an issue.

Comment 9

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bc72e91be738
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef1ca68ea42b

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 8568689 [details] [diff] [review]
1_simplify_pushmarkstack-v0.diff

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc72e91be738

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc72e91be738

Comment 12

[Terrence Cole [:terrence]]

Whoops, forgot to set leave-open.

Comment 13

[Terrence Cole [:terrence]]

Patch 2 did not land because it increased the failure rate from timeouts on my try pushes by ~3-4%. It did not seem to have any strong effect on times except where it fell over, and half a day of visual inspection only turned up a couple of potential issues, which try assures me are not at fault.

It took most of a day, but patch 2 has been split up into 11 obviously-correct sub-patches: The sequence of patches and try runs looks like:

bug 1144331
bug 1144369
bug 1144789
bug 1144794
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4085e5abd78
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=701f405c730d
bug 1144811
bug 1144817
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6772fe742761 (corrupted by 503 errors)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18fff96c867d
bug 1144832
bug 1144834
bug 1144920
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef82c33387eb
bug 1144925
bug 1144931
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72b145b1790d

If any of these try runs has the same high failure rate, we'll need a most 2 more runs to narrow down on the culprit.

Comment 14

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fc4fa9499aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4246efc8a25

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4246efc8a25