Closed Bug 748669 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: peterv, Assigned: peterv)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
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
Attached patch v1 (obsolete) — Splinter Review
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.
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #618952 - Attachment is obsolete: true
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.  :(
Attached patch v1.2 (obsolete) — Splinter Review
Steve, let me know if you're a good reviewer for this or if I should look for someone else. Thanks!
Attachment #689421 - Flags: review?(sphink)
Attachment #655150 - Attachment is obsolete: true
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.
Attachment #689421 - Flags: review?(ted)
Attachment #689421 - Flags: review?(sphink)
Attachment #689421 - Flags: review+
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.
Attachment #689421 - Flags: review?(ted) → review-
(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.
(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!
(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.
Attached patch v1.3 (obsolete) — Splinter Review
Attachment #689421 - Attachment is obsolete: true
Attachment #726645 - Flags: review?(ted)
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.
Attachment #726645 - Flags: review?(ted) → review+
Attached patch v1.4Splinter Review
Attachment #726645 - Attachment is obsolete: true
Attachment #726655 - Flags: review+
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).
Flags: needinfo?(akeybl)
(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?
Flags: needinfo?(ehsan)
(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!
Flags: needinfo?(ehsan)
Target Milestone: --- → mozilla22
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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
Jason, does this fix your shell build?

Peter, I ran into this trying to build --enable-shared-js without --enable-instruments....
Attachment #727043 - Flags: review?(peterv)
Attachment #727043 - Flags: feedback?(jorendorff)
Attachment #727043 - Flags: review?(peterv) → review+
Comment on attachment 727043 [details] [diff] [review]
Patch that fixes bustage for me that looks a lot like jorendorff's

Works here. Thanks, bz.
Attachment #727043 - Flags: feedback?(jorendorff) → feedback+
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
Flags: needinfo?(akeybl)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: