In bug 590181, we had unconditionally enabled aggressive optimization -O3 for everyone using gcc. It's probably not so good an idea, for several reasons: - -O3 is known to be problematic on some older gcc versions - -O3 is known to be problematic with gcc 4.5 (bug 654858) - -O3 makes libxul.so 7MB bigger, increasing cold startup time However, the main advantage of -O3 is to be a better aggressive optimization for PGO. We should probably keep -Os as before by default and only enable -O3 when building with PGO.

Something to maybe keep in mind at some point: there are a bunch of nice wins with -O3 on n900...

Just a thought, maybe we could have some kind of extension of the --enable-optimize flag.

Note that the js/src part is actually mostly useless because js/src/Makefile.in defines MODULE_OPTIMIZE_FLAGS, and it's been a while it's been using -O3 unconditionally.

I filed bug 464328 ages ago to get all that junk out of js/src/Makefile and just make it set things in configure.

On the other hand, doing so would mean using the defaults from the patch here, meaning that without PGO we'd start building with -Os, vs. -O3 previously. That could have a significant impact on performance, though it hasn't been measured. But since we've been building with -O3 for a while in js/src, maybe we simply don't want to change anything there yet.

Well sure, but if we're going to have -O3 as the default, we ought to just set it in configure instead of in the Makefile.

Comment on attachment 532900 [details] [diff] [review] On Linux, use -Os on normal builds and -O3 when PGO is enabled Review of attachment 532900 [details] [diff] [review]: ----------------------------------------------------------------- r+ with one quibble. ::: config/autoconf.mk.in @@ +315,5 @@ > > WARNINGS_AS_ERRORS = @WARNINGS_AS_ERRORS@ > > MOZ_OPTIMIZE = @MOZ_OPTIMIZE@ > +MOZ_OPTIMIZE_FLAGS = $(if $(MOZ_PROFILE_GENERATE)$(MOZ_PROFILE_USE),@MOZ_PGO_OPTIMIZE_FLAGS@,@MOZ_OPTIMIZE_FLAGS@) I think this probably belongs in config.mk. autoconf.mk should stay as simple assignments of substituted values. This might mean you'll have to include config.mk in js/src/Makefile.in to use the right value there.

Something like this? (config.mk is included from rules.mk, which is included in js/src/Makefile.in before using MOZ_OPTIMIZE_FLAGS)

Comment on attachment 533642 [details] [diff] [review] On Linux, use -Os on normal builds and -O3 when PGO is enabled Review of attachment 533642 [details] [diff] [review]: -----------------------------------------------------------------

http://backedoutbykylehuey.com/ http://hg.mozilla.org/mozilla-central/rev/56c1b58f9b29

The case where MOZ_PGO_OPTIMIZE_FLAGS isn't defined was deadly :( Pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=2b2a0ac3a165

We need this in Firefox 6, except if we want to allow Linux distros that build without PGO enabled to ship with bug 654858. We also want this because maemo builds were switched to -O3 as an unexpected side effect, but there is not enough test coverage to assess that it's not subject to similar problems, thus going back to -Os would be safer.

I think this missed the boat, we'll get this for 7.

Requesting re-evaluation for 6. (Perhaps it wasn't clear that currently we have regressions on 6 from bug 590181 and this bug tracks doing something about those regressions.) Bug 659528 covers the broken tracking-firefox6 flag.

re-nominating for Karl per comment 15

The regressions were fairly minor rendering artifacts in a few SVG reftests, right? If that's all it is, that seems like something we can live with. (Especially since they won't show up in our release builds, only non-PGO builds.)

(In reply to comment #17) > only non-PGO builds. Being what most linux distros ship, including Ubuntu.

The layout regressions are not understood, so I can't comment. The start-up time regression would be significant. There's also a footprint regression.

Comment on attachment 534812 [details] [diff] [review] On Linux, use -Os on normal builds and -O3 when PGO is enabled. Review of attachment 534812 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/config.mk @@ +421,5 @@ > ifdef MODULE_OPTIMIZE_FLAGS > CFLAGS += $(MODULE_OPTIMIZE_FLAGS) > CXXFLAGS += $(MODULE_OPTIMIZE_FLAGS) > else > +ifneq (,$(if $(MOZ_PROFILE_GENERATE)$(MOZ_PROFILE_USE),$(MOZ_PGO_OPTIMIZE_FLAGS))) Can you add a comment here explaining what this is testing? It's not very easy to read.

Refreshed (context changes)

http://hg.mozilla.org/integration/mozilla-inbound/rev/e92f98e8a335 Normally, I'd have pushed on b-s, but we don't have talos coverage there.

(In reply to comment #22) > http://hg.mozilla.org/integration/mozilla-inbound/rev/e92f98e8a335 Wrong one, that was meant to be http://hg.mozilla.org/integration/mozilla-inbound/rev/6df0ad2d1df2

I merged this from m-i to m-c, but it will be backed out shortly while we investigate Talos regressions: http://hg.mozilla.org/mozilla-central/rev/6df0ad2d1df2

Correction; this landed on m-c and was not backed out.

(In reply to comment #17) > The regressions were fairly minor rendering artifacts in a few SVG reftests, > right? If that's all it is, that seems like something we can live with. > (Especially since they won't show up in our release builds, only non-PGO > builds.) (In reply to comment #19) > The layout regressions are not understood, so I can't comment. > > The start-up time regression would be significant. > There's also a footprint regression. Discussed in triage here, and we need some help. Is this effectively NPOTB for our official builds, since we are PGO on Linux? If so, why would we rush-land this on beta, versus just letting distros that disable PGO apply this patch as well, and getting it out the door ourselves in the Firefox 7 train? NPOTB makes the risk profile much nicer if we do need to crashland it, but also feels like it reduces the urgency there, since it only helps people who are already custom-patching our code (and could presumably take this patch, too).

(In reply to comment #26) > (In reply to comment #17) > > The regressions were fairly minor rendering artifacts in a few SVG reftests, > > right? If that's all it is, that seems like something we can live with. > > (Especially since they won't show up in our release builds, only non-PGO > > builds.) > > (In reply to comment #19) > > The layout regressions are not understood, so I can't comment. > > > > The start-up time regression would be significant. > > There's also a footprint regression. > > Discussed in triage here, and we need some help. Is this effectively NPOTB > for our official builds, since we are PGO on Linux? Yes > If so, why would we > rush-land this on beta, versus just letting distros that disable PGO apply > this patch as well, and getting it out the door ourselves in the Firefox 7 > train? How do you make sure they (all) do?

Note that we're using this patch in Ubuntu already, because we just can't absorb the size increase that -O3 causes at the moment

This hits the current Thunderbird beta pretty hard too, and this isn't built with PGO on Linux. With everything under mozilla/ getting built with -O3, the current download is more than 2MB bigger than the 5.0 release

(In reply to comment #26) > NPOTB makes the risk profile much nicer if we do need to crashland it, but > also feels like it reduces the urgency there, since it only helps people who > are already custom-patching our code (and could presumably take this patch, > too). I agree with Chris that it would seem to affect Thunderbird, SeaMonkey and other xulrunner based apps that don't build with PGO. Increasing startup time isn't something we want to do unnecessarily. Also xref comment 13 about maemo.

Comment on attachment 539440 [details] [diff] [review] On Linux, use -Os on normal builds and -O3 when PGO is enabled. Approving as this is generally NBOTB for official releases