Closed
Bug 680515
Opened 13 years ago
Closed 13 years ago
Setting MOZ_OPTIMIZE_FLAGS in js/src/configure.in is not effective
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla9
People
(Reporter: espindola, Assigned: khuey)
References
Details
Attachments
(3 files, 6 obsolete files)
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.
Reporter | ||
Comment 1•13 years ago
|
||
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 | ||
Comment 2•13 years ago
|
||
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+
Comment 3•13 years ago
|
||
Landed on inbound.
Comment 5•13 years ago
|
||
(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.
Assignee | ||
Comment 6•13 years ago
|
||
Looks like we lost -fstrict-aliasing -fno-stack-protector
Comment 7•13 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).
Reporter | ||
Comment 8•13 years ago
|
||
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.
Reporter | ||
Comment 10•13 years ago
|
||
We could, but why? Having strict aliasing is probably a good thing.
Reporter | ||
Comment 11•13 years ago
|
||
Attachment #554502 -
Attachment is obsolete: true
Attachment #554632 -
Flags: review?(khuey)
Assignee | ||
Comment 12•13 years ago
|
||
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+
Comment 13•13 years ago
|
||
(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
Assignee | ||
Comment 14•13 years ago
|
||
(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.
Reporter | ||
Comment 16•13 years ago
|
||
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.
Reporter | ||
Comment 19•13 years ago
|
||
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)
Reporter | ||
Comment 21•13 years ago
|
||
Attachment #554681 -
Attachment is obsolete: true
Attachment #554681 -
Flags: review?(khuey)
Attachment #554691 -
Flags: review?(khuey)
Comment 22•13 years ago
|
||
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•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/53f198cfbf47
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Version: unspecified → Trunk
Comment 24•13 years ago
|
||
This changes the optimization flags for JS, and regressed performance on Windows and Android.
Assignee | ||
Comment 25•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 26•13 years ago
|
||
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+
Updated•13 years ago
|
Assignee: respindola → khuey
Assignee | ||
Comment 27•13 years ago
|
||
(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.
Assignee | ||
Comment 28•13 years ago
|
||
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
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 29•13 years ago
|
||
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) }
Assignee | ||
Comment 30•13 years ago
|
||
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.
Reporter | ||
Comment 31•13 years ago
|
||
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.
Assignee | ||
Comment 32•13 years ago
|
||
That's plausible. I'll give that a shot.
Reporter | ||
Comment 33•13 years ago
|
||
http://hg.mozilla.org/try/rev/968b1ff08ce3
Attachment #556803 -
Flags: review?(khuey)
Assignee | ||
Comment 34•13 years ago
|
||
Also could be due to dropping -fno-stack-protector. Unlike the strict aliasing change, that was linux only.
Comment 35•13 years ago
|
||
(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™.
Comment 36•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Attachment #556803 -
Flags: review?(khuey) → review+
Reporter | ||
Comment 37•13 years ago
|
||
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)
Assignee | ||
Comment 38•13 years ago
|
||
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-
Reporter | ||
Comment 39•13 years ago
|
||
Sorry about the noise, the previous patch was in html.
Attachment #556807 -
Attachment is obsolete: true
Attachment #556808 -
Flags: review?(khuey)
Assignee | ||
Updated•13 years ago
|
Attachment #556808 -
Flags: review?(khuey) → review+
Reporter | ||
Comment 40•13 years ago
|
||
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)
Assignee | ||
Comment 41•13 years ago
|
||
Lets skip the stack protector stuff for now and just do aliasing.
Comment 42•13 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.
Reporter | ||
Comment 43•13 years ago
|
||
Are talus runs on Try and Inbound comparable? I got 346 in http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=e5bf31f2b732
Comment 44•13 years ago
|
||
This might depend on whether you are comparing runs on either side of the TI merge, which I think doubled the dromaeo_v8 score.
Assignee | ||
Updated•13 years ago
|
Attachment #556809 -
Flags: review?(khuey)
Comment 45•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/22cc14edadaa please reopen bugs or create followup bugs, pushing patches on resolved fixed bugs is evil :)
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•