Don't normalize opaque types with escaping late-bound regions #89285
Conversation
Turns out, this has some really bad perf implications in large types (issue rust-lang#88862). While we technically can handle them fine, it doesn't change test output either way. For now, revert with an added benchmark. Future attempts to change this back will have to consider perf.
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue rust-lang/rustc-perf#1033 is now merged and this should pick it up (there's a pending master commit in queue right now, which'll get benchmarked first, so it should have the parent in its results). |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit a84e3fa with merge 4c8c238b723c4afdf0522ee26708dc84b917d544... |
|
☀️ Try build successful - checks-actions |
|
Queued 4c8c238b723c4afdf0522ee26708dc84b917d544 with parent c09d637, future comparison URL. |
|
Finished benchmarking commit (4c8c238b723c4afdf0522ee26708dc84b917d544): comparison url. Summary: This change led to very large relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
rustbot
added
perf-regression
|
This is a targeted perf fix for the one benchmark. The slight regression for others is probably expected, but not too bad. |
jackh726
added
the
perf-regression-triaged
|
The only other question here is whether we want to backport to beta or not. I'm not sure how widespread the actually issue is, but on balance, this is a very small change and effectively an extremely partial revert. I'm going to go ahead and mark as beta-nominated; I'll let the team decide if it's worth backporting. |
jackh726
added
the
beta-nominated
|
@bors r+ Seems ok as a stopgap measure |
|
📌 Commit a84e3fa has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (2b6ed3b): comparison url. Summary: This change led to very large relevant mixed results 🤷 in compiler performance.
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 |
apiraino
added
the
T-compiler
rustbot
added
the
beta-accepted
Don't normalize opaque types with escaping late-bound regions Fixes rust-lang#88862 Turns out, this has some really bad perf implications in large types (issue rust-lang#88862). While we technically can handle them fine, it doesn't change test output either way. For now, revert with an added benchmark. Future attempts to change this back will have to consider perf. Needs a perf run once rust-lang/rustc-perf#1033 is merged r? `@nikomatsakis`
ehuss
removed
the
beta-nominated
[beta] Beta rollup * Fix WinUWP std compilation errors due to I/O safety rust-lang#88587 * Disable RemoveZsts in generators to avoid query cycles rust-lang#88979 * Disable the evaluation cache when in intercrate mode rust-lang#88994 * Fix linting when trailing macro expands to a trailing semi rust-lang#88996 * Don't use projection cache or candidate cache in intercrate mode rust-lang#89125 * 2229: Mark insignificant dtor in stdlib rust-lang#89144 * Temporarily rename int_roundings functions to avoid conflicts rust-lang#89184 * [rfc 2229] Drop fully captured upvars in the same order as the regular drop code rust-lang#89208 * Use the correct edition for syntax highlighting doctests rust-lang#89277 * Don't normalize opaque types with escaping late-bound regions rust-lang#89285 * Update Let's Encrypt ROOT CA certificate in dist-(i686|x86_64)-linux docker images rust-lang#89486 Cargo update: * - [beta] 1.56 backports (rust-lang/cargo#9958) * - [beta] Revert "When a dependency does not have a version, git or path… (rust-lang/cargo#9912) * - [beta] Fix rustc --profile=dev unstable check. (rust-lang/cargo#9901)
…ound-vars, r=lcnr Normalize opaques w/ bound vars First, we reenable normalization of opaque types with escaping late bound regions to fix rust-lang/miri#2433. This essentially reverts rust-lang#89285. Second, we mitigate the perf regression found in rust-lang#88862 by simplifying the way that we relate (sub and eq) GeneratorWitness types. This relies on the fact that we construct these GeneratorWitness types somewhat particularly (with all free regions found in the witness types replaced with late bound regions) -- but those bound regions really should be treated as existential regions, not universal ones. Those two facts leads me to believe that we do not need to use the full `higher_ranked_sub` machinery to relate two generator witnesses. I'm pretty confident that this is correct, but I'm glad to discuss this further.
Fixes #88862
Turns out, this has some really bad perf implications in large types (issue #88862). While we technically can handle them fine, it doesn't change test output either way. For now, revert with an added benchmark. Future attempts to change this back will have to consider perf.
Needs a perf run once rust-lang/rustc-perf#1033 is merged
r? @nikomatsakis