Last Comment Bug 705856 - Enable SPS profiler on Linux
: Enable SPS profiler on Linux
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: mozilla11
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
Depends on: 683229
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-28 13:27 PST by Justin Lebar (not reading bugmail)
Modified: 2011-12-16 15:56 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.56 KB, patch)
2011-11-28 13:28 PST, Justin Lebar (not reading bugmail)
b56girard: review+
Details | Diff | Splinter Review
Patch v2 (4.13 KB, patch)
2011-11-29 07:08 PST, Justin Lebar (not reading bugmail)
b56girard: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2011-11-28 13:27:20 PST
Patch in a moment.
Comment 1 Justin Lebar (not reading bugmail) 2011-11-28 13:28:23 PST
Created attachment 577350 [details] [diff] [review]
Patch v1
Comment 2 Justin Lebar (not reading bugmail) 2011-11-28 13:28:38 PST
Are we upstreaming changes to sps, or just checking them in?
Comment 3 Benoit Girard (:BenWa) 2011-11-28 13:36:01 PST
Comment on attachment 577350 [details] [diff] [review]
Patch v1

Review of attachment 577350 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/Makefile.in
@@ +76,5 @@
>  EXTRA_JS_MODULES = \
>    Profiler.jsm \
>    $(NULL)
>  
> +ifneq (,$(filter Android Linux,$(OS_TARGET)))

This is confusing. There must be a better way. In any case we should have ports for all platforms soon so I don't care much about temporary code.

::: tools/profiler/sps/sps_sampler.h
@@ -59,4 +60,4 @@
> >  //      memory stores from being reordered
> >  // Uses: pLinuxKernelMemoryBarrier
> >  # define STORE_SEQUENCER() base::subtle::MemoryBarrier();
> > -#elif ARCH_CPU_ARM_FAMILY
> > +#elif __GNUC__

This was meant to be 'ARCH_CPU_X86_FAMILY'. Other ports have made this fix so we should change it to the same thing here.

r+ with this fixed.
Comment 4 Justin Lebar (not reading bugmail) 2011-11-28 13:40:54 PST
> Are we upstreaming changes to sps, or just checking them in?

?

> This is confusing. There must be a better way.

Heh.  There is not that I'm aware of.  make is the devil.

> This was meant to be 'ARCH_CPU_X86_FAMILY'. Other ports have made this fix so we should change it 
> to the same thing here.

Still need to check for GCC, right?  MSVC doesn't use the same asm syntax.
Comment 5 Justin Lebar (not reading bugmail) 2011-11-29 07:08:52 PST
Created attachment 577602 [details] [diff] [review]
Patch v2

Marking for review mainly to check on the two questions in comment 4.
Comment 6 Benoit Girard (:BenWa) 2011-11-29 08:06:32 PST
Comment on attachment 577602 [details] [diff] [review]
Patch v2

Review of attachment 577602 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/sps/sps_sampler.h
@@ -59,4 +60,4 @@
> >  //      memory stores from being reordered
> >  // Uses: pLinuxKernelMemoryBarrier
> >  # define STORE_SEQUENCER() base::subtle::MemoryBarrier();
> > -#elif ARCH_CPU_ARM_FAMILY
> > +#elif defined(__GNUC__) && defined(ARCH_CPU_X86_FAMILY)

You're right but it doesn't matter to much since the window port already has a more general fix:

See:
https://github.com/ehsan/mozilla-central/blob/spswindows/tools/profiler/sps/sps_sampler.h

Ehsan has already seen this patch and he expects to merge conflicts so I wouldn't worry.
Comment 7 Justin Lebar (not reading bugmail) 2011-12-01 11:55:40 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/039231fd497f
Comment 8 Benoit Girard (:BenWa) 2011-12-05 07:07:32 PST
If you base this on top of bug 699918 it should make it easy to test this change. The windows port is nearly ready. Do you need me to look into the test failures?
Comment 9 Justin Lebar (not reading bugmail) 2011-12-06 01:27:29 PST
Sorry I've gone dark on this bug.  The b2g workweek plus a 10+hr timechange has been...crazy.
Comment 10 Justin Lebar (not reading bugmail) 2011-12-06 18:50:05 PST
> Do you need me to look into the test failures?

That would be really helpful, yes.
Comment 11 Justin Lebar (not reading bugmail) 2011-12-12 12:58:24 PST
I'm looking at this again.
Comment 12 Benoit Girard (:BenWa) 2011-12-12 13:58:28 PST
I didn't have a chance to work on the Linux. We added a feature to let the firefox add-on do the symbolication in Mac OS X.

You should test your changes with the add-on here:
https://builder.addons.mozilla.org/addon/1023834/latest/

I might need to remove some Mac specific stuff when you need to test so just send me a ping (or fork the addon)
Comment 13 Justin Lebar (not reading bugmail) 2011-12-12 18:27:26 PST
I pushed this to try, and it's green on Linux opt and debug.  Dunno what's different.

I guess I'll push to m-i once the tree reopens.

https://tbpl.mozilla.org/?tree=Try&rev=d27d2a9066d6
Comment 14 Benoit Girard (:BenWa) 2011-12-13 08:37:23 PST
Have you had a chance to test with the add-on?
Comment 15 Justin Lebar (not reading bugmail) 2011-12-13 11:28:31 PST
I just tried the add-on.  I'm not sure what it's supposed to do, but when I click "analyze" after "start", the Cleopatra UI is empty, save for a few scroll bars.
Comment 16 Mozilla RelEng Bot 2011-12-15 12:50:50 PST
Try run for 632ee7e8cb35 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=632ee7e8cb35
Results (out of 163 total builds):
    exception: 1
    success: 133
    warnings: 25
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-632ee7e8cb35
Comment 17 Justin Lebar (not reading bugmail) 2011-12-15 13:46:15 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/956d36a82987
Comment 18 Mozilla RelEng Bot 2011-12-15 14:01:06 PST
Try run for 632ee7e8cb35 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=632ee7e8cb35
Results (out of 163 total builds):
    exception: 1
    success: 133
    warnings: 25
    failure: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-632ee7e8cb35
Comment 19 Mozilla RelEng Bot 2011-12-15 14:50:36 PST
Try run for 28efc5d6564c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=28efc5d6564c
Results (out of 207 total builds):
    success: 183
    warnings: 24
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-28efc5d6564c
Comment 20 :Ms2ger (⌚ UTC+1/+2) 2011-12-16 05:40:01 PST
https://hg.mozilla.org/mozilla-central/rev/956d36a82987
Comment 21 Justin Lebar (not reading bugmail) 2011-12-16 08:22:07 PST
Sorry this took so long to get in, Benoit!
Comment 22 Benoit Girard (:BenWa) 2011-12-16 08:29:44 PST
Not a problem.

I assume that glibc backtrace() works in Linux with a profiler (frame pointer) build correct? I landed support for programmatic event tracing for the profiler but ted tells me it's been disabled on Linux. Not having event tracing on Linux will limit the usefulness on this for Linux.
Comment 23 Justin Lebar (not reading bugmail) 2011-12-16 08:32:29 PST
I'm pretty sure I used libc's backtrace() successfully with a --enable-profiling (i.e., -fno-omit-frame-pointer) build on Linux x86-64.
Comment 24 Mozilla RelEng Bot 2011-12-16 15:46:46 PST
Try run for 28efc5d6564c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=28efc5d6564c
Results (out of 207 total builds):
    success: 183
    warnings: 24
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-28efc5d6564c
Comment 25 Lukas Blakk [:lsblakk] use ?needinfo 2011-12-16 15:56:22 PST
(In reply to Mozilla RelEng Bot from comment #24)
sorry for the double bot post - slight differences in string caused this.

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