Closed Bug 701415 Opened 8 years ago Closed 8 years ago

Generate detailed JS information for cycle collector dumps in opt builds

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
dbaron's unlanded patch 1 in Bug 652056 looks like it may do some or all of what I want to do in here.
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.
Attached patch untested WIP, against beta 10 (obsolete) — Splinter Review
Blocks: 723783
Attached patch rebased, against tip (obsolete) — Splinter Review
Attachment #588567 - Attachment is obsolete: true
Attached patch actual full patch (obsolete) — Splinter Review
Attachment #596478 - Attachment is obsolete: true
Attached patch unbitrottedSplinter Review
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.
Great, thanks for checking!
Assignee: general → continuation
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.
Blocks: 753614
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+
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
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.