Last Comment Bug 701415 - Generate detailed JS information for cycle collector dumps in opt builds
: Generate detailed JS information for cycle collector dumps in opt builds
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on:
Blocks: 723783 700645 753614
  Show dependency treegraph
 
Reported: 2011-11-10 09:42 PST by Andrew McCreight [:mccr8]
Modified: 2012-05-16 03:48 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
untested WIP, against beta 10 (5.62 KB, patch)
2012-01-13 17:21 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
rebased, against tip (5.18 KB, patch)
2012-02-12 07:52 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
actual full patch (9.86 KB, patch)
2012-02-12 07:55 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
unbitrotted (9.86 KB, patch)
2012-05-09 16:13 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2011-11-10 09:42:09 PST
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] 2011-11-10 09:47:31 PST
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] 2011-11-11 09:19:49 PST
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 (:billm) 2011-11-11 13:21:57 PST
(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] 2012-01-13 17:21:29 PST
Created attachment 588567 [details] [diff] [review]
untested WIP, against beta 10
Comment 5 Andrew McCreight [:mccr8] 2012-02-12 07:52:08 PST
Created attachment 596478 [details] [diff] [review]
rebased, against tip
Comment 6 Andrew McCreight [:mccr8] 2012-02-12 07:55:15 PST
Created attachment 596479 [details] [diff] [review]
actual full patch
Comment 7 Andrew McCreight [:mccr8] 2012-05-09 16:13:36 PDT
Created attachment 622567 [details] [diff] [review]
unbitrotted
Comment 8 Bill McCloskey (:billm) 2012-05-09 16:41:18 PDT
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] 2012-05-09 16:41:53 PDT
Great, thanks for checking!
Comment 10 Andrew McCreight [:mccr8] 2012-05-09 17:45:54 PDT
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] 2012-05-10 07:25:11 PDT
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 (:billm) 2012-05-14 12:27:07 PDT
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] 2012-05-15 10:33:20 PDT
I changed PrintX names to GetX.

https://hg.mozilla.org/integration/mozilla-inbound/rev/60e143614688
Comment 14 Ed Morley [:emorley] 2012-05-16 03:48:40 PDT
https://hg.mozilla.org/mozilla-central/rev/60e143614688

Note You need to log in before you can comment on or make changes to this bug.