701415 - Generate detailed JS information for cycle collector dumps in opt builds

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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

Comment 2

[Andrew McCreight [:mccr8]]

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?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: untested WIP, against beta 10

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: rebased, against tip

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: actual full patch

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: unbitrotted

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 9

[Andrew McCreight [:mccr8]]

Great, thanks for checking!

Comment 10

[Andrew McCreight [:mccr8]]

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.

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 622567 [details] [diff] [review]
unbitrotted

Try run looked fine, and I checked that the CC dump did have edge names.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 13

[Andrew McCreight [:mccr8]]

I changed PrintX names to GetX.

https://hg.mozilla.org/integration/mozilla-inbound/rev/60e143614688

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/60e143614688