680515 - Setting MOZ_OPTIMIZE_FLAGS in js/src/configure.in is not effective

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

When MODULE_OPTIMIZE_FLAGS is defined, it is used instead of MOZ_OPTIMIZE_FLAGS. There many places in js/src/configure.in that set MOZ_OPTIMIZE_FLAGS. Those are probably bugs.

khuey suggesting just dropping MODULE_OPTIMIZE_FLAGS from js/src, I am testing such a patch.

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: patch I am testing

This fixes the particular problem we were having (-Qy not being used when building js). The try is at

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=d1b64105ac11

Comment 2

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

Comment on attachment 554502 [details] [diff] [review]
patch I am testing

I'm ok with ripping this stuff out ... if we break something it's going to be icc or Solaris and those people can move the logic into configure.

Comment 3

[(no longer active)]

Landed on inbound.

Comment 5

[Phil Ringnalda (:philor)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> I'm ok with ripping this stuff out ... if we break something it's going to
> be icc or Solaris and those people can move the logic into configure.

Nope, it's going to be OS X, and those people can back you out in http://hg.mozilla.org/integration/mozilla-inbound/rev/8f1ad97f4bc2.

10.5 opt, pretty much instant crash in every test suite. 10.6, tests were fine, but 62 jit_test failures.

Comment 6

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

Looks like we lost -fstrict-aliasing -fno-stack-protector

Comment 7

[Luke Wagner [:luke]]

In case you didn't already see, the -fno-stack-protector prevents some pretty lame reappearing compiler bugs that cause js::Interpret to crash (see bug 644250).

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Sorry for the problems. New try:
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=f408343c8225

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

As long as this is going to be MacOS-only, could we take out -fstrict-aliasing? Only -fno-stack-protector is needed to eliminate the crashes.

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

We could, but why?
Having strict aliasing is probably a good thing.

Comment 11

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: new patch

Comment 12

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

Comment on attachment 554632 [details] [diff] [review]
new patch

If we want to drop -fstrict-aliasing, lets do that in a separate bug.

Comment 13

[Mike Hommey [:glandium]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)
> If we want to drop -fstrict-aliasing, lets do that in a separate bug.

That's bug 414641

Comment 14

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

(In reply to Mike Hommey [:glandium] from comment #13)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)
> > If we want to drop -fstrict-aliasing, lets do that in a separate bug.
> 
> That's bug 414641

Well that's to turn it on for the browser.  Bill is talking about turning it off for js.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

As far as I can tell, this patch turns it off on every platform but MacOS. That just doesn't make sense to me. I think we should either enable it on all platforms or disable it on all platforms.

Also, bug 657806 is about disabling it for the JS engine.

Comment 16

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

As far as I can tell, linux and android already set -fomit-frame-pointer in the configure.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry, I was talking about -fstrict-aliasing. In fact, the configure script quite explicitly uses -fno-strict-aliasing on all GNU platforms. The only reason we got strict aliasing before was because MOZ_OPTIMIZE_FLAGS appears last. So the command line looked like:
  g++ ... -fno-strict-aliasing ... -fstrict-aliasing ... file.cpp
           ^ from configure         ^ from MOZ_OPTIMIZE_FLAGS
The second option was overriding the first.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oops, in comment 17 I meant MODULE_OPTIMIZE_FLAGS, not MOZ_OPTIMIZE_FLAGS.

Comment 19

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Add strict aliasing back

You are right, the attached patch adds -fstrict-aliasing back.

I did the change in the Makefile since I am investigating how we could integrate configure and js/src/configure, but I can change configure if you prefer.

Try at:
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=b1dd998cb18c

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 554681 [details] [diff] [review]
Add strict aliasing back

I'm not a good person to review this. I don't know much about our build system.

Comment 21

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Add strict aliasing back, in pgo builds too :-(

Comment 22

[Makoto Kato [:m_kato]]

MSVC target, optimize flag seems to be modified from -O2 to -O1 by this fix.  Should you change MOZ_OPTIMIZE_FLAGS for *-ming* to -O2 instead of -O1 in js/src/configure.in?

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/53f198cfbf47

Comment 24

[Matt Brubeck (:mbrubeck)]

This changes the optimization flags for JS, and regressed performance on Windows and Android.

Comment 25

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

Attachment: Patch

I had to back this out again because it caused perf regressions on Android.

http://hg.mozilla.org/mozilla-central/rev/3d32e28b81a0
http://hg.mozilla.org/mozilla-central/rev/c9f050fe21a3

I went through and transplanted the flags from the Makefile to configure, and dropped -fstrict-aliasing.  I think I preserved everything (except the icc flags), but would like a second pair of eyes.

Sorry about all this Rafael, I should have exercised more diligence when reviewing this :-/

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 554869 [details] [diff] [review]
Patch

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

Thanks for the cleanup, I've wanted this to get done forever.

::: js/src/configure.in
@@ +2163,2 @@
>      fi
> +    MOZ_OPTIMIZE_FLAGS="-O3 -freorder-blocks -fno-reorder-functions $MOZ_FRAMEPTR_FLAGS"

Do we really want -O3 on Android? (I guess that's what we've been shipping with, but do we *really* want it?)

Comment 27

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

(In reply to Ted Mielczarek [:ted, :luser] from comment #26)
> Do we really want -O3 on Android? (I guess that's what we've been shipping
> with, but do we *really* want it?)

I don't know.  I'll file a followup to investigate.

Comment 28

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

http://hg.mozilla.org/mozilla-central/rev/00d1dbeed5e4
http://hg.mozilla.org/mozilla-central/rev/671001912407

Will be watching carefully for Talos regressions.

Comment 29

[Ed Morley [:emorley]]

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/k-SVzslmT5s
{
Dromaeo (V8) decrease 4.31% on Linux x64 Mozilla-Inbound
Previous: avg 172.679 stddev 1.739 of 30 runs up to revision a12d726a5868
    New     : avg 165.239 stddev 1.545 of 5 runs since revision ca5a3569462d
    Change  : -7.440 (4.31% / z=4.279)
}


https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/KRVrqBg_qPs
{
Dromaeo (V8) decrease 4.39% on Linux x64 Firefox
    Previous: avg 174.054 stddev 1.437 of 30 runs up to revision 6c8a909977d3
    New     : avg 166.406 stddev 0.525 of 5 runs since revision 0ac24f429e24
    Change  : -7.648 (4.39% / z=5.322)

Dromaeo (String/Array/Eval/Regex) decrease 3.69% on Linux Firefox
    Previous: avg 693.361 stddev 5.866 of 30 runs up to revision 6c8a909977d3
    New     : avg 667.757 stddev 3.319 of 5 runs since revision 0ac24f429e24
    Change  : -25.604 (3.69% / z=4.365)
}

Comment 30

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

The thing I don't understand about the Talos regressions is that if they were due to this I'd expect them to be across the board, not in a few isolated tests.

Comment 31

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

This would be because of disabling strict aliasing in http://hg.mozilla.org/mozilla-central/rev/00d1dbeed5e4 I assume? I would suggest enabling it back to see if it is fixed.

Comment 32

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

That's plausible.  I'll give that a shot.

Comment 33

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: enable strict aliasing in js/src

http://hg.mozilla.org/try/rev/968b1ff08ce3

Comment 34

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

Also could be due to dropping -fno-stack-protector.  Unlike the strict aliasing change, that was linux only.

Comment 35

[Mike Hommey [:glandium]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #34)
> Also could be due to dropping -fno-stack-protector.  Unlike the strict
> aliasing change, that was linux only.

Unlikely, that one has no meaning unless you do enable -fstack-protector. And IIRC it was added because some distro default to that and that was breaking the js engine. Which means removing it is BAD™.

Comment 36

[Mike Hommey [:glandium]]

Looks like I'm partially wrong.

changeset:   64354:663a3afb238e
user:        Luke Wagner <[e-mail]>
date:        Wed Mar 23 00:39:59 2011 -0700
summary:     Mac crashes from cf1850399b0b seem to be in bogo regalloc for -fstack-protector stack guard.  Try turning it off temporarily to see if that fixes the problem

Comment 37

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: really remove -fno-strict-aliasing from js/src

Try (with talos) is running at:

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=44d91488601a

Comment 38

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

Comment on attachment 556807 [details] [diff] [review]
really remove -fno-strict-aliasing from js/src

Don't think you meant to upload this ...

Comment 39

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: now in plain text :-(

Sorry about the noise, the previous patch was in html.

Comment 40

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: enable strict aliasing and add -fno-stack-protector

Try running at http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=e5bf31f2b732

Let me know which of these patches you like best.

Comment 41

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

Lets skip the stack protector stuff for now and just do aliasing.

Comment 42

[Luke Wagner [:luke]]

(In reply to Mike Hommey [:glandium] from comment #35)
> And IIRC it was added because some distro default to that and that was
> breaking the js engine. Which means removing it is BAD™.

'twas OS X.  bug 644250.

Comment 43

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Are talus runs on Try and Inbound comparable? I got 346 in

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=e5bf31f2b732

Comment 44

[Brian Hackett [Laid off!]]

This might depend on whether you are comparing runs on either side of the TI merge, which I think doubled the dromaeo_v8 score.

Comment 45

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/22cc14edadaa

please reopen bugs or create followup bugs, pushing patches on resolved fixed bugs is evil :)