Closed Bug 705856 Opened 8 years ago Closed 8 years ago

Enable SPS profiler on Linux

Categories

(Core :: General, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(1 file, 1 obsolete file)

Patch in a moment.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #577350 - Flags: review?(bgirard)
Are we upstreaming changes to sps, or just checking them in?
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.
Attachment #577350 - Flags: review?(bgirard) → review+
> 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.
Attached patch Patch v2Splinter Review
Marking for review mainly to check on the two questions in comment 4.
Attachment #577602 - Flags: review?(bgirard)
Attachment #577350 - Attachment is obsolete: true
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.
Attachment #577602 - Flags: review?(bgirard) → review+
Assignee: nobody → justin.lebar+bug
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?
Sorry I've gone dark on this bug.  The b2g workweek plus a 10+hr timechange has been...crazy.
> Do you need me to look into the test failures?

That would be really helpful, yes.
I'm looking at this again.
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)
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
Whiteboard: [needs landing]
Have you had a chance to test with the add-on?
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.
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
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
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
https://hg.mozilla.org/mozilla-central/rev/956d36a82987
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Sorry this took so long to get in, Benoit!
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.
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.
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
(In reply to Mozilla RelEng Bot from comment #24)
sorry for the double bot post - slight differences in string caused this.
You need to log in before you can comment on or make changes to this bug.