Nightlies should not override the module-specific build settings.

RESOLVED FIXED

Status

()

Core
Build Config
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Robert Sayre, Assigned: Robert Sayre)

Tracking

({perf})

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
This is bad. At least, it's bad that we do this in nightlies and release builds. It's unlikely that a global setting is good. For example, sqlite should be built with very high optimization, not "-O2".

Nightlies should not override the module-specific build settings.
Flags: blocking1.9?
(Assignee)

Updated

10 years ago
OS: Mac OS X → All
Hardware: PC → All
(Assignee)

Comment 1

10 years ago
sqlite would like to be built with:

-O3 on gcc
-xO5 on sun
-Ox on msvc

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/db/sqlite3/src/Makefile.in&rev=1.30#59
(Assignee)

Updated

10 years ago
Summary: --enable-optimize overrides module-specific optimization settings → Nightlies should not override the module-specific build settings.
I suggested in another bug that we switch to using
--enable-optimize
--enable-debugger-info-modules

This won't quite work on Linux, afaik, since we need to explicitly set -gstabs+, but we might be able to work around that in other ways.
For reference:
http://lxr.mozilla.org/mozilla/source/config/config.mk#538
http://lxr.mozilla.org/mozilla/source/tools/tinderbox-configs/firefox/macosx/mozconfig#15
Linux is actually goofier, fwiw:
http://lxr.mozilla.org/mozilla/source/tools/tinderbox-configs/firefox/linux/mozconfig#14
Only Win32 doesn't screw with this:
http://lxr.mozilla.org/mozilla/source/tools/tinderbox-configs/firefox/win32/mozconfig

Comment 5

10 years ago
I believe the correct solution is to add

export CFLAGS=-gstabs+
export CXXFLAGS=-gstabs+

to the mozconfig, and remove the --enable-optimize flag(s)

Neither --enable-debugger-info-modules nor MOZ_DEBUG_SYMBOLS affects NSPR on non-windows correctly

Updated

10 years ago
Keywords: perf
(Assignee)

Comment 6

10 years ago
Created attachment 292478 [details] [diff] [review]
use the right settings for each tier1 by default, so modules can override
Attachment #292478 - Flags: review?(ted.mielczarek)
Comment on attachment 292478 [details] [diff] [review]
use the right settings for each tier1 by default, so modules can override

Could you add comments to the mozconfigs so that nobody undoes this later?  Just something like
# Don't add explicit optimize flags here, set them in configure.in, see bug 407794.  Also a comment above the CFLAGS/CXXFLAGS explaining that this is for Breakpad would be helpful.
Attachment #292478 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 8

10 years ago
Created attachment 292484 [details] [diff] [review]
address ted's comments
Assignee: nobody → sayrer
Attachment #292478 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #292484 - Flags: approval1.9?

Comment 9

10 years ago
Comment on attachment 292484 [details] [diff] [review]
address ted's comments

go go sayre
Attachment #292484 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 10

10 years ago
This had to be backed out due to a test failure on Linux.

It seemed to hurt mac performance on some tests and improve it on others. Makes sense: shifting to -Os in a few too many places.

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
(Assignee)

Updated

10 years ago
Depends on: 408258
(Assignee)

Comment 11

10 years ago
Created attachment 293018 [details] [diff] [review]
patch with wallpaper for various issues
Attachment #293018 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 12

10 years ago
Comment on attachment 293018 [details] [diff] [review]
patch with wallpaper for various issues

This contains a workaround for bug 408258, since we should stop shipping builds with that problem immediately. It reverts the OS X default optimization to O2, since -Os that regressed Tp and Tdhtml slightly. And since -Os is no longer the default for spidermonkey on mac, this incorporates the already-reviewed patch from bug 407600.
Attachment #293018 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

10 years ago
Attachment #293018 - Flags: approval1.9?
(Assignee)

Comment 13

10 years ago
Comment on attachment 293018 [details] [diff] [review]
patch with wallpaper for various issues

it blocks, no need to approve
Attachment #293018 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Depends on: 408418, 408422
(Assignee)

Comment 14

10 years ago
This works. Filed bugs on issues uncovered by this change:

bug 408258 
bug 408418
bug 408422
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Blocks: 410414
Blocks: 420759
You need to log in before you can comment on or make changes to this bug.