748669 - Make JS_{Start,Stop}Profiling work with Instruments on Lion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Peter Van der Beken [:peterv]]

I have a patch that does this, needs some more work though. It depends on the DTPerformanceSession framework, but I've managed to load that dynamically so it should not add any new build dependcies. The only problem is finding the framework, I'm not completely happy with hardcoding the path to it.

Comment 1

[Steve Fink [:sfink] [:s:]]

I'm doing something similar with Intel's VTune Amplifier. Currently, I'm using a --with-ampljit[=PATH] configure option. If no argument is given, it falls back onto a hardcoded path (which still sucks because even that path should differ between 32 and 64 bit.)

This is unreviewed and I plan to at least fix the 32/64 issue before landing, but maybe it'll give you ideas:

+MOZ_ARG_WITH_STRING(ampljit,
+[  --with-ampljit=DIR      Report JIT-generated code to VTune Amplifier using support library installed at a given location],
+  MOZ_AMPL_JIT=1
+  MOZ_AMPL_JIT_LIBDIR=$withval,
+  MOZ_AMPL_JIT= )
+
+AC_DEFINE(MOZ_AMPL_JIT)
+if test -n "$MOZ_AMPL_JIT"; then
+  if test "$MOZ_AMPL_JIT_LIBDIR" = "yes"; then
+    MOZ_AMPL_JIT_LIBDIR=/opt/intel/vtune_amplifier_xe/lib64
+  fi
+  LIBS="$LIBS -L$MOZ_AMPL_JIT_LIBDIR"
+  AC_CHECK_LIB(jitprofiling,iJIT_NotifyEvent)
+fi

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

This puts the profile in /tmp (named by the argument given to stopProfiling or "mozilla"), it can then be opened in the Instuments application for analysis.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 5

[Boris Zbarsky [:bzbarsky]]

Peter, what can I do to help this happen?  Do you want me to just take over merging to tip and scaring up reviews?  I keep having to profile on 10.6.  :(

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Steve, let me know if you're a good reviewer for this or if I should look for someone else. Thanks!

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 689421 [details] [diff] [review]
v1.2

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

Seems like a good implementation of my crappy API. :-)

The build bits look ok to me, assuming it's better to do the -DMOZ_INSTRUMENTS in the Makefile.in instead of using AC_DEFINE. But you need a build peer to sign off on that.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 689421 [details] [diff] [review]
v1.2

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

r- for the update channel question.

::: browser/config/mozconfigs/macosx-universal/nightly
@@ +26,5 @@
>  # Package js shell.
>  export MOZ_PACKAGE_JSSHELL=1
>  
> +ac_add_options --enable-instruments
> +ac_add_options --enable-dtrace

These deserve a comment. Perhaps they should be grouped with --enable-profiling? Are we going to want to keep these enabled for aurora/beta/etc?

::: js/src/Makefile.in
@@ +592,5 @@
>  	$(MAKE) -C config/ nsinstall$(HOST_BIN_SUFFIX)
>  endif
>  
> +ifdef MOZ_INSTRUMENTS
> +OS_LIBS += -framework CoreFoundation

Feels like js/src/configure.in should just stick this in some sort of LIBS since you're repeating yourself in a bunch of Makefiles.

::: js/src/shell/Makefile.in
@@ -62,5 @@
> -ifdef MOZ_SHARK
> -CFLAGS += -F/System/Library/PrivateFrameworks
> -CXXFLAGS += -F/System/Library/PrivateFrameworks
> -LDFLAGS += -F/System/Library/PrivateFrameworks -framework CHUD
> -endif

Are we no longer supporting MOZ_SHARK? You left some other code blocks alone.

::: js/xpconnect/shell/Makefile.in
@@ +78,5 @@
>  ifdef MOZ_SHARK
>  DEFINES += -DMOZ_SHARK
> +endif
> +ifdef MOZ_INSTRUMENTS
> +DEFINES += -DMOZ_INSTRUMENTS

I'd move these all to be AC_DEFINEs in configure.in.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +214,5 @@
>    abi += "-shark"
>  #endif
> +#ifdef MOZ_INSTRUMENTS
> +  // Disambiguate optimised and instruments nightlies
> +  abi += "-instruments"

Do we really want this? You're enabling this on our normal nightlies. The Shark hack was because we had special Shark nightlies.

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> > +ac_add_options --enable-instruments
> > +ac_add_options --enable-dtrace
> 
> These deserve a comment. Perhaps they should be grouped with
> --enable-profiling? Are we going to want to keep these enabled for
> aurora/beta/etc?

--enable-profiling is removed on merge to Aurora by Release Management, so they'd need to know to remove the new lines too. That's Alex Keybl/Lukas Blakk/Bhavana Bajaj. Grouping them together makes sense for that.

> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +214,5 @@
> >    abi += "-shark"
> >  #endif
> > +#ifdef MOZ_INSTRUMENTS
> > +  // Disambiguate optimised and instruments nightlies
> > +  abi += "-instruments"
> 
> Do we really want this? You're enabling this on our normal nightlies. The
> Shark hack was because we had special Shark nightlies.

I don't think we do for just one set of nightlies. The shark nightlies are long discontinued so the '-shark' append can go.

Comment 10

[(no longer active)]

(In reply to comment #9)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> > > +ac_add_options --enable-instruments
> > > +ac_add_options --enable-dtrace
> > 
> > These deserve a comment. Perhaps they should be grouped with
> > --enable-profiling? Are we going to want to keep these enabled for
> > aurora/beta/etc?
> 
> --enable-profiling is removed on merge to Aurora by Release Management, so
> they'd need to know to remove the new lines too. That's Alex Keybl/Lukas
> Blakk/Bhavana Bajaj. Grouping them together makes sense for that.

Also, please ping me when this lands, as it probably breaks the merge script on the profiling branch which I use in order to keep testing builds without --enable-profiling so that we don't get surprised by a suddenly failing test for example when we uplift to Aurora.

Thanks!

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> Are we no longer supporting MOZ_SHARK? You left some other code blocks alone.

This actually fixes the MOZ_SHARK support, we haven't needed the CHUD framework since bug 625962 landed, but we were still looking for it. We could enable Shark support on nightlies too if we want (for 10.6 users), both the Instruments and the Shark support will just throw an exception if the user doesn't have them installed.

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v1.3

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 726645 [details] [diff] [review]
v1.3

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

::: browser/config/mozconfigs/macosx-universal/nightly
@@ +26,5 @@
>  # Package js shell.
>  export MOZ_PACKAGE_JSSHELL=1
>  
> +ac_add_options --enable-instruments
> +ac_add_options --enable-dtrace

Please move these up below --enable-profiling as discussed. Per comment 9 you'll also want to let Release Management know that they'll have to remove these on uplift.

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: v1.4

Comment 15

[Peter Van der Beken [:peterv]]

Setting needinfo to Alex to be sure release management sees this before the next merge. This patch adds these two lines:

ac_add_options --enable-instruments
ac_add_options --enable-dtrace

to the mozconfig file for the OS X nightly (browser/config/mozconfigs/macosx-universal/nightly). We should remove them on every merge to Aurora (as I'm told we already do for the enable-profiling line).

Comment 16

[Peter Van der Beken [:peterv]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> Also, please ping me when this lands, as it probably breaks the merge script
> on the profiling branch

Patch is ready to land, do you need me to wait until you've fixed the merge script?

Comment 17

[(no longer active)]

(In reply to comment #16)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> > Also, please ping me when this lands, as it probably breaks the merge script
> > on the profiling branch
> 
> Patch is ready to land, do you need me to wait until you've fixed the merge
> script?

No, I need to take care of it after the patch finds its way to mozilla-central.  I _may_ be taking tomorrow off, so it would be really helpful if you can either land it today on mozilla-central or tomorrow on inbound, so that it gets merged on Thursday.

Thanks!

Comment 18

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/mozilla-central/rev/126563fd3ba1

Comment 19

[Jason Orendorff [:jorendorff]]

Like every change to the build system, this broke --disable-threadsafe standalone SpiderMonkey builds on Mac. Not totally sure what to do about it.

https://gist.github.com/jorendorff/5201650

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that fixes bustage for me that looks a lot like jorendorff's

Jason, does this fix your shell build?

Peter, I ran into this trying to build --enable-shared-js without --enable-instruments....

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 727043 [details] [diff] [review]
Patch that fixes bustage for me that looks a lot like jorendorff's

Works here. Thanks, bz.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Pushed followup as https://hg.mozilla.org/integration/mozilla-inbound/rev/8512166379e8

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8512166379e8

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

this is removed from Aurora 23, Beta 22:

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8161fba947c
https://hg.mozilla.org/releases/mozilla-beta/rev/75aa37cf382c

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

I've updated our merge day script and docs accordingly, this will be removed in the next merge day:

https://gist.github.com/lsblakk/5568843
https://wiki.mozilla.org/Release_Management/Merge_Documentation#Verify_.26_Commit_mozconfig_dtrace_.26_instruments_changes