Closed Bug 1186444 Opened 4 years ago Closed 4 years ago

remove MODULE_OPTIMIZE_FLAGS

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

If I'm reading things correctly, all of our uses of MODULE_OPTIMIZE_FLAGS can
be replaced with appropriate assignments in moz.build files.  Less stuff in
Makefile.in files = win.
It's not obvious to me what MODULE_OPTIMIZE_FLAGS was buying us,
historically, but now that we have moz.build, we can be guaranteed that
any flags we add in moz.build will be added after everything else has
been setup.  So any uses of MODULE_OPTIMIZE_FLAGS can be moved to
moz.build's CFLAGS/CXXFLAGS without any weird repercussions.

I added bug references for the non-config/moz.build changes so it's more
obvious what we're trying to fix.
Attachment #8637260 - Flags: review?(mshal)
I thought there was someplace I could add code to complain about usage of
obsolete variables, but I'm not finding it; this would be a worthwhile variable
to add to the list.

The MOZ_OPTIMIZE if-else chain that we removed MODULE_OPTIMIZE_FLAGS from could
probably stand to be cleaned up, but that is a different bug.
Attachment #8637261 - Flags: review?(mshal)
Blocks: nomakefiles
Comment on attachment 8637260 [details] [diff] [review]
part 1 - move uses of MODULE_OPTIMIZE_FLAGS to moz.build's CFLAGS

Don't you still need to check MOZ_OPTIMIZE before adding these to CFLAGS? In config.mk, the flags are only added inside 'ifeq (1,$(MOZ_OPTIMIZE))' I think. Otherwise, I think it looks fine.
Attachment #8637260 - Flags: review?(mshal) → feedback+
Comment on attachment 8637261 [details] [diff] [review]
part 2 - remove MODULE_OPTIMIZE_FLAGS from config.mk

Looks mostly good, though db/sqlite3/src/Makefile.in still has some MODULE_OPTIMIZE_FLAGS. I guess the  blacklist would help here, which was moved into mozbuild: http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/recursivemake.py#126
Attachment #8637261 - Flags: review?(mshal) → feedback+
(In reply to Michael Shal [:mshal] from comment #3)
> Comment on attachment 8637260 [details] [diff] [review]
> part 1 - move uses of MODULE_OPTIMIZE_FLAGS to moz.build's CFLAGS
> 
> Don't you still need to check MOZ_OPTIMIZE before adding these to CFLAGS? In
> config.mk, the flags are only added inside 'ifeq (1,$(MOZ_OPTIMIZE))' I
> think. Otherwise, I think it looks fine.

For these particular modules, I don't think it makes much difference.  It would be a behavior change, but I think it'd be pretty minimal...and maybe it would even speed up tests on Android in debug mode?

WDYT?

(In reply to Michael Shal [:mshal] from comment #4)
> Looks mostly good, though db/sqlite3/src/Makefile.in still has some
> MODULE_OPTIMIZE_FLAGS. I guess the  blacklist would help here, which was
> moved into mozbuild:
> http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/
> backend/recursivemake.py#126

Ah, so that MODULE_OPTIMIZE_FLAGS got removed by bug 1185616, which explains our disagreement on this point. :)

I will add things to the blacklist, though, thanks for the pointer.
Flags: needinfo?(mshal)
(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #5)
> (In reply to Michael Shal [:mshal] from comment #3)
> > Comment on attachment 8637260 [details] [diff] [review]
> > part 1 - move uses of MODULE_OPTIMIZE_FLAGS to moz.build's CFLAGS
> > 
> > Don't you still need to check MOZ_OPTIMIZE before adding these to CFLAGS? In
> > config.mk, the flags are only added inside 'ifeq (1,$(MOZ_OPTIMIZE))' I
> > think. Otherwise, I think it looks fine.
> 
> For these particular modules, I don't think it makes much difference.  It
> would be a behavior change, but I think it'd be pretty minimal...and maybe
> it would even speed up tests on Android in debug mode?
> 
> WDYT?

I don't have enough historical context to make a call. I don't have any arguments against it though - I was just pointing out the discrepancy.
Flags: needinfo?(mshal)
Attachment #8637260 - Flags: feedback+ → review+
Comment on attachment 8637261 [details] [diff] [review]
part 2 - remove MODULE_OPTIMIZE_FLAGS from config.mk

r+ assuming it's added to the ban list.
Attachment #8637261 - Flags: feedback+ → review+
debug builds are optimized by default. But people who do build with --disable-optimize, well, expect no optimizations... so forcing some -O flags in some random places is not going to make them happy.
It's not obvious to me what MODULE_OPTIMIZE_FLAGS was buying us,
historically, but now that we have moz.build, we can be guaranteed that
any flags we add in moz.build will be added after everything else has
been setup.  So any uses of MODULE_OPTIMIZE_FLAGS can be moved to
moz.build's CFLAGS/CXXFLAGS without any weird repercussions.

I don't know that always having to add MOZ_OPTIMIZE checks is great, but there
you go.  It might be more correct to check |CONFIG['MOZ_OPTIMIZE'] == 1|...
Attachment #8637260 - Attachment is obsolete: true
Attachment #8639416 - Flags: review?(mshal)
MODULE_OPTIMIZE_FLAGS has been added to the blacklist.  Carrying over r+.
Attachment #8637261 - Attachment is obsolete: true
Attachment #8639417 - Flags: review+
Comment on attachment 8639416 [details] [diff] [review]
part 1 - move uses of MODULE_OPTIMIZE_FLAGS to moz.build's CFLAGS

>--- a/gfx/cairo/libpixman/src/Makefile.in
>+++ b/gfx/cairo/libpixman/src/Makefile.in
>-ifeq ($(OS_TARGET),Android)
>-MODULE_OPTIMIZE_FLAGS = -O2
>-endif

>--- a/gfx/cairo/libpixman/src/moz.build
>+++ b/gfx/cairo/libpixman/src/moz.build
>+# See bug 386897.
>+if CONFIG['GNU_CC'] and CONFIG['OS_TARGET'] == 'Android' and CONFIG['MOZ_OPTIMIZE']:
>+    CFLAGS += ['-O2']

Looks like the GNU_CC condition wasn't there in the Makefile - did you intend to add it? I'm not sure if it matters either way, since it's probably always true on android builds.
Attachment #8639416 - Flags: review?(mshal) → review+
(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #1)
> Created attachment 8637260 [details] [diff] [review]
> part 1 - move uses of MODULE_OPTIMIZE_FLAGS to moz.build's CFLAGS
> 
> It's not obvious to me what MODULE_OPTIMIZE_FLAGS was buying us,

*Replacing* the default optimization flags with per-module flags. BTW, does MSVC actually do the "right" thing when you pass it, say, -O -O2?
Duplicate of this bug: 1142513
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.