The default bug view has changed. See this FAQ.

Do nightly builds with profiling disabled

RESOLVED WONTFIX

Status

Release Engineering
General Automation
RESOLVED WONTFIX
3 years ago
a year ago

People

(Reporter: catlee, Assigned: cpeterson)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
mozilla-central only for now

Comment 2

3 years ago
Does this apply for Fennec as well?
(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

3 years ago
Yes, Fennec Nightly is important.
(Assignee)

Comment 5

3 years ago
This is a blocking issue for the games benchmarking initiative.
OS: Linux → All
Hardware: x86_64 → All

Comment 6

3 years ago
:catlee Should this be assigned to you? If not, who is the right person to own this?
(Assignee)

Updated

3 years ago
Flags: needinfo?(catlee)
(Reporter)

Comment 7

3 years ago
Yes, I can take this
Assignee: nobody → catlee
Flags: needinfo?(catlee)
(Reporter)

Comment 8

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

Which Fennec? arm7, arm6 and/or x86?
(Reporter)

Updated

3 years ago
Depends on: 974464
(Reporter)

Comment 9

3 years ago
Created attachment 8378380 [details] [diff] [review]
support enable_noprofiling_build flag
Attachment #8378380 - Flags: review?(nthomas)
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.
Attachment #8378380 - Flags: review?(nthomas) → review+
(Assignee)

Comment 11

3 years ago
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.
(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.)
(Assignee)

Comment 13

3 years ago
--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.
(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?
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).
(Assignee)

Comment 16

3 years ago
(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. :)
(Reporter)

Comment 17

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

Updated

3 years ago
Attachment #8378380 - Flags: checked-in+
(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.
in production
(Assignee)

Comment 20

3 years ago
(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 ^
Flags: needinfo?(akligman)

Comment 21

3 years ago
Nexus 4, which is arm7 I believe.
Flags: needinfo?(akligman)
(Reporter)

Comment 22

3 years ago
Still waiting for dep bug to have someone create the appropriate mozconfigs.
(Assignee)

Comment 23

3 years ago
ack: can you provide the non-profiling mozconfigs?
Flags: needinfo?(akligman)

Comment 24

3 years ago
I don't have these mozconfigs. Ehsan, do you still have yours somewhere? Do we need new ones?
Flags: needinfo?(akligman)
(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!
Delete --enable-profiling, or add --disable-profiling from the standard mozconfig?
(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.
(Assignee)

Comment 28

3 years ago
Created attachment 8387857 [details] [diff] [review]
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
Attachment #8387857 - Flags: review?(mh+mozilla)
Attachment #8387857 - Flags: review?(catlee)
(Assignee)

Comment 29

3 years ago
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.
(Assignee)

Comment 30

3 years ago
Created attachment 8387979 [details] [diff] [review]
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.
Attachment #8387857 - Attachment is obsolete: true
Attachment #8387857 - Flags: review?(mh+mozilla)
Attachment #8387857 - Flags: review?(catlee)
Attachment #8387979 - Flags: review?(mh+mozilla)
Attachment #8387979 - Flags: review?(catlee)
(Reporter)

Comment 31

3 years ago
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.
Attachment #8387979 - Flags: review?(catlee)
(Assignee)

Comment 32

3 years ago
(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).
Flags: needinfo?(catlee)
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
Attachment #8387979 - Flags: review?(mh+mozilla) → review-
(Reporter)

Updated

3 years ago
Flags: needinfo?(catlee)
(Assignee)

Comment 34

3 years ago
Created attachment 8409042 [details] [diff] [review]
mozconfigs-noprofiling-v3.patch

patch v3: override nightly configs instead of release configs.
Assignee: catlee → cpeterson
Status: NEW → ASSIGNED
Attachment #8409042 - Flags: review?(mh+mozilla)
Attachment #8409042 - Flags: review?(catlee)
(Assignee)

Comment 35

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

mozconfigs-noprofiling-v2.patch obsoleted by mozconfigs-noprofiling-v3.patch.
Attachment #8387979 - Attachment is obsolete: true
(Assignee)

Comment 36

3 years ago
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?
Attachment #8409042 - Flags: review?(mh+mozilla)
Attachment #8409042 - Flags: review?(catlee)
Attachment #8409042 - Flags: feedback?(mh+mozilla)
Flags: needinfo?(khuey)
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.
Attachment #8409042 - Flags: feedback?(mh+mozilla)
(Assignee)

Updated

3 years ago
Blocks: 933949
Flags: needinfo?(khuey)
(Assignee)

Comment 38

3 years ago
The browser benchmarking automation is testing whether we need no-profiling builds.
Blocks: 1013650
No longer blocks: 933949
(Reporter)

Comment 39

3 years ago
Any updates here?
(Assignee)

Comment 40

3 years ago
We have tested yet. The benchmark automation team is regrouping with mbest to determine our priorities for which builds and platforms to test.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.