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

RESOLVED FIXED in mozilla9

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: espindola, Assigned: khuey)

Tracking

Trunk
mozilla9
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

5.26 KB, patch
ted
: review+
Details | Diff | Splinter Review
1.80 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.69 KB, patch
Details | Diff | Splinter Review
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.
Created attachment 554502 [details] [diff] [review]
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
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #554502 - Flags: review?(khuey)
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.
Attachment #554502 - Flags: review?(khuey) → review+
Landed on inbound.
Duplicate of this bug: 464328
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Looks like we lost -fstrict-aliasing -fno-stack-protector

Comment 7

6 years ago
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).
Sorry for the problems. New try:
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=f408343c8225
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.
We could, but why?
Having strict aliasing is probably a good thing.
Created attachment 554632 [details] [diff] [review]
new patch
Attachment #554502 - Attachment is obsolete: true
Attachment #554632 - Flags: review?(khuey)
Comment on attachment 554632 [details] [diff] [review]
new patch

If we want to drop -fstrict-aliasing, lets do that in a separate bug.
Attachment #554632 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12)
> If we want to drop -fstrict-aliasing, lets do that in a separate bug.

That's bug 414641
(In reply to Mike Hommey [:glandium] from comment #13)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
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.
As far as I can tell, linux and android already set -fomit-frame-pointer in the configure.
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.
Oops, in comment 17 I meant MODULE_OPTIMIZE_FLAGS, not MOZ_OPTIMIZE_FLAGS.
Created attachment 554681 [details] [diff] [review]
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
Attachment #554681 - Flags: review?(wmccloskey)
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.
Attachment #554681 - Flags: review?(wmccloskey) → review?(khuey)
Created attachment 554691 [details] [diff] [review]
Add strict aliasing back, in pgo builds too :-(
Attachment #554681 - Attachment is obsolete: true
Attachment #554681 - Flags: review?(khuey)
Attachment #554691 - Flags: review?(khuey)
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?
http://hg.mozilla.org/mozilla-central/rev/53f198cfbf47
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Version: unspecified → Trunk
This changes the optimization flags for JS, and regressed performance on Windows and Android.
Created attachment 554869 [details] [diff] [review]
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 :-/
Attachment #554632 - Attachment is obsolete: true
Attachment #554691 - Attachment is obsolete: true
Attachment #554691 - Flags: review?(khuey)
Attachment #554869 - Flags: review?(ted.mielczarek)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?)
Attachment #554869 - Flags: review?(ted.mielczarek) → review+
Assignee: respindola → khuey
(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.
http://hg.mozilla.org/mozilla-central/rev/00d1dbeed5e4
http://hg.mozilla.org/mozilla-central/rev/671001912407

Will be watching carefully for Talos regressions.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
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)
}
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.
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.
That's plausible.  I'll give that a shot.
Created attachment 556803 [details] [diff] [review]
enable strict aliasing in js/src

http://hg.mozilla.org/try/rev/968b1ff08ce3
Attachment #556803 - Flags: review?(khuey)
Also could be due to dropping -fno-stack-protector.  Unlike the strict aliasing change, that was linux only.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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™.
Looks like I'm partially wrong.

changeset:   64354:663a3afb238e
user:        Luke Wagner <lw@mozilla.com>
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
Attachment #556803 - Flags: review?(khuey) → review+
Created attachment 556807 [details] [diff] [review]
really remove -fno-strict-aliasing from js/src

Try (with talos) is running at:

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=44d91488601a
Attachment #556803 - Attachment is obsolete: true
Attachment #556807 - Flags: review?(khuey)
Comment on attachment 556807 [details] [diff] [review]
really remove -fno-strict-aliasing from js/src

Don't think you meant to upload this ...
Attachment #556807 - Flags: review?(khuey) → review-
Created attachment 556808 [details] [diff] [review]
now in plain text :-(

Sorry about the noise, the previous patch was in html.
Attachment #556807 - Attachment is obsolete: true
Attachment #556808 - Flags: review?(khuey)
Attachment #556808 - Flags: review?(khuey) → review+
Created attachment 556809 [details] [diff] [review]
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.
Attachment #556809 - Flags: review?(khuey)
Lets skip the stack protector stuff for now and just do aliasing.

Comment 42

6 years ago
(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.
Are talus runs on Try and Inbound comparable? I got 346 in

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=e5bf31f2b732
This might depend on whether you are comparing runs on either side of the TI merge, which I think doubled the dromaeo_v8 score.
Attachment #556809 - Flags: review?(khuey)
http://hg.mozilla.org/mozilla-central/rev/22cc14edadaa

please reopen bugs or create followup bugs, pushing patches on resolved fixed bugs is evil :)
You need to log in before you can comment on or make changes to this bug.