Closed
Bug 1380286
Opened 7 years ago
Closed 7 years ago
Introduce ProfilerStackCollector
Categories
(Core :: Gecko Profiler, enhancement)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(1 file, 2 obsolete files)
28.44 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
This allows code outside the profiler to get fully interleaved stack traces containing frames from the pseudo-stack, native stack, and JS stack.
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
(Copied from bug 1367406) mystor, please take a look at ProfilerStackCollector and see if it fits your needs. There are two things I haven't addressed yet. - Privacy concerns about the dynamic strings. Is the dynamic string always privacy-sensitive? If so, that would make things easy, because BHRStackCollector could just always ignore the aStr argument. - I'm not sure what additional processing of JITReturnAddr frames is required.
Assignee | ||
Updated•7 years ago
|
Attachment #8885667 -
Flags: feedback?(michael)
Comment 3•7 years ago
|
||
Comment on attachment 8885667 [details] [diff] [review] Introduce ProfilerStackCollector Review of attachment 8885667 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good, but I think that we need to make sure that we don't call methods on ActivePS, as it won't be active while BHR is running. ::: tools/profiler/core/platform.cpp @@ +668,1 @@ > bool includeDynamicString = !ActivePS::FeaturePrivacy(aLock); We can't read from ActivePS here. @@ +3074,5 @@ > + { > + MergeStacks(lock, isSynchronous, *info, aRegs, nativeStack, > + aCollector); > + > + if (ActivePS::FeatureLeaf(lock)) { profiler_suspend_and_sample_thread can be called when ActivePS is not present. We can't call FeatureLeaf on it. We should make profiler_suspend_and_sample_thread take in a features flag list explicitly. ::: tools/profiler/public/GeckoProfiler.h @@ +276,5 @@ > bool aSampleNative = true)) > > +// An object of this class is passed to profiler_suspend_and_sample_thread(). > +// For each stack frame, one of the Collect methods will be called. The > +// destructor can be used to process the gathered stack frames. We pass in this object by reference, when the destructor is run is controlled by the caller, so it seems strange to reference it here. You mention that the target thread is suspended while these methods are being called on the definition of profiler_suspend_and_sample_thread, but it seems like a good idea to repeat that here. @@ +291,5 @@ > + virtual void SetIsMainThread() {} > + > + virtual void CollectNativeLeafAddr(void* aAddr) = 0; > + > + virtual void CollectJitReturnAddr(void* aAddr) = 0; Do we need to be on the main thread in order to extract information about the stack frame for this JitReturnAddr? I would assume so, but just wanted to confirm. @@ +294,5 @@ > + > + virtual void CollectJitReturnAddr(void* aAddr) = 0; > + > + // aLabel is static and never null. aStr may be null. aLineNumber may be -1. > + virtual void CollectCodeLocation( It would be nice to know if this function is defined in content or chrome code. I'd like to be able to report dynamic strings in the case that they are present in chrome code. @@ +297,5 @@ > + // aLabel is static and never null. aStr may be null. aLineNumber may be -1. > + virtual void CollectCodeLocation( > + const char* aLabel, const char* aStr, int aLineNumber, > + mozilla::Maybe<js::ProfileEntry::Category> aCategory) = 0; > +}; IIRC the old code also included pseudostack entries here, but I don't see a callback for PseudoStack frames. Are they no longer collected or perhaps counted as CodeLocations?
Attachment #8885667 -
Flags: feedback?(michael) → feedback+
Assignee | ||
Comment 4•7 years ago
|
||
> > + virtual void CollectJitReturnAddr(void* aAddr) = 0; > > Do we need to be on the main thread in order to extract information about > the stack frame for this JitReturnAddr? I would assume so, but just wanted > to confirm. I don't know. Where else in the profiler does this extraction occur? Is it the stuff in ProfileBuffer::StreamSamplesToJSON()? > > + // aLabel is static and never null. aStr may be null. aLineNumber may be -1. > > + virtual void CollectCodeLocation( > > It would be nice to know if this function is defined in content or chrome > code. I'd like to be able to report dynamic strings in the case that they > are present in chrome code. I don't know how do distinguish those two cases, sorry. > > + virtual void CollectCodeLocation( > > + const char* aLabel, const char* aStr, int aLineNumber, > > + mozilla::Maybe<js::ProfileEntry::Category> aCategory) = 0; > > +}; > > IIRC the old code also included pseudostack entries here, but I don't see a > callback for PseudoStack frames. Are they no longer collected or perhaps > counted as CodeLocations? The latter. CodeLocations cover (a) pseudostack frames and (b) JIT frames when doing synchronous samples or Wasm frames. See the two calls to CollectCodeLocation(). It made sense to combine them because they have similar structure, but let me know if this causes difficulties and we can reconsider.
Assignee | ||
Comment 5•7 years ago
|
||
New version makes the following changes: - Tweaks comments in GeckoProfiler.h as suggested. - Passes in features from above, and avoids touching ActivePS in MergeStacks() and related functions. - Adds a test that calls profiler_suspend_and_sample_thread() when the profiler is both active and inactive. mystor, do you think this is getting close to a point where it can land, and then you can tweak it as you need for BHR purposes?
Attachment #8886066 -
Flags: review?(michael)
Assignee | ||
Updated•7 years ago
|
Attachment #8885667 -
Attachment is obsolete: true
Comment 6•7 years ago
|
||
Comment on attachment 8886066 [details] [diff] [review] Introduce ProfilerStackCollector Review of attachment 8886066 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good, and I would be quite happy with it as-is, but there's some more information I'd want to get from the JS pseudostack frames :). r=me if you want to do something like that in a follow-up. ::: tools/profiler/core/platform.cpp @@ +686,2 @@ > if (entry.isJs()) { > JSScript* script = entry.script(); In ThreadStackHelper.cpp (which is the current code which we use), we can check if the script entry we're looking at is a Chrome script entry (http://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/xpcom/threads/ThreadStackHelper.cpp#277-329). It would be nice to get data like that even when HasPrivacy is set. Right now with this I would get less info about JS stacks then I get from the current system.
Attachment #8886066 -
Flags: review?(michael) → review+
Assignee | ||
Comment 7•7 years ago
|
||
Let me see if I understand... When dealing with JS frames in AddPseudoEntry() -- i.e. when entry.isJS() is true -- do you want the profiler to ignore FeaturePrivacy if IsChromeJSScript() is true?
Flags: needinfo?(michael)
Comment 8•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #7) > Let me see if I understand... When dealing with JS frames in > AddPseudoEntry() -- i.e. when entry.isJS() is true -- do you want the > profiler to ignore FeaturePrivacy if IsChromeJSScript() is true? I think that would be a good solution. We can also add a different feature flag. Right now we capture chrome JS stacks in BHR, and I don't think we want to change that :).
Flags: needinfo?(michael)
Assignee | ||
Comment 9•7 years ago
|
||
This allows code outside the profiler to get fully interleaved stack traces containing frames from the pseudo-stack, native stack, and JS stack.
Attachment #8886896 -
Flags: review?(mstange)
Assignee | ||
Updated•7 years ago
|
Attachment #8886066 -
Attachment is obsolete: true
Comment 10•7 years ago
|
||
Comment on attachment 8886896 [details] [diff] [review] Introduce ProfilerStackCollector Review of attachment 8886896 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Sorry for the delay here. I don't trust myself to give useful feedback on the IsChromeJSScript function, though. ::: tools/profiler/core/platform.cpp @@ +664,2 @@ > { > // WARNING: this function runs within the profiler's "critical section". seems... risky I'd like a JS engine person to review this function.
Attachment #8886896 -
Flags: review?(mstange) → review+
Comment 11•7 years ago
|
||
> I don't trust myself to give useful feedback on the IsChromeJSScript > function, though. > > ::: tools/profiler/core/platform.cpp > @@ +664,2 @@ > > { > > // WARNING: this function runs within the profiler's "critical section". > > seems... risky > > I'd like a JS engine person to review this function. I think that this is probably copied from http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/xpcom/threads/ThreadStackHelper.cpp#218-230 which is already being run inside a critical section, so I think it'll be OK.
Comment 12•7 years ago
|
||
I see, that's fine then.
Assignee | ||
Comment 13•7 years ago
|
||
Yes, and I checked that the function is really simple -- it merely accesses field in a couple of structs.
Assignee | ||
Comment 14•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3923ce220df31c0a1fb589158f1d2a3f40a93aef Bug 1380286 - Introduce ProfilerStackCollector. r=mstange.
Comment 15•7 years ago
|
||
\o/ Thanks for your help getting this working!
Assignee | ||
Comment 16•7 years ago
|
||
You're welcome. Let me know if there are any troubles getting it integrated into the BHR.
Comment 17•7 years ago
|
||
backed out for hazard failures like https://treeherder.mozilla.org/logviewer.html#?job_id=117298399&repo=mozilla-inbound
Flags: needinfo?(n.nethercote)
Comment 18•7 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/18b80915d073 Backed out changeset 3923ce220df3 for hazard failures
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(n.nethercote)
Assignee | ||
Comment 19•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4890e14058f44cf958aa138a03e571f645b96ea6 Bug 1380286 (attempt 2) - Introduce ProfilerStackCollector. r=mstange.
Comment 20•7 years ago
|
||
Pushed by nnethercote@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce87710ff85 (follow-up) - Fix hazard bustage. r=bustage.
Assignee | ||
Comment 21•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce87710ff85998f8afcf821e961fe6ee03554bb Bug 1380286 (follow-up) - Fix hazard bustage. r=bustage.
Comment 22•7 years ago
|
||
Thank you!
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4890e14058f4 https://hg.mozilla.org/mozilla-central/rev/4ce87710ff85
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•