717059 - Use glibc backtrace on profiling builds

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Fix Float

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Add optional platform/build specific features

Comment 3

[Benoit Girard (:BenWa)]

Attachment: add 'stackwalk' optional feature for Mac on profiling builds

Comment 4

[(no longer active)]

Comment on attachment 587496 [details] [diff] [review]
Fix Float

Review of attachment 587496 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/public/nsIProfiler.idl
@@ +41,5 @@
>    void StartProfiler(in PRUint32 aInterval, in PRUint32 aEntries);
>    void StopProfiler();
>    string GetProfile();
>    boolean IsActive();
> +  void GetResponsivenessTimes(out PRUint32 aCount, [retval, array, size_is(aCount)] out double aResult);

Please rev the uuid.

Comment 5

[(no longer active)]

Comment on attachment 587497 [details] [diff] [review]
Add optional platform/build specific features

Review of attachment 587497 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/public/nsIProfiler.idl
@@ +44,5 @@
>    void StopProfiler();
>    string GetProfile();
>    boolean IsActive();
>    void GetResponsivenessTimes(out PRUint32 aCount, [retval, array, size_is(aCount)] out double aResult);
> +  void GetFeatures(out PRUint32 aCount, [retval, array, size_is(aCount)] out string aFeatures);

You need to rev the uuid, but you can do that once for this bug as a whole.

::: tools/profiler/sps/TableTicker.cpp
@@ +229,4 @@
>  class TableTicker: public Sampler {
>   public:
> +  explicit TableTicker(int aInterval, int aEntrySize, Stack *aStack,
> +                       const char** aFeatures, uint32_t aFeatureCount)

Nit: drop 'explicit'.

@@ +235,5 @@
>      , mStack(aStack)
>      , mSaveRequested(false)
>    {
> +    mUseNSStackWalk = hasFeature(aFeatures, aFeatureCount, "nsstackwalk");
> +    mUseNSDescribe = hasFeature(aFeatures, aFeatureCount, "nsdescribe");

I think it's a bit premature to decide whether we want to use NS_DescribeCodeAddress or not.  Note that at least on Windows, it has a performance cost which is probably not acceptable in a profile.  I suggest dropping mUseNSDescribe for now.

Comment 6

[(no longer active)]

Comment on attachment 587500 [details] [diff] [review]
add 'stackwalk' optional feature for Mac on profiling builds

Review of attachment 587500 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/Makefile.in
@@ +93,5 @@
>  DEFINES += -DMOZ_ENABLE_PROFILER_SPS
>  
>  CPPSRCS += \
>    shared-libraries-macos.cc \
> +  platform-linux.cc \

What is the purpose of this change?  We used to build platform-macos.cc but not platform-linux.cc, and now we're doing the opposite?

::: tools/profiler/sps/TableTicker.cpp
@@ +142,1 @@
>    };

Hmm, I find this setup questionable.  How are we supposed to know which member of the union is valid?  Note that these fields are not the same size on x86.

@@ +152,4 @@
>      : mWritePos(0)
>      , mReadPos(0)
>      , mEntrySize(aEntrySize)
> +    , mUseNSDescribe(aUseNSDescribe)

Like I said, I'd drop this.  If you do, please make the ctor explicit as well.

@@ +371,5 @@
> +static
> +void doNSBackTrace(Profile &aProfile)
> +{
> +  aProfile.addTag(ProfileEntry('s', "XRE_Main", 0));
> +  NS_StackWalk(stack_callback, 2, &aProfile, 0);

Why are you skipping two frames?

@@ +442,5 @@
> +
> +          if (aUseNSDescribe) {
> +            nsCodeAddressDetails details;
> +            nsresult rv = NS_DescribeCodeAddress(pc, &details);
> +            printf("Describe\n");

Do we need this printf?

@@ +448,5 @@
> +              snprintf(tagBuff, 1024, "l-%s\n", details.function);
> +              tag += string(tagBuff);
> +              break;
> +            }
> +          }

How much perf impact does this code have?

Comment 7

[Benoit Girard (:BenWa)]

Attachment: add 'stackwalk' optional feature for Mac/Linux on profiling builds

This work except well. We get crash on Linux when trying to backtrace within the OpenGL driver. We should wait until we find a way to resolve these crashes before turning on this feature on Linux.

Comment 8

[(no longer active)]

Comment on attachment 588235 [details] [diff] [review]
add 'stackwalk' optional feature for Mac/Linux on profiling builds

Review of attachment 588235 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: tools/profiler/Makefile.in
@@ +91,5 @@
>  ifeq ($(OS_TARGET),Darwin)
> +# For now we use platform-linux.cc because we can't unwind
> +# another thread on mac using backtrace(), the implementation
> +# for platform-macosx.cc is in the hg history and should be
> +# used when we can stackwalk using a thread handle.

\o/

::: tools/profiler/sps/TableTicker.cpp
@@ +340,5 @@
>    int count = backtrace (array, 100);
>  
> +  aProfile.addTag(ProfileEntry('s', "XRE_Main", 0));
> +
> +  for (int i = 0; i < count; i++) {

This will give you reverse stacks.  Please file a followup to fix it, and CC me.  Thanks!

@@ +352,5 @@
> +void stack_callback(void *aPc, void *aClosure)
> +{
> +  Profile* profile = static_cast<Profile*>(aClosure);
> +  profile->addTag(ProfileEntry('l', (uintptr_t)aPc));
> +}

Please remove this function.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: add 'stackwalk' optional feature for Mac/Linux on profiling builds - fixed

Comment 10

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac78d7a4e819

Comment 11

[Benoit Girard (:BenWa)]

Landed a small bustage fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07cfb8019fcd

Forgot XP_UNIX implied ANDROID.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 587497 [details] [diff] [review]
Add optional platform/build specific features

>+nsProfiler::GetFeatures(PRUint32 *aCount, char ***aFeatures)
>+{
>+  while (features[++len]);

This will cause build warnings, please make it

while (features[++len]) {
  continue;
}

or even increment len in the body.

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2c1f1e307171
https://hg.mozilla.org/mozilla-central/rev/014f28a0543c
https://hg.mozilla.org/mozilla-central/rev/ac78d7a4e819
https://hg.mozilla.org/mozilla-central/rev/07cfb8019fcd

Comment 14

[(no longer active)]

(In reply to Ms2ger from comment #12)
> Comment on attachment 587497 [details] [diff] [review]
> Add optional platform/build specific features
> 
> >+nsProfiler::GetFeatures(PRUint32 *aCount, char ***aFeatures)
> >+{
> >+  while (features[++len]);
> 
> This will cause build warnings, please make it
> 
> while (features[++len]) {
>   continue;
> }
> 
> or even increment len in the body.

Please file a follow-up bug.

Comment 15

[Benoit Girard (:BenWa)]

Attachment: Fix for GetFeatures

This fixes the warning and properly handles zero length.

Comment 16

[Nickolay_Ponomarev]

FWIW, the nightly with this change crashes on me with the profiling add-on: https://crash-stats.mozilla.com/report/index/bp-d0c98cb8-11bb-4cde-b1c2-8048b2120114

Comment 17

[Benoit Girard (:BenWa)]

(In reply to Nickolay_Ponomarev from comment #16)
> FWIW, the nightly with this change crashes on me with the profiling add-on:
> https://crash-stats.mozilla.com/report/index/bp-d0c98cb8-11bb-4cde-b1c2-
> 8048b2120114

I've fixed the extension to not use the stack walking feature. It wont crash for the anymore. We still need a bit of time to stabilize stack walking on all platforms.

Comment 18

[Nickolay_Ponomarev]

Oh, you'll fix that crash with the "Fix for GetFeatures" patch above. Sorry for the noise.

Comment 19

[Benoit Girard (:BenWa)]

Reopen until Attachment #588636 [details] [diff] lands

Comment 20

[Justin Wood (:Callek)]

https://hg.mozilla.org/mozilla-central/rev/aa8f474988e6 (from m-i merge)

Comment 21

[Benoit Girard (:BenWa)]

re-open, output isn't clear without skip frames.

Comment 22

[Benoit Girard (:BenWa)]

Attachment: Skip 4 frames on mac

Comment 23

[Benoit Girard (:BenWa)]

Attachment: Skip 4 frames on mac

Comment 24

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 589984 [details] [diff] [review]
Skip 4 frames on mac

Review of attachment 589984 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/sps/TableTicker.cpp
@@ +347,4 @@
>  
> +  int skip_frames = 0;
> +#ifdef XP_MACOSX
> +  skip_frames = 4;

It would be nice if you could add more details about where the 4 comes from. Could it change when we have different have different inlining decisions.

Perhaps you could use the addresses of the functions you expect on the stack to make this more accurate?

Comment 25

[Benoit Girard (:BenWa)]

Comment on attachment 589984 [details] [diff] [review]
Skip 4 frames on mac

Move skip frame logic to the extension