Closed Bug 1380286 Opened 2 years ago Closed 2 years ago

Introduce ProfilerStackCollector

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(1 file, 2 obsolete files)

This allows code outside the profiler to get fully interleaved stack traces
containing frames from the pseudo-stack, native stack, and JS stack.
Attached patch Introduce ProfilerStackCollector (obsolete) — Splinter Review
(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.
Attachment #8885667 - Flags: feedback?(michael)
Blocks: 1367406
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+
> > +  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.
Attached patch Introduce ProfilerStackCollector (obsolete) — Splinter Review
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)
Attachment #8885667 - Attachment is obsolete: true
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+
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)
(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)
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)
Attachment #8886066 - Attachment is obsolete: true
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+
> 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.
I see, that's fine then.
Yes, and I checked that the function is really simple -- it merely accesses field in a couple of structs.
\o/ Thanks for your help getting this working!
You're welcome. Let me know if there are any troubles getting it integrated into the BHR.
backed out for hazard failures like https://treeherder.mozilla.org/logviewer.html#?job_id=117298399&repo=mozilla-inbound
Flags: needinfo?(n.nethercote)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18b80915d073
Backed out changeset 3923ce220df3 for hazard failures
Flags: needinfo?(n.nethercote)
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce87710ff85
(follow-up) - Fix hazard bustage. r=bustage.
Thank you!
https://hg.mozilla.org/mozilla-central/rev/4890e14058f4
https://hg.mozilla.org/mozilla-central/rev/4ce87710ff85
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1384766
Depends on: 1428072
You need to log in before you can comment on or make changes to this bug.