1163790 - Share Class-based tracing optimizations between marking and tenuring

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 26_share_visit_tracelist-v0.diff

markTraceList and scan_unboxed are close enough to identical that a simple shim functor can bridge them easily. Since the unboxed scanning in processMarkStackTop does not re-enter and always exits after succeeding, there is no reason it needs to be directly in processMarkStackTop.

Comment 1

[Terrence Cole [:terrence]]

Attachment: 27_share_maybeInlineClassTracing-v0.diff

We can also now easily share the class-based inlining we do for unboxed and typed objects for the same reason.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 8604312 [details] [diff] [review]
27_share_maybeInlineClassTracing-v0.diff

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

::: js/src/gc/Marking.cpp
@@ +1068,5 @@
>  
> +// Returns true if we successfully inline tracing for the given Class.
> +template <typename Functor, typename... Args>
> +static inline bool
> +MaybeInlineClassTracing(Functor f, JSObject* obj, const Class* clasp, Args&&... args)

This interface is kind of weird.  Could this just call clasp->trace() itself and then you can rename this CallTraceHook() or something.

@@ +1290,5 @@
>              clasp->trace(this, obj);
>          }
>  
>          if (!clasp->isNative())
>              return;

This test only needs to be done if clasp->trace is non-NULL, i.e. all non-native classes have trace hooks.  I doubt we assert that anywhere but we could.  So you could remove this test and the one in TenuringTracer::traceObject and put one in the common function you're adding.

Comment 3

[Terrence Cole [:terrence]]

Attachment: 26.2_share_maybeInlineClassTracing-v0.diff

(In reply to Brian Hackett (:bhackett) from comment #2)
> Comment on attachment 8604312 [details] [diff] [review]
> 27_share_maybeInlineClassTracing-v0.diff
> 
> Review of attachment 8604312 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/Marking.cpp
> @@ +1068,5 @@
> >  
> > +// Returns true if we successfully inline tracing for the given Class.
> > +template <typename Functor, typename... Args>
> > +static inline bool
> > +MaybeInlineClassTracing(Functor f, JSObject* obj, const Class* clasp, Args&&... args)
> 
> This interface is kind of weird.  Could this just call clasp->trace() itself
> and then you can rename this CallTraceHook() or something.

Yeah, I was considering a follow-up to do that, but I might as well just do it now.

> @@ +1290,5 @@
> >              clasp->trace(this, obj);
> >          }
> >  
> >          if (!clasp->isNative())
> >              return;
> 
> This test only needs to be done if clasp->trace is non-NULL, i.e. all
> non-native classes have trace hooks.  I doubt we assert that anywhere but we
> could.  So you could remove this test and the one in
> TenuringTracer::traceObject and put one in the common function you're adding.

Added! Once we have this, it basically becomes a matter of knowing if there is additional native-specific tracing to do. To make this dependence totally explicit I've made CallTraceHook return a non-null NativeObject* if there is additional tracing to do.

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 8605935 [details] [diff] [review]
26.2_share_maybeInlineClassTracing-v0.diff

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

::: js/src/gc/Marking.cpp
@@ +1075,5 @@
> +    const Class* clasp = obj->getClass();
> +    MOZ_ASSERT(clasp);
> +    MOZ_ASSERT(obj->isNative() == clasp->isNative());
> +
> +    MOZ_ASSERT_IF(!clasp->trace, obj->isNative());

This assert is unnecessary.

Comment 5

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5a9f672a712

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d26ba5fa305
https://hg.mozilla.org/integration/mozilla-inbound/rev/489740128625

Comment 7

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97b85843afe2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c288b6b0166d

Comment 8

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=302777514294

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/849b7fbb2bbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/0269d7785019

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/849b7fbb2bbe
https://hg.mozilla.org/mozilla-central/rev/0269d7785019