Last Comment Bug 654049 - Change optimization level when building cairo, pixmap and sqlite
: Change optimization level when building cairo, pixmap and sqlite
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla8
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 676499
Blocks: 590181
  Show dependency treegraph
 
Reported: 2011-05-01 23:29 PDT by Mike Hommey [:glandium]
Modified: 2011-08-07 15:35 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use global optimization flags with GCC for jemalloc, cairo, pixman and sqlite (2.56 KB, patch)
2011-07-18 05:10 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review
Use global optimization flags with GCC for jemalloc, cairo, pixman and sqlite (2.66 KB, patch)
2011-07-19 09:07 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review
Use global optimization flags for jemalloc, cairo, pixman and sqlite. (2.65 KB, patch)
2011-07-26 10:08 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review

Description Mike Hommey [:glandium] 2011-05-01 23:29:35 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-05-02 05:03:01 PDT
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] 2011-05-02 05:09:19 PDT
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 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-02 05:24:24 PDT
We shouldn't be using mkdepend anywhere these days, at least on Tier 1 platforms.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2011-05-02 06:17:55 PDT
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] 2011-05-02 06:24:05 PDT
(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] 2011-07-18 05:10:57 PDT
Created attachment 546498 [details] [diff] [review]
Use global optimization flags with GCC for jemalloc, cairo, pixman and sqlite
Comment 8 Justin Lebar (not reading bugmail) 2011-07-18 06:01:56 PDT
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] 2011-07-18 06:13:22 PDT
(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) 2011-07-18 06:16:59 PDT
Oh, I totally misread the "ifndef GNU_CC" as "ifdef GNU_CC".  That makes a big difference.  :)
Comment 11 Mike Hommey [:glandium] 2011-07-18 08:37:27 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-07-19 08:50:54 PDT
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] 2011-07-19 08:58:18 PDT
(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] 2011-07-19 09:07:23 PDT
Created attachment 546802 [details] [diff] [review]
Use global optimization flags with GCC for jemalloc, cairo, pixman and sqlite

How about that?
Comment 15 Ted Mielczarek [:ted.mielczarek] 2011-07-19 09:09:47 PDT
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 2011-07-19 16:44:23 PDT
(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] 2011-07-19 23:47:01 PDT
Good question indeed. We're doing PGO builds on windows, doesn't that make the optimization level option more or less pointless?
Comment 18 Ted Mielczarek [:ted.mielczarek] 2011-07-20 05:29:32 PDT
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] 2011-07-20 05:41:18 PDT
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) 2011-07-20 06:11:39 PDT
(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 22 Mike Hommey [:glandium] 2011-07-21 05:36:05 PDT
Backed out because of Android talos regression
http://hg.mozilla.org/integration/mozilla-inbound/rev/ce8ad064cdcb
Comment 23 :Ehsan Akhgari (out sick) 2011-07-21 08:15:17 PDT
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] 2011-07-21 09:43:51 PDT
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] 2011-07-21 09:44:53 PDT
(In reply to comment #24)
> (the apk is 700K bigger)

840K, even
Comment 26 Mike Hommey [:glandium] 2011-07-26 10:08:09 PDT
Created attachment 548510 [details] [diff] [review]
Use global optimization flags for jemalloc, cairo, pixman and sqlite.
Comment 28 Marco Bonardo [::mak] 2011-08-01 08:09:44 PDT
http://hg.mozilla.org/mozilla-central/rev/86e9d6953e50
Comment 29 Shawn Wilsher :sdwilsh 2011-08-07 15:35:11 PDT
Please cc me in the future if you are changing how SQLite is built!

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