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

Disable drop range tracking in generators #93165

Merged
merged 3 commits into from
Jan 23, 2022

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Jan 21, 2022

Generator drop tracking caused an ICE for generators involving the Never type (Issue #93161). Since this breaks a test case with miri, we temporarily disable drop tracking so miri is unblocked while we properly fix the issue.

Generator drop tracking caused an ICE for generators involving the Never
type (Issue rust-lang#93161). Since this breaks miri, we temporarily disable drop
tracking so miri is unblocked while we properly fix the issue.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 21, 2022
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(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 Jan 21, 2022
@eholk
Copy link
Contributor Author

eholk commented Jan 21, 2022

r? @RalfJung

I went ahead and put this PR up in case we need a quick fix to the issue, since the original PR is large and would be hard to revert. I'll leave it up to you, @RalfJung, if we want to merge it. I'll look into a proper fix today. I suspect it will be pretty straightforward, but I haven't looked at it yet.

@RalfJung
Copy link
Member

FWIW this does not just break Miri, it breaks rustc as well. rustc just does not have a testcase that triggers the ICE, it seems. But I see you added one. :)

So, disabling that if is safe, the rest of the generator pass will still work correctly? I don't know this code at all so I cannot really evaluate this. But generally I agree disabling this until the ICE is fixed makes sense.

Cc @nikomatsakis who reviewed #91032

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 21, 2022

📌 Commit ead84d0 has been approved by nikomatsakis

@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 Jan 21, 2022
@eholk
Copy link
Contributor Author

eholk commented Jan 21, 2022

I updated the PR description to mention that this breaks one of the miri test cases, to hopefully make it clearer that rustc breaks in the same way.

Disabling the if is safe because the if only prunes types from appearing in the generator. It's okay to over-approximate, but under-approximating is a problem (that's why this test case is failing). Disabling the if still does all the analysis to determine if we can prune the type, but doesn't actually do anything with it, so we'll have the same behavior even if we do some extra work.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2022
…ng, r=nikomatsakis

Disable drop range tracking in generators

Generator drop tracking caused an ICE for generators involving the Never type (Issue rust-lang#93161). Since this breaks a test case with miri, we temporarily disable drop tracking so miri is unblocked while we properly fix the issue.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2022
…ng, r=nikomatsakis

Disable drop range tracking in generators

Generator drop tracking caused an ICE for generators involving the Never type (Issue rust-lang#93161). Since this breaks a test case with miri, we temporarily disable drop tracking so miri is unblocked while we properly fix the issue.
@matthiaskrgr
Copy link
Member

@bors p=1 since we are getting several reports about the ICE already

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@RalfJung
Copy link
Member

@bors retry Could not resolve host: ci-mirrors.rust-lang.org

@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 Jan 23, 2022
@bors
Copy link
Contributor

bors commented Jan 23, 2022

⌛ Testing commit ead84d0 with merge d13e8dd...

@bors
Copy link
Contributor

bors commented Jan 23, 2022

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing d13e8dd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 23, 2022
@bors bors merged commit d13e8dd into rust-lang:master Jan 23, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 23, 2022
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #93165!

Tested on commit d13e8dd.
Direct link to PR: #93165

🎉 miri on linux: test-fail → test-pass (cc @RalfJung @oli-obk @eddyb).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jan 23, 2022
Tested on commit rust-lang/rust@d13e8dd.
Direct link to PR: <rust-lang/rust#93165>

🎉 miri on linux: test-fail → test-pass (cc @RalfJung @oli-obk @eddyb).
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d13e8dd): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

  • Very large regression in instruction counts (up to 879.2% on full builds of deeply-nested-async check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Jan 23, 2022
@lqd
Copy link
Member

lqd commented Jan 23, 2022

At least these perf results make clear that #91032 was indeed the source of the -89.5% perf results for the rollup in which it landed.

This PR soft reverts it, negating the original change.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jan 23, 2022
eholk added a commit to eholk/rust that referenced this pull request Jan 25, 2022
The previous PR, rust-lang#93165, still performed the drop range analysis
despite ignoring the results. Unfortunately, there were ICEs in
the analysis as well, so some packages failed to build (see the
issue rust-lang#93197 for an example). This change further disables the
analysis and just provides dummy results in that case.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 1, 2022
…pnkfelix

Disable drop range analysis

The previous PR, rust-lang#93165, still performed the drop range analysis despite ignoring the results. Unfortunately, there were ICEs in the analysis as well, so some packages failed to build (see the issue rust-lang#93197 for an example). This change further disables the analysis and just provides dummy results in that case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.