Last Comment Bug 653311 - Jprof doesn't work for many (most?) Linux distros
: Jprof doesn't work for many (most?) Linux distros
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla6
Assigned To: Randell Jesup [:jesup]
:
Mentors:
Depends on:
Blocks: 187053
  Show dependency treegraph
 
Reported: 2011-04-27 17:50 PDT by Randell Jesup [:jesup]
Modified: 2011-05-17 03:29 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to make jprof work on Ubuntu 8.04LTS and general improvements (20.48 KB, patch)
2011-04-28 02:41 PDT, Randell Jesup [:jesup]
jim_nance: review+
Details | Diff | Review
hg export of updated patch (27.55 KB, patch)
2011-04-29 13:07 PDT, Randell Jesup [:jesup]
rjesup: review+
Details | Diff | Review

Description Randell Jesup [:jesup] 2011-04-27 17:50:40 PDT
Jprof seems to have been somewhat left behind by modern GCC/Linux distros - out of the box it fails to save any stacks on Ubuntu 8.04 LTS, and probably a bunch of others.

Largely this is due to the roll-your-own stack backtrace, which is very tied to the ABI, GCC setup and glibc.  It at minimum should use glibc's backtrace() or equivalent.  Even better would have it working under Windows as well using CaptureStackBackTrace() or equivalent.

Related bug: Bug 187053 (Support jprof on Windows)
Comment 1 Randell Jesup [:jesup] 2011-04-28 02:41:45 PDT
Created attachment 528815 [details] [diff] [review]
Patch to make jprof work on Ubuntu 8.04LTS and general improvements

Note: patch includes support for reporting jprof results on a per-thread basis, though under linux by default jprof will only capture the main thread.  There are external preload libraries (made for use with gprof) that could be adapted to make jprof capture all threads, in which case this ability would be useful.

Reviewed with JST Simulacrum - passes cleanly except for fprintf string lengths.

Improved output - includes %ages after all counts, though caller/callee count percentages can be misleading if there's a loop (just as the counts have always been misleading there).  Also includes "(self)" for recursive calls.

The only issue I anticipate might be the #includes of sys/syscall.h and execinfo.h.  Since jprof is currently a linux/glib tool, I don't think this is an issue (shouldn't break anywhere --enable-jprof isn't already broken).
Comment 2 Randell Jesup [:jesup] 2011-04-28 02:48:53 PDT
Comment on attachment 528815 [details] [diff] [review]
Patch to make jprof work on Ubuntu 8.04LTS and general improvements

I probably should add myself to Contributors as well...

Requesting review from dbaron (other reviews welcome)
Comment 3 Randell Jesup [:jesup] 2011-04-28 02:52:41 PDT
Comment on attachment 528815 [details] [diff] [review]
Patch to make jprof work on Ubuntu 8.04LTS and general improvements

Review of attachment 528815 [details] [diff] [review]:

Adding jim nance
Comment 4 Randell Jesup [:jesup] 2011-04-28 09:10:11 PDT
Added bz; made bug 187053 blocked by this though technically it isn't.
Comment 5 Randell Jesup [:jesup] 2011-04-29 05:46:29 PDT
Comment on attachment 528815 [details] [diff] [review]
Patch to make jprof work on Ubuntu 8.04LTS and general improvements

Added r=? for bzbarsky, who has worked in jprof
Comment 6 Randell Jesup [:jesup] 2011-04-29 08:03:42 PDT
While waiting for reviews and my checkin privs to be renewed, I'm working on updating the README.html (and one minor tweak to the JP_PERIOD code to allow "0.001").  New patch to be uploaded RSN.  Deltas from this patch to code should be minimal, so looking at this patch won't be wasted work.
Comment 7 Boris Zbarsky [:bz] 2011-04-29 09:41:19 PDT
I'm not really competent to review this (nor do I think it needs a second reviewer).
Comment 8 Randell Jesup [:jesup] 2011-04-29 11:21:36 PDT
Ok - I'll get a quick refresh of the r= when I update the patch then, probably from Jim Nance (original author).  Thanks bz
Comment 9 Randell Jesup [:jesup] 2011-04-29 13:07:03 PDT
Created attachment 529180 [details] [diff] [review]
hg export of updated patch

Ok, I think this is done now.  Final review?
Comment 10 jim_nance 2011-05-02 07:09:46 PDT
I'm not sure I have the power to give you the review, but it looks good to me.

Thanks for working on this.
Comment 11 Randell Jesup [:jesup] 2011-05-02 20:15:47 PDT
Comment on attachment 529180 [details] [diff] [review]
hg export of updated patch

Marking r=+ for Jim Nance
Comment 12 Randell Jesup [:jesup] 2011-05-02 20:17:46 PDT
I'll commit this as soon as I have commit access again (and the tree is open, etc).
Comment 13 Randell Jesup [:jesup] 2011-05-16 21:17:04 PDT
Checked in as changeset 69590:9968ed6b629a
Comment 14 :Ms2ger 2011-05-17 03:29:42 PDT
FWIW, changeset links like this:

http://hg.mozilla.org/mozilla-central/rev/9968ed6b629a

are encouraged.

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