Common up and Inline all the marking paths

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks 1 bug)

Trunk
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

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

Comment 1

4 years ago
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
Status: NEW → ASSIGNED
Attachment #8568689 - Flags: review?(jcoppeard)
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.
Attachment #8568689 - Flags: review?(jcoppeard) → review+
Assignee

Comment 3

4 years ago
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]
2_dont_use_marker_for_buffering_gray_roots-v0.diff

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

Great, that's much better.
Attachment #8568911 - Flags: review?(jcoppeard) → review+
Assignee

Comment 5

4 years ago
With bufferGrayRoots out of the picture, implementing CallbackTracer as a sibling of GCMarker, instead of a parent, becomes trivial.
Attachment #8569313 - Flags: review?(jcoppeard)
Assignee

Comment 6

4 years ago
And the browser bits.
Attachment #8569315 - Flags: review?(continuation)
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.
Attachment #8569313 - Flags: review?(jcoppeard) → review+
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.
Attachment #8569315 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/bc72e91be738
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee

Comment 12

4 years ago
Whoops, forgot to set leave-open.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Assignee

Updated

4 years ago
Depends on: 1144331
Assignee

Updated

4 years ago
Depends on: 1144369
Assignee

Updated

4 years ago
Depends on: 1144789
Assignee

Updated

4 years ago
Depends on: 1144794
Assignee

Updated

4 years ago
Depends on: 1144811
Assignee

Updated

4 years ago
Depends on: 1144817
Assignee

Updated

4 years ago
Depends on: 1144832
Assignee

Updated

4 years ago
Depends on: 1144834
Assignee

Updated

4 years ago
Depends on: 1144920
Assignee

Updated

4 years ago
Depends on: 1144925
Assignee

Updated

4 years ago
Depends on: 1144931
Assignee

Comment 13

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

Updated

4 years ago
Attachment #8568911 - Flags: checkin+
Assignee

Updated

4 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/a4246efc8a25
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.