Closed
Bug 970432
Opened 11 years ago
Closed 9 years ago
Do nightly builds with profiling disabled
Categories
(Release Engineering :: General, defect)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: catlee, Assigned: cpeterson)
References
Details
Attachments
(2 files, 2 obsolete files)
3.51 KB,
patch
|
nthomas
:
review+
catlee
:
checked-in+
|
Details | Diff | Splinter Review |
2.22 KB,
patch
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
mozilla-central only for now
Comment 2•11 years ago
|
||
Does this apply for Fennec as well?
Comment 3•11 years ago
|
||
(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•11 years ago
|
||
Yes, Fennec Nightly is important.
Assignee | ||
Comment 5•11 years ago
|
||
This is a blocking issue for the games benchmarking initiative.
OS: Linux → All
Hardware: x86_64 → All
Comment 6•11 years ago
|
||
:catlee Should this be assigned to you? If not, who is the right person to own this?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(catlee)
Reporter | ||
Comment 7•11 years ago
|
||
Yes, I can take this
Assignee: nobody → catlee
Flags: needinfo?(catlee)
Reporter | ||
Comment 8•11 years ago
|
||
(In reply to Alan K [:ack] from comment #4)
> Yes, Fennec Nightly is important.
Which Fennec? arm7, arm6 and/or x86?
Reporter | ||
Comment 9•11 years ago
|
||
Attachment #8378380 -
Flags: review?(nthomas)
Comment 10•11 years ago
|
||
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•11 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.
Comment 12•11 years ago
|
||
(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•11 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.
Comment 14•11 years ago
|
||
(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•11 years ago
|
||
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•11 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•11 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•11 years ago
|
Attachment #8378380 -
Flags: checked-in+
Comment 18•11 years ago
|
||
(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•11 years ago
|
||
in production
Assignee | ||
Comment 20•11 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)
Reporter | ||
Comment 22•11 years ago
|
||
Still waiting for dep bug to have someone create the appropriate mozconfigs.
Assignee | ||
Comment 23•11 years ago
|
||
ack: can you provide the non-profiling mozconfigs?
Flags: needinfo?(akligman)
Comment 24•11 years ago
|
||
I don't have these mozconfigs. Ehsan, do you still have yours somewhere? Do we need new ones?
Flags: needinfo?(akligman)
Comment 25•11 years ago
|
||
(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?
Comment 27•11 years ago
|
||
(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•11 years ago
|
||
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•11 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•11 years ago
|
||
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•11 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•11 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 33•11 years ago
|
||
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•11 years ago
|
Flags: needinfo?(catlee)
Assignee | ||
Comment 34•11 years ago
|
||
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•11 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•11 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)
Comment 37•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #8409042 -
Flags: feedback?(mh+mozilla)
Assignee | ||
Comment 38•10 years ago
|
||
The browser benchmarking automation is testing whether we need no-profiling builds.
Reporter | ||
Comment 39•10 years ago
|
||
Any updates here?
Assignee | ||
Comment 40•10 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.
Comment 41•9 years ago
|
||
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
Closed: 9 years ago
Resolution: --- → WONTFIX
Updated•7 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•