Last Comment Bug 655003 - Sort out optimization defaults
: Sort out optimization defaults
Status: RESOLVED FIXED
[landed m-c 6/15] [-O3 on PGO builds ...
: footprint, perf, regression
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla7
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 659528
Blocks: 464328 590181 659265
  Show dependency treegraph
 
Reported: 2011-05-05 06:56 PDT by Mike Hommey [:glandium]
Modified: 2013-12-27 14:34 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
On Linux, use -Os on normal builds and -O3 when PGO is enabled (5.85 KB, patch)
2011-05-17 01:39 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review
On Linux, use -Os on normal builds and -O3 when PGO is enabled (7.41 KB, patch)
2011-05-19 07:44 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review
On Linux, use -Os on normal builds and -O3 when PGO is enabled. (7.48 KB, patch)
2011-05-24 09:54 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review
On Linux, use -Os on normal builds and -O3 when PGO is enabled. (7.48 KB, patch)
2011-06-14 23:08 PDT, Mike Hommey [:glandium]
christian: approval‑mozilla‑beta+
Details | Diff | Review

Description Mike Hommey [:glandium] 2011-05-05 06:56:33 PDT
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.
Comment 1 Mike Hommey [:glandium] 2011-05-05 07:28:12 PDT
Something to maybe keep in mind at some point: there are a bunch of nice wins with -O3 on n900...
Comment 2 Mike Hommey [:glandium] 2011-05-05 09:07:16 PDT
Just a thought, maybe we could have some kind of extension of the --enable-optimize flag.
Comment 3 Mike Hommey [:glandium] 2011-05-17 01:39:48 PDT
Created attachment 532900 [details] [diff] [review]
On Linux, use -Os on normal builds and -O3 when PGO is enabled

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.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-05-17 05:19:06 PDT
I filed bug 464328 ages ago to get all that junk out of js/src/Makefile and just make it set things in configure.
Comment 5 Mike Hommey [:glandium] 2011-05-17 05:37:58 PDT
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.
Comment 6 Ted Mielczarek [:ted.mielczarek] 2011-05-17 05:41:19 PDT
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 7 Ted Mielczarek [:ted.mielczarek] 2011-05-19 06:44:35 PDT
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.
Comment 8 Mike Hommey [:glandium] 2011-05-19 07:44:53 PDT
Created attachment 533642 [details] [diff] [review]
On Linux, use -Os on normal builds and -O3 when PGO is enabled

Something like this?
(config.mk is included from rules.mk, which is included in js/src/Makefile.in before using MOZ_OPTIMIZE_FLAGS)
Comment 9 Ted Mielczarek [:ted.mielczarek] 2011-05-23 08:46:11 PDT
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]:
-----------------------------------------------------------------
Comment 10 Mike Hommey [:glandium] 2011-05-24 04:08:47 PDT
http://hg.mozilla.org/mozilla-central/rev/0bf1fe7f9be1
Comment 12 Mike Hommey [:glandium] 2011-05-24 09:54:29 PDT
Created attachment 534812 [details] [diff] [review]
On Linux, use -Os on normal builds and -O3 when PGO is enabled.

The case where MOZ_PGO_OPTIMIZE_FLAGS isn't defined was deadly :(

Pushed to try:
http://tbpl.mozilla.org/?tree=Try&rev=2b2a0ac3a165
Comment 13 Mike Hommey [:glandium] 2011-05-24 09:59:40 PDT
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.
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2011-05-24 14:50:10 PDT
I think this missed the boat, we'll get this for 7.
Comment 15 Karl Tomlinson (ni?:karlt) 2011-05-24 17:54:43 PDT
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.
Comment 16 Asa Dotzler [:asa] 2011-05-24 22:33:19 PDT
re-nominating for Karl per comment 15
Comment 17 Ted Mielczarek [:ted.mielczarek] 2011-05-25 04:21:34 PDT
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.)
Comment 18 Mike Hommey [:glandium] 2011-05-25 05:02:01 PDT
(In reply to comment #17)
> only non-PGO builds.

Being what most linux distros ship, including Ubuntu.
Comment 19 Karl Tomlinson (ni?:karlt) 2011-05-25 16:00:18 PDT
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 20 Ted Mielczarek [:ted.mielczarek] 2011-06-03 07:40:06 PDT
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.
Comment 21 Mike Hommey [:glandium] 2011-06-14 23:08:37 PDT
Created attachment 539440 [details] [diff] [review]
On Linux, use -Os on normal builds and -O3 when PGO is enabled.

Refreshed (context changes)
Comment 22 Mike Hommey [:glandium] 2011-06-14 23:22:45 PDT
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.
Comment 24 Matt Brubeck (:mbrubeck) 2011-06-15 08:48:51 PDT
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
Comment 25 Matt Brubeck (:mbrubeck) 2011-06-15 19:46:39 PDT
Correction; this landed on m-c and was not backed out.
Comment 26 Johnathan Nightingale [:johnath] 2011-07-18 14:33:24 PDT
(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).
Comment 27 Mike Hommey [:glandium] 2011-07-18 14:54:57 PDT
(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?
Comment 28 Chris Coulson 2011-07-20 10:52:38 PDT
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
Comment 29 Chris Coulson 2011-07-20 11:04:23 PDT
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
Comment 30 Mark Banner (:standard8) 2011-07-25 13:34:02 PDT
(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 31 christian 2011-07-25 14:33:05 PDT
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
Comment 32 Mike Hommey [:glandium] 2011-07-26 01:09:19 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/0c07b1a14ec4

Note You need to log in before you can comment on or make changes to this bug.