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.

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.

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.

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

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

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.

(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.

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...

(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...

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

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 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?

(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.

How about that?

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.

(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?

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

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.

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.

(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.

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

http://hg.mozilla.org/mozilla-central/rev/83098dfb4bce ... and then I immediately noticed the backout, so: http://hg.mozilla.org/mozilla-central/rev/ce8ad064cdcb :)

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...

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

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