The default bug view has changed. See this FAQ.

Generate detailed JS information for cycle collector dumps in opt builds

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Currently, detailed information in JS_TraceChildren is only generated in debug builds.  Bill says that this can be done without impacting the GC because the GC and CC use different mark paths.  We still need to be careful about not doing a lot of string building on non-dumping CCs, but that can probably be done by setting some kind of flag in the tracer.  Checking a boolean flag in a bunch of places probably isn't that bad for the CC.
(Assignee)

Comment 1

6 years ago
dbaron's unlanded patch 1 in Bug 652056 looks like it may do some or all of what I want to do in here.
(Assignee)

Comment 2

6 years ago
Part 1 of this patch is to grab the values set by the JS engine's debug printer, like dbaron's patch (though it can be made simpler now that I added a JS API function to grab the edge value from a printer).  However, the setting of printers are actually disabled in non-DEBUG builds, by setting JS_SET_TRACING_DETAILS to be a no-op.

Bill, it looks to me like these macros are called in the GC's marking.  I guess I could add all 3 arguments to the tracer to Mark, and then only set them once we test (IS_GC_MARKING_TRACER(trc)) but that seems kind of gross.  Do you have any ideas?
(In reply to Andrew McCreight [:mccr8] from comment #2)
> Bill, it looks to me like these macros are called in the GC's marking.  I
> guess I could add all 3 arguments to the tracer to Mark, and then only set
> them once we test (IS_GC_MARKING_TRACER(trc)) but that seems kind of gross. 
> Do you have any ideas?

I think all we need is to remove the DEBUG-only stuff in JS_SET_TRACING_DETAILS and at the end of Mark. Most marking is done off these paths, so I don't expect this would hurt performance. But we should still check.
(Assignee)

Comment 4

5 years ago
Created attachment 588567 [details] [diff] [review]
untested WIP, against beta 10
(Assignee)

Updated

5 years ago
Blocks: 723783
(Assignee)

Comment 5

5 years ago
Created attachment 596478 [details] [diff] [review]
rebased, against tip
Attachment #588567 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 596479 [details] [diff] [review]
actual full patch
Attachment #596478 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 622567 [details] [diff] [review]
unbitrotted
Attachment #596479 - Attachment is obsolete: true
I just tested earley-boyer and splay with and without this patch. It seems to be faster when the debug information is included. That's odd, but I think it means we shouldn't worry about performance.
(Assignee)

Comment 9

5 years ago
Great, thanks for checking!
(Assignee)

Updated

5 years ago
Assignee: general → continuation
(Assignee)

Comment 10

5 years ago
Hmm.  I guess it would make sense to enable DumpHeapComplete, too.  Maybe I'll do that in a follow up patch, as this is probably close to being landable.
(Assignee)

Updated

5 years ago
Blocks: 753614
(Assignee)

Comment 11

5 years ago
Comment on attachment 622567 [details] [diff] [review]
unbitrotted

Try run looked fine, and I checked that the CC dump did have edge names.
Attachment #622567 - Flags: review?(wmccloskey)
Comment on attachment 622567 [details] [diff] [review]
unbitrotted

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

Looks good. I also just tried the JS realtime raytracer benchmark, and the mark times don't seem to be affected by this.

While you're touching this code, could you change the all PrintX names to GetX?
Attachment #622567 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 13

5 years ago
I changed PrintX names to GetX.

https://hg.mozilla.org/integration/mozilla-inbound/rev/60e143614688
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/60e143614688
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.