Closed Bug 717059 Opened 8 years ago Closed 8 years ago

Use glibc backtrace on profiling builds

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(5 files, 3 obsolete files)

No description provided.
Attached patch Fix FloatSplinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #587496 - Flags: review?(jmuizelaar)
Attachment #587497 - Flags: review?(jmuizelaar)
Attachment #587500 - Flags: review?(jmuizelaar)
Attachment #587496 - Flags: review?(jmuizelaar) → review?(ehsan)
Attachment #587497 - Flags: review?(jmuizelaar) → review?(ehsan)
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)
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+
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.
(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.
This fixes the warning and properly handles zero length.
Attachment #588636 - Flags: review?(ehsan)
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
(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.
Oh, you'll fix that crash with the "Fix for GetFeatures" patch above. Sorry for the noise.
Attachment #588636 - Flags: review?(ehsan) → review?(jmuizelaar)
Attachment #588636 - Flags: review?(jmuizelaar) → review+
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
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
re-open, output isn't clear without skip frames.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Skip 4 frames on mac (obsolete) — Splinter Review
Attachment #589982 - Flags: review?(jmuizelaar)
Attached patch Skip 4 frames on mac (obsolete) — Splinter Review
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: bgirard → nobody
Component: General → Gecko Profiler
QA Contact: general → gecko-profiler
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
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Assignee: nobody → bgirard
You need to log in before you can comment on or make changes to this bug.