Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Set opt-level to 3" #51165

Merged
merged 2 commits into from
May 29, 2018
Merged

Revert "Set opt-level to 3" #51165

merged 2 commits into from
May 29, 2018

Conversation

SimonSapin
Copy link
Contributor

This reverts commit aad9840.

Level 3 (possibly indirectly, the underlying bug might be in XCode’s linker) causes unit tests to sefault when compiled with some versions of XCode: #50867

It also appears to cause some segfaults on Windows: #50329 (comment), and regressions in some rustc performance benchmarks: #50329 (comment)

This reverts commit aad9840.

Level 3 (possibly indirectly, the underlying bug might be in XCode’s linker)
causes unit tests to sefault when compiled with some versions of XCode:
rust-lang#50867

It also appears to cause some segfaults on Windows:
rust-lang#50329 (comment)

… and regressions in some rustc performance benchmarks:
rust-lang#50329 (comment)
@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2018
@SimonSapin
Copy link
Contributor Author

r? @alexcrichton, CC @Zoxc

@SimonSapin
Copy link
Contributor Author

Unfortunately, adding a test so that #50867 doesn’t regress again is tricky since the Travis-CI config uses newer XCode than versions that are affected according to #50867 (comment).

@SimonSapin
Copy link
Contributor Author

SimonSapin commented May 29, 2018

@bors p=1
#50867 blocks upgrading Nightly in Servo.

src/Cargo.toml Outdated
@@ -40,6 +40,14 @@ members = [
"tools/rls/test_data/workspace_symbol",
]

# Curiously, compiletest will segfault if compiled with opt-level=3 on 64-bit
# MSVC when running the compile-fail test suite when a should-fail test panics.
# But hey if this is removed and it gets past the bots, sounds good to me.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the comment to point to #50867. MSVC is no longer the cause here keeping us on opt-level = 2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 29, 2018

📌 Commit 5067d2f has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2018
@bors
Copy link
Contributor

bors commented May 29, 2018

⌛ Testing commit 5067d2f with merge 524ad9b...

bors added a commit that referenced this pull request May 29, 2018
Revert "Set opt-level to 3"

This reverts commit aad9840.

Level 3 (possibly indirectly, the underlying bug might be in XCode’s linker) causes unit tests to sefault when compiled with some versions of XCode: #50867

It also appears to cause some segfaults on Windows: #50329 (comment), and regressions in some rustc performance benchmarks: #50329 (comment)
@bors
Copy link
Contributor

bors commented May 29, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 524ad9b to master...

@nnethercote
Copy link
Contributor

kennytm added a commit to kennytm/rust that referenced this pull request Jul 10, 2018
bors added a commit that referenced this pull request Jul 11, 2018
Set opt-level = 3 the third time.

This PR reverts #51165 (set -O2 to fix #50867),
which reverted #50329 (set -O3),
which was second attempt of #48204 (set -O3, closed due to Windows segfault that is fixed now),
which reverted #42123 (set -O2 to fix spurious Windows segfaults),
which reverted #41967 (set -O3).

Last time we've found that setting -O3 regressed the wall time of NLL (#50329 (comment)), so we may need another perf run to confirm. I'd like to check this *after* the LLVM 7 upgrade #51966 has been merged, so marking this as <kbd>S-blocked</kbd> for now.
kennytm added a commit to kennytm/rust that referenced this pull request Jul 12, 2018
bors added a commit that referenced this pull request Jul 14, 2018
Set opt-level = 3 the third time.

This PR reverts #51165 (set -O2 for fixing #50867),
which reverted #50329 (set -O3),
which was second attempt of #48204 (set -O3, closed due to Windows segfault that is fixed now),
which reverted #42123 (set -O2 to fix spurious Windows segfaults),
which reverted #41967 (set -O3).

Since we have found the root cause of #50867, this optimization could be tried again.

Last time we've found that setting -O3 regressed the wall time of NLL (#50329 (comment)), so we may need another perf run to confirm. I'd like to check this *after* the LLVM 7 upgrade #51966 has been merged, so marking this as <kbd>S-blocked</kbd> for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants