Closed Bug 592923 Opened 14 years ago Closed 14 years ago

Add --enable-profiling configure option

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

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

References

Details

Attachments

(1 file, 5 obsolete files)

Patch v3.2
18.17 KB, patch
ted
: review+
bzbarsky
: approval2.0+
Details | Diff | Splinter Review 
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).
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).
(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.
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.
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.
Would anyone object if I fixed bug 593116 while I'm hacking around in js/src/Makefile.in?
Attached patch Patch v1 (obsolete) — — Splinter Review 
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...
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.
(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.
Attachment #471596 - Flags: review?(ted.mielczarek)
Attached patch Patch v2 (obsolete) — — Splinter Review 
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.
(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.
Attached patch Patch v3 (obsolete) — — Splinter Review 
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??
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.
Attached patch patch with linux fix (obsolete) — — Splinter Review 
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?
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?
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.
Attached patch Patch v3.1 (obsolete) — — Splinter Review 
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)
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 :-)
(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-
Attached patch Patch v3.2 — — Splinter Review 
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?
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?
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?
OS: Windows CE → All
Hardware: x86_64 → All
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+
Checked in: http://hg.mozilla.org/mozilla-central/rev/c60ae9b15dd2
plus bustage fix: http://hg.mozilla.org/mozilla-central/rev/2402ccc1d428
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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
Blocks: 604871
The "make --enable-shark imply --enable-profiling" part of this is totally broken.  See bug 636495.
Depends on: 636495
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: