Share Class-based tracing optimizations between marking and tenuring

RESOLVED FIXED in Firefox 41

Status

()

Core
JavaScript: GC
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8604311 [details] [diff] [review]
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.
Attachment #8604311 - Flags: review?(bhackett1024)
(Assignee)

Comment 1

3 years ago
Created attachment 8604312 [details] [diff] [review]
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.
Attachment #8604312 - Flags: review?(bhackett1024)
Attachment #8604311 - Flags: review?(bhackett1024) → review+
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.
Attachment #8604312 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

3 years ago
Created attachment 8605935 [details] [diff] [review]
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.
Attachment #8604312 - Attachment is obsolete: true
Attachment #8605935 - Flags: review?(bhackett1024)
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.
Attachment #8605935 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/849b7fbb2bbe
https://hg.mozilla.org/mozilla-central/rev/0269d7785019
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.