The default bug view has changed. See this FAQ.

Enable SPS profiler on Linux

RESOLVED FIXED in mozilla11

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

unspecified
mozilla11
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Patch in a moment.
(Assignee)

Comment 1

5 years ago
Created attachment 577350 [details] [diff] [review]
Patch v1
Attachment #577350 - Flags: review?(bgirard)
(Assignee)

Comment 2

5 years ago
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+
(Assignee)

Comment 4

5 years ago
> 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.
(Assignee)

Comment 5

5 years ago
Created attachment 577602 [details] [diff] [review]
Patch v2

Marking for review mainly to check on the two questions in comment 4.
Attachment #577602 - Flags: review?(bgirard)
(Assignee)

Updated

5 years ago
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)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/039231fd497f
(Assignee)

Updated

5 years ago
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?
(Assignee)

Comment 9

5 years ago
Sorry I've gone dark on this bug.  The b2g workweek plus a 10+hr timechange has been...crazy.
(Assignee)

Comment 10

5 years ago
> Do you need me to look into the test failures?

That would be really helpful, yes.
(Assignee)

Comment 11

5 years ago
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)
(Assignee)

Comment 13

5 years ago
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
(Assignee)

Updated

5 years ago
Whiteboard: [needs landing]
Have you had a chance to test with the add-on?
(Assignee)

Comment 15

5 years ago
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

5 years ago
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
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/956d36a82987
Whiteboard: [needs landing]

Comment 18

5 years ago
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

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(Assignee)

Comment 21

5 years ago
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.
(Assignee)

Comment 23

5 years ago
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

5 years ago
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.