Closed Bug 820233 Opened 7 years ago Closed 5 years ago

Eliminate calls to js::GCThingTraceKind from NoteJSHolder

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1105069

People

(Reporter: smaug, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(4 obsolete files)

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.
Assignee: general → nobody
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.
Depends on: 1105069
Summary: Inline js::GCThingTraceKind ? → js::GCThingTraceKind takes a lot of time in CheckParticipatesInCycleCollection
Summary: js::GCThingTraceKind takes a lot of time in CheckParticipatesInCycleCollection → Strongly type JS callbacks in nsCycleCollectionNoteRootCallback and nsCycleCollectionTraversalCallback
Blocks: 698919
NoteJSRoot seems to only be called in XPCJSRuntime::SuspectWrappedNative, so the arg type could just be changed to JSObject*.
Assignee: nobody → continuation
It looks like this can be done locally by eliminating the indirection of TraceCallbackFunc.
Summary: Strongly type JS callbacks in nsCycleCollectionNoteRootCallback and nsCycleCollectionTraversalCallback → Eliminate calls to js::GCThingTraceKind from NoteJSHolder
Also need to filter out values that are strings.  This passed a tiny bit of local testing.
Attachment #8532830 - Attachment is obsolete: true
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.
try run from a slightly earlier version of the patch:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=982a29c227ca
Attachment #8533353 - Flags: review?(bugs)
(In reply to Andrew McCreight [:mccr8] from comment #5)
> Also need to filter out values that are strings. 


Hmm, why
(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 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+
Attachment #8533353 - Flags: review?(bugs) → review+
(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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Attachment #8532831 - Attachment is obsolete: true
Attached patch Use cellPtr, not void*. WIP. (obsolete) — Splinter Review
This still needs some kind of conversion from jsid to GCCellPtr.
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.
Attachment #8533353 - Attachment is obsolete: true
Attachment #8533938 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1105069
You need to log in before you can comment on or make changes to this bug.