654049 - Change optimization level when building cairo, pixmap and sqlite

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

As mentioned by Makoto Kato in bug 590181 comment 135, some Makefile still sets optimize flag to -O2:
http://mxr.mozilla.org/mozilla-central/source/db/sqlite3/src/Makefile.in
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/Makefile.in
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/Makefile.in

Blame identifies the corresponding commits:
http://hg.mozilla.org/mozilla-central/rev/96f77ba91358 (erf "temporarily")
http://hg.mozilla.org/mozilla-central/rev/2287ccec814a

This seems to indicate there's no particular reason for that to be -O2 specifically. I don't even know why it's not simply using MOZ_OPTIMIZE_FLAGS.

Comment 1

[(not currently active) Ted Mielczarek]

I would bet that the sqlite thing was simply to get it down from -O3 to -O2. bug 341137 comment 33 explains why it was -O3 to begin with. I'd guess that we should simply remove all the special optimize flags there and let the compiler sort it out now that we have PGO. Similarly, bug 386897 wanted more aggressive optimization for gfx stuff, which is moot now that we're at -O3/PGO.

Comment 2

[Mike Hommey [:glandium]]

Looking for MODULE_OPTIMIZE_FLAGS, there are a few more places where flags are overridden
http://mxr.mozilla.org/mozilla-central/search?string=MODULE_OPTIMIZE_FLAGS

Might be worth taking care of (some of) those too.

Comment 3

[Mike Hommey [:glandium]]

These are the ones that look like could be removed completely:
http://mxr.mozilla.org/mozilla-central/source/config/Makefile.in#78
http://mxr.mozilla.org/mozilla-central/source/config/mkdepend/Makefile.in#50 (btw, I wonder if mkdepend is still useful)
http://mxr.mozilla.org/mozilla-central/source/js/src/config/Makefile.in#70
http://mxr.mozilla.org/mozilla-central/source/js/src/config/mkdepend/Makefile.in#50
http://mxr.mozilla.org/mozilla-central/source/memory/jemalloc/Makefile.in#112 (though, I'm not completely sure)

We also probably want to tweak these:
http://mxr.mozilla.org/mozilla-central/source/js/src/Makefile.in#84

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We shouldn't be using mkdepend anywhere these days, at least on Tier 1 platforms.

Comment 5

[(not currently active) Ted Mielczarek]

I wouldn't worry about the config/ stuff at all, really, although we can just remove those out of principle. Having -O3 in config/ always weirded me out, honestly. jemalloc is a bit scary, but hopefully GCC won't miscompile it or anything silly like that.

Comment 6

[Mike Hommey [:glandium]]

(In reply to comment #5)
> I wouldn't worry about the config/ stuff at all, really, although we can just
> remove those out of principle. Having -O3 in config/ always weirded me out,
> honestly. jemalloc is a bit scary, but hopefully GCC won't miscompile it or
> anything silly like that.

About jemalloc, I'm afraid of other platforms, though. So it would probably need an ifndef GNU_CC or something like that.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Use global optimization flags with GCC for jemalloc, cairo, pixman and sqlite

Comment 8

[Justin Lebar (not reading bugmail)]

Do we have any evidence gcc is miscompiling jemalloc at -O3?  Otherwise, why force it to -O2?  It seems worth trying to make it go fast...

Comment 9

[Mike Hommey [:glandium]]

(In reply to comment #8)
> Do we have any evidence gcc is miscompiling jemalloc at -O3?  Otherwise, why
> force it to -O2?  It seems worth trying to make it go fast...

That's what the patch is for, though the effect on non pgo builds would be to build -Os...

Comment 10

[Justin Lebar (not reading bugmail)]

Oh, I totally misread the "ifndef GNU_CC" as "ifdef GNU_CC".  That makes a big difference.  :)

Comment 11

[Mike Hommey [:glandium]]

http://tbpl.mozilla.org/?tree=Try&rev=8807e66e84d6 Looks like nothing breaks as a consequence, but not all builds and tests are finished yet.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 546498 [details] [diff] [review]
Use global optimization flags with GCC for jemalloc, cairo, pixman and sqlite

Review of attachment 546498 [details] [diff] [review]:
-----------------------------------------------------------------

::: db/sqlite3/src/Makefile.in
@@ +81,5 @@
>  MODULE_OPTIMIZE_FLAGS = -xO5
>  endif
>  ifeq ($(OS_ARCH),WINNT)
>  MODULE_OPTIMIZE_FLAGS = -O2
>  endif

I wonder if we still need these for Windows?

::: memory/jemalloc/Makefile.in
@@ +130,2 @@
>  MODULE_OPTIMIZE_FLAGS = -O2
> +endif

I bet you can probably just remove this block entirely. This would narrow it down to non-Windows non-GCC non-Solaris builds, which is what, like AIX?

Comment 13

[Mike Hommey [:glandium]]

(In reply to comment #12)
> Comment on attachment 546498 [details] [diff] [review] [review]
> Use global optimization flags with GCC for jemalloc, cairo, pixman and sqlite
> 
> Review of attachment 546498 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: db/sqlite3/src/Makefile.in
> @@ +81,5 @@
> >  MODULE_OPTIMIZE_FLAGS = -xO5
> >  endif
> >  ifeq ($(OS_ARCH),WINNT)
> >  MODULE_OPTIMIZE_FLAGS = -O2
> >  endif
> 
> I wonder if we still need these for Windows?

I guess we don't
http://hg.mozilla.org/mozilla-central/rev/2287ccec814a
http://hg.mozilla.org/mozilla-central/rev/96f77ba91358

> ::: memory/jemalloc/Makefile.in
> @@ +130,2 @@
> >  MODULE_OPTIMIZE_FLAGS = -O2
> > +endif
> 
> I bet you can probably just remove this block entirely. This would narrow it
> down to non-Windows non-GCC non-Solaris builds, which is what, like AIX?

Fair enough.

Comment 14

[Mike Hommey [:glandium]]

Attachment: Use global optimization flags with GCC for jemalloc, cairo, pixman and sqlite

How about that?

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 546802 [details] [diff] [review]
Use global optimization flags with GCC for jemalloc, cairo, pixman and sqlite

Review of attachment 546802 [details] [diff] [review]:
-----------------------------------------------------------------

Make sure you watch for OS X perf changes when this lands. I think it should be fine, but who knows.

Comment 16

[Matheus Kerschbaum]

(In reply to comment #12)
> 
> ::: db/sqlite3/src/Makefile.in
> @@ +81,5 @@
> >  MODULE_OPTIMIZE_FLAGS = -xO5
> >  endif
> >  ifeq ($(OS_ARCH),WINNT)
> >  MODULE_OPTIMIZE_FLAGS = -O2
> >  endif
> 
> I wonder if we still need these for Windows?

Isn't -O1 the default opt flag on Windows? (http://mxr.mozilla.org/mozilla-central/source/configure.in#2342)

So wouldn't this patch make SQLite, cairo and pixman be less optimized?

Comment 17

[Mike Hommey [:glandium]]

Good question indeed. We're doing PGO builds on windows, doesn't that make the optimization level option more or less pointless?

Comment 18

[(not currently active) Ted Mielczarek]

I wouldn't expect it to make any difference unless there's code that is actually perf-sensitive in our Talos tests that doesn't get exercised by our profiling step. Seems unlikely to me, but we can certainly find out.

I don't really know what MSVC does with the default optimization level when building PGO. It may be that it uses it as the default for non-hot code, or it may ignore it entirely and compile hot code for speed and cold code for size.

Comment 19

[Mike Hommey [:glandium]]

Reading the msdn documentation, it does look like PGO and optimization level are somehow orthogonal, except that you're not expected to do PGO with /Od. If it is indeed unrelated, that means we should probably switch the entire tree to /O2 instead of the current default.

Comment 20

[Justin Lebar (not reading bugmail)]

(In reply to comment #19)
> Reading the msdn documentation, it does look like PGO and optimization level
> are somehow orthogonal, except that you're not expected to do PGO with /Od.
> If it is indeed unrelated, that means we should probably switch the entire
> tree to /O2 instead of the current default.

This is bug 595295.

Comment 21

[Mike Hommey [:glandium]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/83098dfb4bce

Comment 22

[Mike Hommey [:glandium]]

Backed out because of Android talos regression
http://hg.mozilla.org/integration/mozilla-inbound/rev/ce8ad064cdcb

Comment 23

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/83098dfb4bce

... and then I immediately noticed the backout, so:

http://hg.mozilla.org/mozilla-central/rev/ce8ad064cdcb

:)

Comment 24

[Mike Hommey [:glandium]]

So, I tried building the entire tree at -O2 on android. Not only are the builds bigger (the apk is 700K bigger), but they're not even faster. They even look slower in some cases... (tdhtml_nochrome is even significantly slower (provided low values are better))

So all in all, we might want to keep -O2 for android, and i guess the regression due to cairo...

Comment 25

[Mike Hommey [:glandium]]

(In reply to comment #24)
> (the apk is 700K bigger)

840K, even

Comment 26

[Mike Hommey [:glandium]]

Attachment: Use global optimization flags for jemalloc, cairo, pixman and sqlite.

Comment 27

[Mike Hommey [:glandium]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/86e9d6953e50

Comment 28

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/86e9d6953e50

Comment 29

[Shawn Wilsher :sdwilsh]

Please cc me in the future if you are changing how SQLite is built!