Sort out optimization defaults

RESOLVED FIXED in Firefox 6

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

({footprint, perf, regression})

Trunk
mozilla7
All
Linux
footprint, perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6+ fixed)

Details

(Whiteboard: [landed m-c 6/15] [-O3 on PGO builds & -Os normal builds])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
Something to maybe keep in mind at some point: there are a bunch of nice wins with -O3 on n900...
(Assignee)

Comment 2

6 years ago
Just a thought, maybe we could have some kind of extension of the --enable-optimize flag.
(Assignee)

Comment 3

6 years ago
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.
Attachment #532900 - Flags: review?(ted.mielczarek)
I filed bug 464328 ages ago to get all that junk out of js/src/Makefile and just make it set things in configure.
(Assignee)

Comment 5

6 years ago
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.
Assignee: nobody → mh+mozilla
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.
Attachment #532900 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 8

6 years ago
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)
Attachment #533642 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

6 years ago
Attachment #532900 - Attachment is obsolete: true
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]:
-----------------------------------------------------------------
Attachment #533642 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/mozilla-central/rev/0bf1fe7f9be1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(Assignee)

Updated

6 years ago
Blocks: 659265
(Assignee)

Updated

6 years ago
Blocks: 464328
http://backedoutbykylehuey.com/
http://hg.mozilla.org/mozilla-central/rev/56c1b58f9b29
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla6 → ---
(Assignee)

Comment 12

6 years ago
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
Attachment #534812 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

6 years ago
Attachment #533642 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
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.
tracking-firefox6: --- → ?
I think this missed the boat, we'll get this for 7.
tracking-firefox6: ? → -
Depends on: 659528
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.
Keywords: regression

Comment 16

6 years ago
re-nominating for Karl per comment 15
tracking-firefox6: - → ?
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.)
(Assignee)

Comment 18

6 years ago
(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.
Keywords: footprint, perf

Updated

6 years ago
Whiteboard: nomination at comment 15

Updated

6 years ago
tracking-firefox6: ? → +
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.
Attachment #534812 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 21

6 years ago
Created attachment 539440 [details] [diff] [review]
On Linux, use -Os on normal builds and -O3 when PGO is enabled.

Refreshed (context changes)
(Assignee)

Updated

6 years ago
Attachment #534812 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
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.
Whiteboard: nomination at comment 15 → [inbound] nomination at comment 15
(Assignee)

Comment 23

6 years ago
(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.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Whiteboard: [inbound] nomination at comment 15 → nomination at comment 15
(Assignee)

Updated

6 years ago
Attachment #539440 - Flags: approval-mozilla-beta?

Updated

6 years ago
Whiteboard: nomination at comment 15 → [landed m-c 6/15] [-O3 on PGO builds & -Os normal builds]
(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).
(Assignee)

Comment 27

6 years ago
(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

6 years ago
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

6 years ago
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 31

6 years ago
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
Attachment #539440 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 32

6 years ago
http://hg.mozilla.org/releases/mozilla-beta/rev/0c07b1a14ec4
status-firefox6: --- → fixed
You need to log in before you can comment on or make changes to this bug.