Add --enable-profiling configure option

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
7 years ago
7 years ago

People

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

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
perf and shark both like having frame pointers.  But we don't like frame pointers because they make our code slow.

We should be able to set a flag in configure which says "give me what I need in order to use a sampling profiler", which in practice means -g and -fno-disable-frame-pointer, I think.

FWIW, the following mozconfig is not sufficient:

 export CFLAGS="-fno-omit-frame-pointer"
 export CXXFLAGS="-fno-omit-frame-pointer"
 ac_add_options --enable-debug_symbols='-fno-omit-frame-pointer'

as places (and probably js and other modules) don't pick up these flags.
We already have options like --enable-shark, --enable-jprof, etc. that we should piggy back on.

Additionally, AIUI this is only relevant for x86.  x86-64 doesn't have frame pointers in this sense (and I doubt anyone profiles firefox on any architectures besides x86, x86-64, and ARM).
(Assignee)

Comment 2

7 years ago
I'm on Linux x64 and I needed -fno-omit-frame-pointer.
Really? x86-64 profiling tools can't deal without a frame pointer? (Which is the default specified in the ABI!)

Also, you could do something like --enable-optimize="-O2", which globally overrides optimization flags.
Ideally, profiling would use flags as close as possible to what we actually build as (which I think is mostly -O3 on Linux/Mac now).
All you should have to do is add logic to test for MOZ_ENABLE_PROFILING (or whatever) at http://mxr.mozilla.org/mozilla-central/source/configure.in#2037 http://mxr.mozilla.org/mozilla-central/source/configure.in#2196 http://mxr.mozilla.org/mozilla-central/source/configure.in#2830 and add a check to compile with -Oy- on MSVC.
(Assignee)

Comment 6

7 years ago
(In reply to comment #1)
> We already have options like --enable-shark, --enable-jprof, etc. that we
> should piggy back on.

We could make --enable-shark imply --enable-profiling.  I dunno whether that's necessary for --enable-jprof (which is a Java profiler??).  I'd personally be OK without adding the implies relation so that Shark users would be aware that they can build without --enable-shark (which adds other stuff they may or may not want) and still profile.
(Assignee)

Comment 7

7 years ago
Also, are we sure that MOZ_OPTIMIZE_FLAGS is the right variable to set?

In particular, it looks like MODULE_OPTIMIZE_FLAGS overrides MOZ_OPTIMIZE_FLAGS (at least, js/src is still compiling with -fomit-frame-pointer, which I assume comes from MODULE_OPTIMIZE_FLAGS in js/src/Makefile.in, but js/src/shell is compiled with -fno-omit-frame-pointer as I specified in js/src/configure.in).
Yes. MOZ_OPTIMIZE_FLAGS is the global set of optimization flags. MODULE_OPTIMIZE_FLAGS are allowed to override it. It's sort of unfortunate, but it is what it is. --enable-optimize=foo overrides MODULE_OPTIMIZE_FLAGS, as well.
(Assignee)

Comment 9

7 years ago
Well, it looks like js/src is the only place that overrides it to add -fomit-frame-pointer.  I'll just special case it in there.
(Assignee)

Comment 10

7 years ago
Would anyone object if I fixed bug 593116 while I'm hacking around in js/src/Makefile.in?
(Assignee)

Comment 11

7 years ago
Created attachment 471596 [details] [diff] [review]
Patch v1

Tested only on Linux 64. I make's output and verified that -fomit-frame-pointer doesn't appear anywhere.

I'll spin a mac build now...
(Assignee)

Comment 12

7 years ago
Actually, scratch that -- the mac I though I had is in use.
LGTM except that there's no reason to ACDEFINE MOZ_PROFILING unless you need to use that from c++ somewhere.
(Assignee)

Comment 14

7 years ago
(In reply to comment #13)
> LGTM except that there's no reason to ACDEFINE MOZ_PROFILING unless you need to
> use that from c++ somewhere.

I figured I'd AC_DEFINE it since MOZ_SHARK et al. are AC_DEFINED.  But I don't care either way.
(Assignee)

Updated

7 years ago
Attachment #471596 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 15

7 years ago
Created attachment 474143 [details] [diff] [review]
Patch v2

Rebased atop patches in bug 590181 and bug 593116.
Attachment #471596 - Attachment is obsolete: true
Attachment #474143 - Flags: review?(ted.mielczarek)
Attachment #471596 - Flags: review?(ted.mielczarek)
Are you using this outside of configure at all?  If not, it doesn't need to be AC_DEFINEd or AC_SUBSTd.  Also, please make --enable-shark, etc set MOZ_PROFILING appropriately.
(Assignee)

Comment 17

7 years ago
(In reply to comment #16)
> Are you using this outside of configure at all?  If not, it doesn't need to be
> AC_DEFINEd or AC_SUBSTd.  Also, please make --enable-shark, etc set
> MOZ_PROFILING appropriately.

Like I said in comment 6, I kind of like the idea of having --enable-shark be separate from --enable-profiling because the two are independent.

You can use shark without --enable-profiling -- you just won't get caller information, which may or may not bother you.  And you can use --enable-shark without --enable-profiling if you want the shark-specific hooks added to the js engine but don't care about getting caller info.

That said, I don't use shark, so I don't care either way.  :)  Also, I think --enable-shark may have been broken by an OSX update, which removed a necessary header.

I'll remove the AC_SUBST and AC_DEFINEs.
> you just won't get caller information, which may or may not bother you.

I can't think of a circumstance where it won't bother you...  I think --enable-shark should imply --enable-profiling; its point is to produce builds that people can use with minimal effort to profile in shark.

As for the 10.6 shark thing, that would be covered by bug 595748.
(Assignee)

Comment 19

7 years ago
Created attachment 475608 [details] [diff] [review]
Patch v3

Untested.  I also have no idea jprof and vtune need frame pointers.
Attachment #474143 - Attachment is obsolete: true
Attachment #475608 - Flags: review?(ted.mielczarek)
Attachment #474143 - Flags: review?(ted.mielczarek)
Is it worth it to make --enable-profiling imply -g at least on Mac/Linux too?
I have a very weird bug report to make against this patch.

The linux 'perf' profiler is completely unable to pick up symbol names from firefox, even when built with the --enable-profiling provided by this patch.

[bjacob@cahouette 50fish]$ perf report -g flat --sort dso,symbol | head -30 
# Samples: 19362611696
#
# Overhead      Shared Object  Symbol
# ........  .................  ......
#
    97.47%       7f8da3c75643  [.] 0x007f8da3c75643
     0.43%  [kernel]           [k] hpet_next_event
     0.08%  [kernel]           [k] fget_light
     0.08%  [kernel]           [k] do_sys_poll
     0.06%  [kernel]           [k] schedule
     0.06%  [kernel]           [k] perf_ctx_adjust_freq
     0.06%  [kernel]           [k] task_of
     0.05%  [kernel]           [k] unix_poll
     0.05%  [kernel]           [k] audit_syscall_entry
     0.05%  [kernel]           [k] hrtimer_interrupt
     0.05%  [kernel]           [k] select_task_rq_fair
     0.05%  [kernel]           [k] _raw_spin_lock
     0.04%  [kernel]           [k] scm_send
     0.04%  [kernel]           [k] system_call
     0.04%  [kernel]           [k] copy_user_generic_string
     0.04%  [kernel]           [k] irq_entries_start
     0.03%  [kernel]           [k] native_apic_mem_write
     0.03%  [kernel]           [k] find_busiest_group
     0.03%  [kernel]           [k] x86_pmu_read
     0.03%  [kernel]           [k] dequeue_task
     0.03%  [kernel]           [k] perf_disable
     0.03%  [kernel]           [k] __rcu_process_callbacks
     0.03%  [kernel]           [k] rb_next
     0.03%  [kernel]           [k] pick_next_task_fair
     0.03%  [kernel]           [k] rb_insert_color


Details:

I have modified this patch so it applies against m-c and added -g everywhere it adds -fno-omit-frame-pointers.

My MOZCONFIG is:
ac_add_options --enable-application=browser
mk_add_options MOZ_CO_PROJECT=browser
ac_add_options --disable-tests
ac_add_options --enable-profiling

I'm on linux 2.6.33, x86-64, GCC 4.4
It turns out that 'make' is still passing -fomit-frame-pointer instead of -fomit-frame-pointer. Yet configure did report that it picked up my --enable-profiling. Do I need to nuke my objdir??
(Assignee)

Comment 23

7 years ago
In reply to comment #20)
> Is it worth it to make --enable-profiling imply -g at least on Mac/Linux too?

Yes, I think so, if that's not already implied.

(In reply to comment #22)
> Do I need to nuke my objdir??

Can you try and report back if it worked?  I'm not sure how the build magic works here.

Note that a clobber build should be no slower than a depend build, since you have to recompile everything to add -fno-omit-frame-pointer.
Created attachment 477130 [details] [diff] [review]
patch with linux fix

The patch had a bug in the linux path, it gave --enable-profiling the opposite meaning.
By the way Justin: what's with these lines?

        GCC_VERSION=`$CC -v 2>&1 | awk '/^gcc version/ { print $3 }'`
        case $GCC_VERSION in
        4.1.*|4.2.*|4.5.*)
            # -Os is broken on gcc 4.1.x 4.2.x, 4.5.x we need to tweak it to get good results.
            MOZ_OPTIMIZE_SIZE_TWEAK="-finline-limit=50"
        esac

I'm asking you because I see that you actually tried to remove them, but brought them back in a back-out.

If -Os is really buggy in GCC, this needs a bug report, as this is something where a bug report with a good pre-processed-source attachment (run with -save-temps, see http://gcc.gnu.org/bugs/#detailed ) would presumably be handled swiftly by the GCC guys.
(In reply to comment #24)
> Created attachment 477130 [details] [diff] [review]
> patch with linux fix
> 
> The patch had a bug in the linux path, it gave --enable-profiling the opposite
> meaning.

It's still not working here, despite the compilation command lines now looking fine :-(

I even added -g, no luck

For reference, the command line for a single cpp file:


c++ -o WebGLContextGL.o -c -I../../../dist/stl_wrappers -I../../../dist/system_wrappers -include /home/bjacob/mozilla-central/config/gcc_hidden.h -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux2.6.33.6-147.fc13\" -DOSARCH=Linux -DEXCLUDE_SKIA_DEPENDENCIES -DCHROMIUM_MOZILLA_BUILD  -DOS_LINUX=1 -DOS_POSIX=1  -D_IMPL_NS_LAYOUT -I/home/bjacob/mozilla-central/ipc/chromium/src -I/home/bjacob/mozilla-central/ipc/glue -I../../../ipc/ipdl/_ipdlheaders  -I/home/bjacob/mozilla-central/content/canvas/src -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/home/bjacob/build/firefoxrelease/dist/include/nspr -I/home/bjacob/build/firefoxrelease/dist/include/nss      -I/home/bjacob/mozilla-central/content/canvas/src/../../../layout/xul/base/src -I/home/bjacob/mozilla-central/content/canvas/src/../../../layout/style -I/home/bjacob/mozilla-central/content/canvas/src/../../../layout/generic -I/home/bjacob/mozilla-central/content/canvas/src/../../base/src -I/home/bjacob/mozilla-central/content/canvas/src/../../html/content/src   -fPIC -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -Wno-long-long -fno-strict-aliasing -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -O2 -fno-omit-frame-pointer -g -I/home/bjacob/build/firefoxrelease/dist/include/cairo -pthread -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0     -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/WebGLContextGL.pp /home/bjacob/mozilla-central/content/canvas/src/WebGLContextGL.cpp
No luck as in perf doesn't deal?

Does it have limitations in terms of symbol format (stabs, dwarf, whatever) that it uses?  Or does it just fail to use symbols from inside the object file?  If you write a small C++ program that loops a bunch, compile it with "-O3 -fno-omit-frame-pointer -g" and profile it, does perf work on that?
(Assignee)

Comment 28

7 years ago
Thanks for catching the bug in the Linux case.  I must have mistyped when I was rebasing the patch.

(In reply to comment #25)
> By the way Justin: what's with these lines?
> 
>         GCC_VERSION=`$CC -v 2>&1 | awk '/^gcc version/ { print $3 }'`
>         case $GCC_VERSION in
>         4.1.*|4.2.*|4.5.*)
>             # -Os is broken on gcc 4.1.x 4.2.x, 4.5.x we need to tweak it to
> get good results.
>             MOZ_OPTIMIZE_SIZE_TWEAK="-finline-limit=50"
>         esac
> 
> I'm asking you because I see that you actually tried to remove them, but
> brought them back in a back-out.

I landed a patch to switch to -O3, which makes those lines unnecessary, but had to back out because an orange that I thought was intermittent was actually permanent.

"Buggy" is a matter of perspective.  Its inlining heuristic is more conservative than we want.  But -O3 doesn't have this problem, so I'm able to remove those lines.

Taras has been in contact with the gcc people; I believe they're aware of the -Os inlining issue, but see it as expected ebehavior, since -Os optimizes only for size.
OS: Linux → Windows CE
(In reply to comment #27)
> No luck as in perf doesn't deal?

Yes.

> 
> Does it have limitations in terms of symbol format (stabs, dwarf, whatever)
> that it uses?  Or does it just fail to use symbols from inside the object file?
>  If you write a small C++ program that loops a bunch, compile it with "-O3
> -fno-omit-frame-pointer -g" and profile it, does perf work on that?

The it works fine: perf shows all the symbol names properly.

But then I tried with -O3 -fomit-frame-pointer and it still works !!

Also notice that until a couple of months ago or so, I had no trouble profiling optimized mozilla-central builds. For example in this bug report:

https://bugzilla.mozilla.org/show_bug.cgi?id=557423

In conclusion:
  * something changed during the summer that makes our opt builds no longer profilable in perf
  * that isn't -fomit-frame-pointer
I assume you're just running the build from your objdir as opposed to using make package or some such?

Does adding --enable-debugger-info-modules and --enable-debug-symbols help?

If not, want to bisect to see when the behavior changed?
(Assignee)

Comment 31

7 years ago
You need -fnoomit-frame-pointer to get call graphs.  -g should be sufficient to resolve the names of functions at the top level of the call graph.
(In reply to comment #30)
> Does adding --enable-debugger-info-modules and --enable-debug-symbols help?

The former is deprecated, replaced by the latter, FYI.
(Assignee)

Comment 33

7 years ago
Created attachment 477183 [details] [diff] [review]
Patch v3.1

Properly sets -fno-omit-frame-pointer for Linux.
Attachment #475608 - Attachment is obsolete: true
Attachment #477130 - Attachment is obsolete: true
Attachment #477183 - Flags: review?(ted.mielczarek)
Attachment #475608 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 34

7 years ago
I just clobbered and rebuilt with

. $topsrcdir/browser/config/mozconfig
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../release
ac_add_options --enable-optimize
ac_add_options --disable-debug
ac_add_options --enable-profiling
ac_add_options --srcdir=../src
ac_add_options --enable-application=browser
mk_add_options MOZ_MAKE_FLAGS="-j10"

and get full call graphs with perf.  Linux-64 2.6.32-24-generic, gcc 4.4.3.

Benoit, are you invoking perf record with -i?  Otherwise you might be getting symbols only for the shell script which invokes firefox-bin.
Ah no, I know where my problem was. I was attaching perf to a running firefox, using perf -p $PID, and it seems that in that case perf isn't able to pick up the symbols ! This is quite stupid as it should be able to find back the command line, hence the executable, from the process info. Sorry for the noise, at least I fixed a bug in your patch :-)
(Assignee)

Comment 36

7 years ago
(In reply to comment #35)
> Ah no, I know where my problem was. I was attaching perf to a running firefox,
> using perf -p $PID, and it seems that in that case perf isn't able to pick up
> the symbols !

This might be the same problem as not invoking with -i; dist/bin/firefox is a shell script which invokes dist/bin/firefox-bin.  I'm not sure what $PID is, but if it's like $! in bash, that would be the PID of the shell script, not firefox-bin.

I tried running dist/bin/firefox, then running

  perf record -gp $(ps -a | grep firefox-bin)

and that worked fine.
By $PID I just meant the PID of firefox, that was "pseudo code"! As obtained with ps.

So:
   perf record -gf dist/bin/firefox
             ---> works for me !
             ---> no need for -i

But:
   dist/bin/firefox
   perf  record -gp $(ps -a | grep firefox-bin)
             ---> doesn't work for me.

Since that works for you I guess there's some difference here between our distros, or perf versions. I had just clobbered and rebuilt with:

. $topsrcdir/browser/config/mozconfig
ac_add_options --enable-optimize
ac_add_options --disable-debug
ac_add_options --enable-profiling
ac_add_options --enable-application=browser
ac_add_options --enable-debug-symbols


I would still like to find a way to correctly attach to running process, but what I have now allows me to work already.

The reason why it "used to work" for me was that two months ago I was always using perf to launch rather than attaching to already launched firefox.
Comment on attachment 477183 [details] [diff] [review]
Patch v3.1

The concept of the patch looks fine, but there's one glaring issue. You're trying to make --enable-shark and the like imply --enable-profiling, but those options get handled *after* the code that sets MOZ_OPTIMIZE_FLAGS, where you're referencing $MOZ_PROFILING, so it can't work.
Attachment #477183 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 39

7 years ago
Created attachment 480368 [details] [diff] [review]
Patch v3.2

Ah, yes.  This should be better.
Attachment #477183 - Attachment is obsolete: true
Attachment #480368 - Flags: review?(ted.mielczarek)
Assignee: nobody → justin.lebar+bug
Attachment #480368 - Flags: review?(ted.mielczarek) → review+
Is this ready for landing?
(Assignee)

Comment 41

7 years ago
Yes, unless there are merge conflicts.

I probably won't be able to watch the tree today, so if you want to land it, please be my guest!
I'll add it to the landing queue, as I've already landed my share of csets for the day...  Could you please upload a check-in ready patch?
(Assignee)

Comment 43

7 years ago
Comment on attachment 480368 [details] [diff] [review]
Patch v3.2

Oh, silly me: This needs approval first.
Attachment #480368 - Flags: approval2.0?
Comment on attachment 480368 [details] [diff] [review]
Patch v3.2

NPOTB changes do not require approval.
Attachment #480368 - Flags: approval2.0?
(Assignee)

Updated

7 years ago
OS: Windows CE → All
Hardware: x86_64 → All
(Assignee)

Comment 45

7 years ago
Comment on attachment 480368 [details] [diff] [review]
Patch v3.2

On IRC, there was some disagreement as to whether this is POTB.  To be conservative, I'm re-requesting here.
Attachment #480368 - Flags: approval2.0?
Comment on attachment 480368 [details] [diff] [review]
Patch v3.2

Let's just get this landed.  ;)
Attachment #480368 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 47

7 years ago
Checked in: http://hg.mozilla.org/mozilla-central/rev/c60ae9b15dd2
plus bustage fix: http://hg.mozilla.org/mozilla-central/rev/2402ccc1d428
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 48

7 years ago
Ugh, I caught the error in the first bustage fix, but then pushed the broken version anyway!

Follow-up fix: http://hg.mozilla.org/mozilla-central/rev/565bb05fb42b

Updated

7 years ago
Blocks: 604871
The "make --enable-shark imply --enable-profiling" part of this is totally broken.  See bug 636495.
Depends on: 636495
You need to log in before you can comment on or make changes to this bug.