Closed
Bug 717059
Opened 11 years ago
Closed 11 years ago
Use glibc backtrace on profiling builds
Categories
(Core :: Gecko Profiler, defect)
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: BenWa, Assigned: BenWa)
References
Details
Attachments
(5 files, 3 obsolete files)
4.49 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
10.48 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
16.75 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
16.45 KB,
patch
|
Details | Diff | Splinter Review | |
958 bytes,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #587497 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #587500 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•11 years ago
|
Attachment #587496 -
Flags: review?(jmuizelaar) → review?(ehsan)
Assignee | ||
Updated•11 years ago
|
Attachment #587497 -
Flags: review?(jmuizelaar) → review?(ehsan)
Assignee | ||
Updated•11 years ago
|
Attachment #587500 -
Flags: review?(jmuizelaar) → review?(ehsan)
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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 6•11 years ago
|
||
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•11 years ago
|
||
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 8•11 years ago
|
||
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•11 years ago
|
||
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac78d7a4e819
Assignee | ||
Comment 11•11 years ago
|
||
Landed a small bustage fixed: https://hg.mozilla.org/integration/mozilla-inbound/rev/07cfb8019fcd Forgot XP_UNIX implied ANDROID.
Comment 12•11 years ago
|
||
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•11 years ago
|
||
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Comment 14•11 years ago
|
||
(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•11 years ago
|
||
This fixes the warning and properly handles zero length.
Attachment #588636 -
Flags: review?(ehsan)
Comment 16•11 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•11 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•11 years ago
|
||
Oh, you'll fix that crash with the "Fix for GetFeatures" patch above. Sorry for the noise.
Assignee | ||
Updated•11 years ago
|
Attachment #588636 -
Flags: review?(ehsan) → review?(jmuizelaar)
Updated•11 years ago
|
Attachment #588636 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 19•11 years ago
|
||
Reopen until Attachment #588636 [details] [diff] lands
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aa8f474988e6 (from m-i merge)
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•11 years ago
|
||
re-open, output isn't clear without skip frames.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #589982 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 23•11 years ago
|
||
Attachment #589982 -
Attachment is obsolete: true
Attachment #589982 -
Flags: review?(jmuizelaar)
Attachment #589984 -
Flags: review?(jmuizelaar)
Comment 24•11 years ago
|
||
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•11 years ago
|
Assignee: bgirard → nobody
Component: General → Gecko Profiler
QA Contact: general → gecko-profiler
Assignee | ||
Comment 25•11 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•11 years ago
|
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Assignee: nobody → bgirard
You need to log in
before you can comment on or make changes to this bug.
Description
•