Last Comment Bug 717059 - Use glibc backtrace on profiling builds
: Use glibc backtrace on profiling builds
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
Depends on:
Blocks: 713227
  Show dependency treegraph
 
Reported: 2012-01-10 15:01 PST by Benoit Girard (:BenWa)
Modified: 2012-01-30 05:24 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix Float (4.49 KB, patch)
2012-01-10 15:02 PST, Benoit Girard (:BenWa)
ehsan: review+
Details | Diff | Review
Add optional platform/build specific features (10.48 KB, patch)
2012-01-10 15:03 PST, Benoit Girard (:BenWa)
ehsan: review+
Details | Diff | Review
add 'stackwalk' optional feature for Mac on profiling builds (11.23 KB, patch)
2012-01-10 15:04 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Review
add 'stackwalk' optional feature for Mac/Linux on profiling builds (16.75 KB, patch)
2012-01-12 16:35 PST, Benoit Girard (:BenWa)
ehsan: review+
Details | Diff | Review
add 'stackwalk' optional feature for Mac/Linux on profiling builds - fixed (16.45 KB, patch)
2012-01-12 17:10 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Fix for GetFeatures (958 bytes, patch)
2012-01-14 08:03 PST, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Review
Skip 4 frames on mac (2.05 KB, patch)
2012-01-19 14:00 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Skip 4 frames on mac (1.17 KB, patch)
2012-01-19 14:01 PST, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Review

Description Benoit Girard (:BenWa) 2012-01-10 15:01:04 PST

    
Comment 1 Benoit Girard (:BenWa) 2012-01-10 15:02:16 PST
Created attachment 587496 [details] [diff] [review]
Fix Float
Comment 2 Benoit Girard (:BenWa) 2012-01-10 15:03:09 PST
Created attachment 587497 [details] [diff] [review]
Add optional platform/build specific features
Comment 3 Benoit Girard (:BenWa) 2012-01-10 15:04:08 PST
Created attachment 587500 [details] [diff] [review]
add 'stackwalk' optional feature for Mac on profiling builds
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2012-01-11 12:02:16 PST
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-01-11 12:12:01 PST
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-01-11 12:22:17 PST
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) 2012-01-12 16:35:28 PST
Created attachment 588235 [details] [diff] [review]
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-01-12 16:59:37 PST
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) 2012-01-12 17:10:06 PST
Created attachment 588245 [details] [diff] [review]
add 'stackwalk' optional feature for Mac/Linux on profiling builds - fixed
Comment 11 Benoit Girard (:BenWa) 2012-01-12 17:42:45 PST
Landed a small bustage fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07cfb8019fcd

Forgot XP_UNIX implied ANDROID.
Comment 12 :Ms2ger 2012-01-13 02:42:32 PST
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 14 :Ehsan Akhgari (busy, don't ask for review please) 2012-01-13 09:21:14 PST
(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) 2012-01-14 08:03:15 PST
Created attachment 588636 [details] [diff] [review]
Fix for GetFeatures

This fixes the warning and properly handles zero length.
Comment 16 Nickolay_Ponomarev 2012-01-14 11:10:51 PST
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) 2012-01-15 07:14:41 PST
(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 2012-01-15 09:06:09 PST
Oh, you'll fix that crash with the "Fix for GetFeatures" patch above. Sorry for the noise.
Comment 19 Benoit Girard (:BenWa) 2012-01-16 08:37:38 PST
Reopen until Attachment #588636 [details] [diff] lands
Comment 20 Justin Wood (:Callek) 2012-01-16 19:52:34 PST
https://hg.mozilla.org/mozilla-central/rev/aa8f474988e6 (from m-i merge)
Comment 21 Benoit Girard (:BenWa) 2012-01-19 13:59:41 PST
re-open, output isn't clear without skip frames.
Comment 22 Benoit Girard (:BenWa) 2012-01-19 14:00:34 PST
Created attachment 589982 [details] [diff] [review]
Skip 4 frames on mac
Comment 23 Benoit Girard (:BenWa) 2012-01-19 14:01:40 PST
Created attachment 589984 [details] [diff] [review]
Skip 4 frames on mac
Comment 24 Jeff Muizelaar [:jrmuizel] 2012-01-20 11:50:19 PST
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) 2012-01-20 21:22:20 PST
Comment on attachment 589984 [details] [diff] [review]
Skip 4 frames on mac

Move skip frame logic to the extension

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