820233 - Eliminate calls to js::GCThingTraceKind from NoteJSHolder

Status: RESOLVED DUPLICATE of bug 1105069

(Core :: JavaScript Engine, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

I was profiling base level CC and since FF UI has plenty of JS, lots of the time
is spent tracing JS, and there GCThingTraceKind shows up in the profiles a bit.
I wonder whether it could be possible to inline that function.

Comment 1

[Andrew McCreight [:mccr8]]

In the first slice of the CC, we still spend a decent chunk of time running GCThingTraceKind.  I think that almost every place that calls NoteJSRoot and NoteJSChild knows what kind it is dealing with, so once bug 1105069 has landed, it should be easy to just eliminate these calls and make the CC traverse interface strongly typed and just get rid of those calls. I'm not sure if bug 1105069 itself does that.

Comment 2

[Andrew McCreight [:mccr8]]

NoteJSRoot seems to only be called in XPCJSRuntime::SuspectWrappedNative, so the arg type could just be changed to JSObject*.

Comment 3

[Andrew McCreight [:mccr8]]

It looks like this can be done locally by eliminating the indirection of TraceCallbackFunc.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: eliminate indirection in CheckParticipates, untested WIP

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: eliminate indirection in CheckParticipates, WIP

Also need to filter out values that are strings.  This passed a tiny bit of local testing.

Comment 6

[Andrew McCreight [:mccr8]]

I did some testing, with 20 tabs of TechCrunch open, timing CycleCollectedJSRuntime::TraverseNativeRoots.  Sometimes it doesn't take any time, but about half of the time it does take some time.  Without this patch, the long ones were taking 3.9ms to 4.4ms.  With the patch, they were taking 3.4ms to 3.6ms.  That's a pretty decent improvement, around 10%, which is what smaug was seeing js::GCThingTraceKind take on his profile, though obviously it is still taking quite a while.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Eliminate indirection in CheckParticipates.

try run from a slightly earlier version of the patch:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=982a29c227ca

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> Also need to filter out values that are strings. 


Hmm, why

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #8)
> Hmm, why
Because I assert in JSHolderTraceImpl that we don't pass in any non-CCGCThings.  I think they'll just get filtered out later if we do pass it in.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8533353 [details] [diff] [review]
Eliminate indirection in CheckParticipates.

Since CheckParticipatesInCycleCollection can deal with null-whatever, 
I think we should add a check to NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK
that it is really used for JS::Values only.
Maybe
static_assert(sizeof(*(tmp->_field)) == sizeof(JS::Value));
?
(Doesn't matter too much what is put there, just something which ensures it is used for JS::Values only.)

With that, r+

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #10)
> Since CheckParticipatesInCycleCollection can deal with null-whatever, 
> I think we should add a check to
> NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK
> that it is really used for JS::Values only.
> Maybe
> static_assert(sizeof(*(tmp->_field)) == sizeof(JS::Value));
> ?
> (Doesn't matter too much what is put there, just something which ensures it
> is used for JS::Values only.)

I don't understand how this is really related to this patch?  I agree that sounds like a good idea, though I'd probably do it by making NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK call a new method that takes an argument.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Hmm, looks like I misread something since I thought the current implementation would just
return early if null was passed. But looks like we crash already. So nm.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: Use cellPtr, not void*. WIP.

This still needs some kind of conversion from jsid to GCCellPtr.

Comment 14

[Andrew McCreight [:mccr8]]

Bug 1105069 is going to eliminate the call to js::GCThingTraceKind in a slightly different way so I guess I didn't actually need to write this patch after all.