The default bug view has changed. See this FAQ.

Use glibc backtrace on profiling builds

RESOLVED FIXED in mozilla12

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

Trunk
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 587496 [details] [diff] [review]
Fix Float
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #587496 - Flags: review?(jmuizelaar)
(Assignee)

Comment 2

5 years ago
Created attachment 587497 [details] [diff] [review]
Add optional platform/build specific features
Attachment #587497 - Flags: review?(jmuizelaar)
(Assignee)

Comment 3

5 years ago
Created attachment 587500 [details] [diff] [review]
add 'stackwalk' optional feature for Mac on profiling builds
Attachment #587500 - Flags: review?(jmuizelaar)
(Assignee)

Updated

5 years ago
Attachment #587496 - Flags: review?(jmuizelaar) → review?(ehsan)
(Assignee)

Updated

5 years ago
Attachment #587497 - Flags: review?(jmuizelaar) → review?(ehsan)
(Assignee)

Updated

5 years ago
Attachment #587500 - Flags: review?(jmuizelaar) → review?(ehsan)
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.
Attachment #587496 - Flags: review?(ehsan) → review+
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.
Attachment #587497 - Flags: review?(ehsan) → review+
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?
Attachment #587500 - Flags: review?(ehsan)
(Assignee)

Comment 7

5 years ago
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.
Attachment #587500 - Attachment is obsolete: true
Attachment #588235 - Flags: review?(ehsan)
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.
Attachment #588235 - Flags: review?(ehsan) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 588245 [details] [diff] [review]
add 'stackwalk' optional feature for Mac/Linux on profiling builds - fixed
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac78d7a4e819
(Assignee)

Comment 11

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

Forgot XP_UNIX implied ANDROID.
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.
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(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.
(Assignee)

Comment 15

5 years ago
Created attachment 588636 [details] [diff] [review]
Fix for GetFeatures

This fixes the warning and properly handles zero length.
Attachment #588636 - Flags: review?(ehsan)

Comment 16

5 years ago
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
(Assignee)

Comment 17

5 years ago
(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

5 years ago
Oh, you'll fix that crash with the "Fix for GetFeatures" patch above. Sorry for the noise.
(Assignee)

Updated

5 years ago
Attachment #588636 - Flags: review?(ehsan) → review?(jmuizelaar)
Attachment #588636 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 19

5 years ago
Reopen until Attachment #588636 [details] [diff] lands
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/aa8f474988e6 (from m-i merge)
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

5 years ago
re-open, output isn't clear without skip frames.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 22

5 years ago
Created attachment 589982 [details] [diff] [review]
Skip 4 frames on mac
Attachment #589982 - Flags: review?(jmuizelaar)
(Assignee)

Comment 23

5 years ago
Created attachment 589984 [details] [diff] [review]
Skip 4 frames on mac
Attachment #589982 - Attachment is obsolete: true
Attachment #589982 - Flags: review?(jmuizelaar)
Attachment #589984 - Flags: review?(jmuizelaar)
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?
Attachment #589984 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

5 years ago
Assignee: bgirard → nobody
Component: General → Gecko Profiler
QA Contact: general → gecko-profiler
(Assignee)

Comment 25

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

Move skip frame logic to the extension
Attachment #589984 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Assignee: nobody → bgirard
You need to log in before you can comment on or make changes to this bug.