970432 - Do nightly builds with profiling disabled

Status: RESOLVED WONTFIX

(Release Engineering :: General, defect)

Description

[Chris AtLee [:catlee]]

It's useful for debugging to have regular optimized builds with profiling disabled.

Having these done nightly is sufficient, so let's enable nightly builds with profiling disabled for Win32, Linux64 and OSX.

Comment 1

[Chris AtLee [:catlee]]

mozilla-central only for now

Comment 2

[Alan K [:ack]]

Does this apply for Fennec as well?

Comment 3

[(no longer active)]

(In reply to Alan K [:ack] from comment #2)
> Does this apply for Fennec as well?

If you guys want to do perf comparisons between Fennec Nightly and other channels, then yes.

Comment 4

[Alan K [:ack]]

Yes, Fennec Nightly is important.

Comment 5

[Chris Peterson [:cpeterson]]

This is a blocking issue for the games benchmarking initiative.

Comment 6

[Alan K [:ack]]

:catlee Should this be assigned to you? If not, who is the right person to own this?

Comment 7

[Chris AtLee [:catlee]]

Yes, I can take this

Comment 8

[Chris AtLee [:catlee]]

(In reply to Alan K [:ack] from comment #4)
> Yes, Fennec Nightly is important.

Which Fennec? arm7, arm6 and/or x86?

Comment 9

[Chris AtLee [:catlee]]

Attachment: support enable_noprofiling_build flag

Comment 10

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

Comment on attachment 8378380 [details] [diff] [review]
support enable_noprofiling_build flag

I double checked the code in tree and there's lots of #ifdef MOZ_PROFILING, so a simple strip of symbols isn't enough. Never mind.

Comment 11

[Chris Peterson [:cpeterson]]

We may need a custom mozconfig file because we don't want Nightly's --enable-js-diagnostics and we probably do want Beta/Release's MOZ_PGO=1.

Comment 12

[(no longer active)]

(In reply to comment #11)
> We may need a custom mozconfig file because we don't want Nightly's
> --enable-js-diagnostics and we probably do want Beta/Release's MOZ_PGO=1.

Yeah, this is effectively what I had before (but I did have --enable-js-diagnostics, or at least I didn't explicitly remove it.  Not sure what that does.)

Comment 13

[Chris Peterson [:cpeterson]]

--enable-js-diagnostics #defines JS_CRASH_DIAGNOSTICS, which enables SpiderMonkey memory poisoning and GC and compartment asserts, even in release builds. We definitely don't want that for benchmarking.

Comment 14

[(no longer active)]

(In reply to comment #13)
> --enable-js-diagnostics #defines JS_CRASH_DIAGNOSTICS, which enables
> SpiderMonkey memory poisoning and GC and compartment asserts, even in release
> builds. We definitely don't want that for benchmarking.

Basically I think we want the Release mozconfig + --enable-profiling, right?

Comment 15

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

FYI, catlee's patch expects an in-tree mozconfig at locations like browser/config/mozconfigs/win32/nightly-noprofiling (in tree, repeat for other platforms as needed).

Comment 16

[Chris Peterson [:cpeterson]]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #14)
> 
> Basically I think we want the Release mozconfig + --enable-profiling, right?

No, we specifically do not want --enable-profiling because these builds will be used for benchmarking, where we don't want the profiling code's overhead. I admit this situation is quite confusing. :)

Comment 17

[Chris AtLee [:catlee]]

You can do what you want with the mozconfigs, they are yet to be written! I filed bug 974464 for them.

Comment 18

[(no longer active)]

(In reply to comment #16)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #14)
> > 
> > Basically I think we want the Release mozconfig + --enable-profiling, right?
> 
> No, we specifically do not want --enable-profiling because these builds will be
> used for benchmarking, where we don't want the profiling code's overhead. I
> admit this situation is quite confusing. :)

Er, that was supposed to be a minus, not a plus.  But I also realized that --enable-profiling doesn't appear in those mozconfigs.  So I think what we want is just the release mozconfig.

Comment 19

[[e-mail] (:bhearsum)]

in production

Comment 20

[Chris Peterson [:cpeterson]]

(In reply to Chris AtLee [:catlee] from comment #8)
> (In reply to Alan K [:ack] from comment #4)
> > Yes, Fennec Nightly is important.
> 
> Which Fennec? arm7, arm6 and/or x86?

Alan ^

Comment 21

[Alan K [:ack]]

Nexus 4, which is arm7 I believe.

Comment 22

[Chris AtLee [:catlee]]

Still waiting for dep bug to have someone create the appropriate mozconfigs.

Comment 23

[Chris Peterson [:cpeterson]]

ack: can you provide the non-profiling mozconfigs?

Comment 24

[Alan K [:ack]]

I don't have these mozconfigs. Ehsan, do you still have yours somewhere? Do we need new ones?

Comment 25

[(no longer active)]

(In reply to Alan K [:ack] from comment #24)
> I don't have these mozconfigs. Ehsan, do you still have yours somewhere? Do
> we need new ones?

Not sure what you mean, Alan.  I never wrote those mozconfigs.  Someone should write them!

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Delete --enable-profiling, or add --disable-profiling from the standard mozconfig?

Comment 27

[(no longer active)]

(In reply to comment #26)
> Delete --enable-profiling, or add --disable-profiling from the standard
> mozconfig?

Copying mozconfigs is a bad idea.  It's must better to source one mozconfig into the other and add --enable-profiling in it.  This way you ensure that the mozconfigs will never get out of sync.

Comment 28

[Chris Peterson [:cpeterson]]

Attachment: mozconfigs-noprofiling.patch

Add nightly-noprofiling mozconfigs for Win32, Linux64, Mac OS X, and Android ARMv7. Source each platform's release mozconfig, then try to restore Nightly channel branding.

The --disable-js-diagnostics and --disable-profiling flags are technically not required, but I added them just to be explicit because resetting these flags is the whole reason for this bug! :)

https://tbpl.mozilla.org/?tree=Try&rev=6418d63a33bb

Comment 29

[Chris Peterson [:cpeterson]]

I think I might need to add `ac_add_options --with-macbundlename-prefix=Firefox` (from bug 696436) from the macosx-universal nightly mozconfig to rename the noprofiling's application bundle to FirefoxNightly.app.

Comment 30

[Chris Peterson [:cpeterson]]

Attachment: mozconfigs-noprofiling-v2.patch

Patch v2:
* Add "Firefox" prefix to OS X application bundle.
* release mozconfig already included mozconfig.common.override, so don't include it again.

Comment 31

[Chris AtLee [:catlee]]

Comment on attachment 8387979 [details] [diff] [review]
mozconfigs-noprofiling-v2.patch

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

Not a big fan of basing this on the release mozconfig...but will defer to glandium.

Comment 32

[Chris Peterson [:cpeterson]]

(In reply to Chris AtLee [:catlee] from comment #31)
> Comment on attachment 8387979 [details] [diff] [review]
> mozconfigs-noprofiling-v2.patch
> 
> Review of attachment 8387979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not a big fan of basing this on the release mozconfig...but will defer to
> glandium.

catlee: what are your concerns about copying the release mozconfig? Are you worried that the noprofiling build might inherit some flags that cause the builds to interfere Mozilla's official release build?

If I imported the nightly mozconfig instead, I would have to disable every flag from that file and then enable most of the release mozconfig's flags (like MOZ_PGO=1).

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 8387979 [details] [diff] [review]
mozconfigs-noprofiling-v2.patch

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

You're trading disabling /nightly options with disabling /release options, which is not much better. And you're more likely to miss nightly changes this way.
Note MOZ_PGO=1 is set on nightly-nightlies at the buildbot level (which adds to the overall confusion)

::: browser/config/mozconfigs/linux64/nightly-noprofiling
@@ +3,5 @@
> +ac_add_options --disable-js-diagnostics
> +ac_add_options --disable-official-branding
> +ac_add_options --disable-profiling
> +
> +# Overide release mozconfig's override of EARLY_BETA_OR_EARLIER.

Override

Comment 34

[Chris Peterson [:cpeterson]]

Attachment: mozconfigs-noprofiling-v3.patch

patch v3: override nightly configs instead of release configs.

Comment 35

[Chris Peterson [:cpeterson]]

Comment on attachment 8387979 [details] [diff] [review]
mozconfigs-noprofiling-v2.patch

mozconfigs-noprofiling-v2.patch obsoleted by mozconfigs-noprofiling-v3.patch.

Comment 36

[Chris Peterson [:cpeterson]]

Comment on attachment 8409042 [details] [diff] [review]
mozconfigs-noprofiling-v3.patch

Canceling review because --disable-profiling has side effects other than omitting frame pointer that will affect benchmark perf:

https://hg.mozilla.org/mozilla-central/annotate/28dc0100f9b0/xpcom/glue/nsISupportsImpl.h#l44

In particular, bug 907914 added expensive thread-safety assertions in Nightly's default opt builds. The assertions skewed profiling results, so bug 919380 disabled them in profiling builds (i.e. default Nightly builds). My problem is that my benchmarking builds (with --disable-profiling and --disable-js-diagnostics) will now have those expensive assertions enabled.

* glandium or khuey: do you have any suggestions on how to avoid this problem? I don't really want to add yet another build config flag for --enable-benchmarking. :) Maybe build benchmarking builds with --enable-profiling but add -fomit-frame-pointer to my CXXFLAGS?

Comment 37

[Mike Hommey [:glandium]]

So, in fact, what you seem to want is not to build as a nightly, but rather to build as a release. Sadly, NIGHTLY_BUILD depends on GRE_MILESTONE.

Comment 38

[Chris Peterson [:cpeterson]]

The browser benchmarking automation is testing whether we need no-profiling builds.

Comment 39

[Chris AtLee [:catlee]]

Any updates here?

Comment 40

[Chris Peterson [:cpeterson]]

We have tested yet. The benchmark automation team is regrouping with mbest to determine our priorities for which builds and platforms to test.

Comment 41

[Dan Minor [:dminor]]

We're replacing mozbench with arewefastyet. I'm not sure if this is still relevant, but given the lack of activity here, I think it's safe to close this for now.