Closed Bug 1135985 Opened 6 years ago Closed 6 years ago

Common up and Inline all the marking paths


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox39 --- fixed


(Reporter: terrence, Assigned: terrence)


(Blocks 1 open bug)



(4 files)

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.
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.
Assignee: nobody → terrence
Attachment #8568689 - Flags: review?(jcoppeard)
Comment on attachment 8568689 [details] [diff] [review]

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


::: 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,


@@ +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.
Attachment #8568689 - Flags: review?(jcoppeard) → review+
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.
Attachment #8568911 - Flags: review?(jcoppeard)
Comment on attachment 8568911 [details] [diff] [review]

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

Great, that's much better.
Attachment #8568911 - Flags: review?(jcoppeard) → review+
With bufferGrayRoots out of the picture, implementing CallbackTracer as a sibling of GCMarker, instead of a parent, becomes trivial.
Attachment #8569313 - Flags: review?(jcoppeard)
And the browser bits.
Attachment #8569315 - Flags: review?(continuation)
Comment on attachment 8569313 [details] [diff] [review]

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.
Attachment #8569313 - Flags: review?(jcoppeard) → review+
Comment on attachment 8569315 [details] [diff] [review]

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.
Attachment #8569315 - Flags: review?(continuation) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Whoops, forgot to set leave-open.
Keywords: leave-open
Resolution: FIXED → ---
Depends on: 1144331
Depends on: 1144369
Depends on: 1144789
Depends on: 1144794
Depends on: 1144811
Depends on: 1144817
Depends on: 1144832
Depends on: 1144834
Depends on: 1144920
Depends on: 1144925
Depends on: 1144931
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
bug 1144811
bug 1144817 (corrupted by 503 errors)
bug 1144832
bug 1144834
bug 1144920
bug 1144925
bug 1144931

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.
Attachment #8568911 - Flags: checkin+
Keywords: leave-open
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.