Templatize the tracing paths

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks 2 bugs)

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

4 years ago
Issues with the current implementation:

* Poor opt-build stack trace names: We explicitly duplicate all the code with a giant macro expansion: e.g. MarkObject(JSObject**) and MarkObject(JSFunction**) get separate bodies. Generally the compiler is smart enough to coalesce the identical implementations, but then we end up with IsAccessorShapeAboutToBeFinalized being used for all types of Objects, Scripts, etc, not to mention Shapes. Templates /should/ get us better names here, although it's hard to verify without deploying it.

* Generating way, way too much code: We're currently macro expanding a giant block of code for each of our most-derived types, then calling our templated internal bits with the derived type, leading to different copies of CheckMarkedThing, MarkInternal, etc for JSObject, JSFunction, etc. The optimizer gets rid of most of this slop, but there's no reason to generate it in the first place. The new paths accept derived types for ease of use, but immediately downcast to the layout type before doing anything further so that we have a minimal number of instances of the internal marking bits.

* We do not statically verify that the indicated base type is correct: Since we use the derived type directly, we could easily generate and use MarkObject(Shape*) and never notice. The new implementation uses compile-time C++ t  automatically select the right tracing type without ambiguity.

* Too many inapplicable type-specific runtime checks: Despite generating code for the most derived types, we don't take advantage of this type-info. We end up checking all types to see if they are PermanentAtoms, for example.

* Too many inapplicable trace-kind-specific runtime checks: Since Value/Id unpacking happens at the top level instead of the bottom level, all Value/Id unpacking has to set the tracing name, location, etc and cart around the name arg. Since Value/Id can only contain base-typed edges, we can easily treat them as base typed pointers and do the unpacking after selecting the trace kind.

* The existing paths are pointer specific: we have to duplicate all of our generated functions for Value and Id with copy-and-paste code.

* Marking is the wrong name: What we're doing is tracing; the JSTracer type determines the side-effects of traversal. This changes the MarkType terminology to TraceEdge.

Weaknesses of the new approach:

* Still has a ton of MACROs: On the other hand, these are now small and limited to the cpp side; the header has trivial, non-macro definitions for the tracing callbacks.

* Uses some fugly C++: Taking advantage of type information in C++, even with C++11, is still fantastically ugly. Use of particularly ugly features is at least fairly limited. I've included extensive comments above these places to limit the difficulty of understanding what's going on. I didn't have too much trouble remembering how things worked after a month of not looking at it.

* Now treats tracing info in an inconsistent manner: Ideally we'd be able to move the tracing details stuff from JSTracer to JS::CallbackTracer. Unfortunately we still have to interact with the legacy tracer, so we can't totally ignore it on the DoMarking side yet.

* Its a fairly radical change: If my performance predictions are off, we may need to backout or patch heavily.
Attachment #8582730 - Flags: review?(sphink)
Attachment #8582730 - Flags: review?(jcoppeard)
Assignee

Comment 1

4 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2202110815e5

Replace our generic Mark functions with TraceEdge. The biggest surprise here is that we need to move HashKeyRef to Marking.h. I have literally /no idea/ where it was getting its forward declaration of Mark before: it certainly shouldn't be possible.
Attachment #8582735 - Flags: review?(jcoppeard)
Comment on attachment 8582730 [details] [diff] [review]
templatize_tracing_layer-v0.diff

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

Nice!

::: js/src/gc/Marking.cpp
@@ +390,5 @@
> +{
> +    auto layout = reinterpret_cast<typename PtrBaseGCType<T>::type *>(thingp->unsafeGet());
> +    DispatchToTracer(trc, layout, name, -1);
> +}
> +#define INSTANTIATE_ALL_VALID_TRACE_FUNCTIONS(type) \

We could common up all these instantiations into one block to reduce the amount of macro code.

@@ +478,5 @@
> +DoMarking(GCMarker *gcmarker, T thing)
> +{
> +    static_assert(!IsBaseOf<JSString, T>::value, "strings must use the string marking path");
> +    static_assert(!IsBaseOf<JSObject, T>::value, "objects must use the object marking path");
> +    static_assert(!IsBaseOf<JSScript, T>::value, "scripts must use the script marking path");

T is a pointer here so we either need to use RemovePointer or do something like:

    static_assert(!IsSame<JSObject*, T>::value, etc...
Attachment #8582730 - Flags: review?(jcoppeard) → review+
Attachment #8582735 - Flags: review?(jcoppeard) → review+
Assignee

Updated

4 years ago
Blocks: 1147533
Comment on attachment 8582730 [details] [diff] [review]
templatize_tracing_layer-v0.diff

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

::: js/src/gc/Marking.cpp
@@ -188,5 @@
>      MOZ_ASSERT_IF(!MovingTracer::IsMovingTracer(trc), CurrentThreadCanAccessZone(zone));
>      MOZ_ASSERT_IF(!MovingTracer::IsMovingTracer(trc), CurrentThreadCanAccessRuntime(rt));
>  
>      MOZ_ASSERT(zone->runtimeFromAnyThread() == trc->runtime());
> -    MOZ_ASSERT(trc->hasTracingDetails());

I guess I didn't totally follow the logic, but what happened to this assert?

@@ +363,5 @@
> +                    : IsBaseOf<Shape, T>::value        ? TraceKind::Shape
> +                    : IsBaseOf<BaseShape, T>::value    ? TraceKind::BaseShape
> +                    : IsBaseOf<jit::JitCode, T>::value ? TraceKind::JitCode
> +                    : IsBaseOf<LazyScript, T>::value   ? TraceKind::LazyScript
> +                    :                                    TraceKind::ObjectGroup>

I kinda wonder if this would be slightly more readable if instead of IsBaseOf<Base, T>::value it were TypeRelationship<T, Base>::isSubclassOf or ::isa (and presumably there would be ::isConvertibleTo and whatever else is useful.) Or TypeRelationship<Base, T>::isBaseClassOf.

On second thought... nah. (It's just that IsBaseOf<Base, T>::value reads a little funny. I initially read it "is the base class of Base T?" which is backwards. I guess it's meant to be "Base IsBaseOf T?")

@@ +366,5 @@
> +                    : IsBaseOf<LazyScript, T>::value   ? TraceKind::LazyScript
> +                    :                                    TraceKind::ObjectGroup>
> +struct BaseGCType;
> +#define IMPL_BASE_GC_TYPE(name, type_) \
> +    template <typename T> struct BaseGCType<T, TraceKind:: name> { typedef type_ type; };

is the space between TraceKind:: and name needed?

@@ +467,5 @@
> +    CheckMarkedThing(trc, *thingp);
> +
> +    if (trc->isMarkingTracer())
> +        return DoMarking(static_cast<GCMarker*>(trc), *thingp);
> +    return DoTracing(static_cast<JS::CallbackTracer*>(trc), thingp, name, i);

Ooh, I like this little chunk of logic.

@@ +514,5 @@
> +DoMarking<JSString *>(GCMarker *gcmarker, JSString *str)
> +{
> +    // Don't mark permanent atoms, as they may be associated with another
> +    // runtime. Note that PushMarkStack() also checks this, but the tests and
> +    // maybeAlive write below should only be done on the main thread.

"maybeAlive flag write", maybe? But wait -- what maybeAlive write? Is this just for the zone test?

@@ +587,5 @@
> +    } else if (vp->isString()) {
> +        JSString *prior = vp->toString();
> +        JSString *str = prior;
> +        DoTracing(trc, &str, name, i);
> +        if (str != prior)

Heh. Do we need a MOZ_IMPOSSIBLE() to go along with MOZ_UNLIKELY()? :-)
Attachment #8582730 - Flags: review?(sphink) → review+
Assignee

Updated

4 years ago
Blocks: 1147588
Assignee

Comment 4

4 years ago
(In reply to Steve Fink [:sfink, :s:] from comment #3)
> Comment on attachment 8582730 [details] [diff] [review]
> templatize_tracing_layer-v0.diff
> 
> Review of attachment 8582730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/Marking.cpp
> @@ -188,5 @@
> >      MOZ_ASSERT_IF(!MovingTracer::IsMovingTracer(trc), CurrentThreadCanAccessZone(zone));
> >      MOZ_ASSERT_IF(!MovingTracer::IsMovingTracer(trc), CurrentThreadCanAccessRuntime(rt));
> >  
> >      MOZ_ASSERT(zone->runtimeFromAnyThread() == trc->runtime());
> > -    MOZ_ASSERT(trc->hasTracingDetails());
> 
> I guess I didn't totally follow the logic, but what happened to this assert?

It is now only called under DoTracing and asserting it right after setting it seemed silly.

> @@ +363,5 @@
> > +                    : IsBaseOf<Shape, T>::value        ? TraceKind::Shape
> > +                    : IsBaseOf<BaseShape, T>::value    ? TraceKind::BaseShape
> > +                    : IsBaseOf<jit::JitCode, T>::value ? TraceKind::JitCode
> > +                    : IsBaseOf<LazyScript, T>::value   ? TraceKind::LazyScript
> > +                    :                                    TraceKind::ObjectGroup>
> 
> I kinda wonder if this would be slightly more readable if instead of
> IsBaseOf<Base, T>::value it were TypeRelationship<T, Base>::isSubclassOf or
> ::isa (and presumably there would be ::isConvertibleTo and whatever else is
> useful.) Or TypeRelationship<Base, T>::isBaseClassOf.

I agree, that would be nice. Take it up with whoever stamped is_base_of into the spec. :-(

> On second thought... nah. (It's just that IsBaseOf<Base, T>::value reads a
> little funny. I initially read it "is the base class of Base T?" which is
> backwards. I guess it's meant to be "Base IsBaseOf T?")

Same here!

> @@ +366,5 @@
> > +                    : IsBaseOf<LazyScript, T>::value   ? TraceKind::LazyScript
> > +                    :                                    TraceKind::ObjectGroup>
> > +struct BaseGCType;
> > +#define IMPL_BASE_GC_TYPE(name, type_) \
> > +    template <typename T> struct BaseGCType<T, TraceKind:: name> { typedef type_ type; };
> 
> is the space between TraceKind:: and name needed?

Yes. Neither clang nor gcc expanded "name" if it was adjacent.

> @@ +467,5 @@
> > +    CheckMarkedThing(trc, *thingp);
> > +
> > +    if (trc->isMarkingTracer())
> > +        return DoMarking(static_cast<GCMarker*>(trc), *thingp);
> > +    return DoTracing(static_cast<JS::CallbackTracer*>(trc), thingp, name, i);
> 
> Ooh, I like this little chunk of logic.

:-D  Yup, this is the crux of it.

Once everything flows through here, we can add |if (IsNurseryTracer(trc)) DoNurseryTrace(trc)| and fairly trivially strip all of the indirect calls from nursery collection. My initial hacks show this should get us a 20% speed boost to nursery collection.

> @@ +514,5 @@
> > +DoMarking<JSString *>(GCMarker *gcmarker, JSString *str)
> > +{
> > +    // Don't mark permanent atoms, as they may be associated with another
> > +    // runtime. Note that PushMarkStack() also checks this, but the tests and
> > +    // maybeAlive write below should only be done on the main thread.
> 
> "maybeAlive flag write", maybe? But wait -- what maybeAlive write? Is this
> just for the zone test?

Good catch! Now that we're not even trying to call SetMaybeAlive for String, this comment can be simplified.

> @@ +587,5 @@
> > +    } else if (vp->isString()) {
> > +        JSString *prior = vp->toString();
> > +        JSString *str = prior;
> > +        DoTracing(trc, &str, name, i);
> > +        if (str != prior)
> 
> Heh. Do we need a MOZ_IMPOSSIBLE() to go along with MOZ_UNLIKELY()? :-)

AFAICT, almost all of these sites have been future proofed already. Will be useful as soon as we start compacting other thing kinds.
Assignee

Updated

4 years ago
Blocks: 1147665
Assignee

Updated

4 years ago
Blocks: 1147669
Assignee

Updated

4 years ago
Depends on: 1148534
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/99415fbccf83 because something in this push caused frequent 10.10 debug devtools-2 assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8197104&repo=mozilla-inbound
And the 853e3ad56dad patch has the wrong bug number in it.
Assignee

Comment 8

4 years ago
(In reply to Phil Ringnalda (:philor) from comment #7)
> And the 853e3ad56dad patch has the wrong bug number in it.

Thanks for the heads up! Fixed in the current push.

The problem before was in the last patch in the series (bug 1147670):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=514b9ba692a9

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c20d08789e80
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/34efecb56e56
Assignee

Updated

4 years ago
Blocks: 1149352
https://hg.mozilla.org/mozilla-central/rev/c20d08789e80
https://hg.mozilla.org/mozilla-central/rev/34efecb56e56
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.