Fix bad caching of ~const Drop bounds
#92149
Conversation
rustbot
added
the
T-compiler
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @oli-obk |
|
This is potentially perf-sensitive: @bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit aaaad5b with merge 5557e81be8ea74c8d6a3785b02976c3c96ec2e4f... |
|
☀️ Try build successful - checks-actions |
|
Queued 5557e81be8ea74c8d6a3785b02976c3c96ec2e4f with parent 99b0799, future comparison URL. |
|
Finished benchmarking commit (5557e81be8ea74c8d6a3785b02976c3c96ec2e4f): comparison url. Summary: This change led to moderate relevant regressions 😿 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
|
Just some thoughts: perf suggests that always checking if a trait is the |
|
The big regression is in llvm, which can't be a result of this PR. The 0.3% regressions look legit (typeck), but considering this pretty bad regression, let's take the hit. |
|
@bors r+ |
|
📌 Commit aaaad5b has been approved by |
bors
added
S-waiting-on-bors
fee1-dead
added
the
const-hack
| // Regression test for #92111. | ||
| // | ||
| // The issue was that we normalize trait bounds before caching | ||
| // results of selection. Checking that `impl NoDrop for S` requires |
There was a problem hiding this comment.
| // results of selection. Checking that `impl NoDrop for S` requires | |
| // results of selection. Checking that `impl Tr for S` requires |
There was a problem hiding this comment.
Nice catch! Though bors has already begun testing this PR. The fix would probably have to be separated to another PR.
|
☀️ Test successful - checks-actions |
| @@ -734,6 +734,15 @@ pub struct TraitPredicate<'tcx> { | |||
| pub type PolyTraitPredicate<'tcx> = ty::Binder<'tcx, TraitPredicate<'tcx>>; | |||
|
|
|||
| impl<'tcx> TraitPredicate<'tcx> { | |||
| pub fn remap_constness(&mut self, tcx: TyCtxt<'tcx>, param_env: &mut ParamEnv<'tcx>) { | |||
| if unlikely!(Some(self.trait_ref.def_id) == tcx.lang_items().drop_trait()) { | |||
There was a problem hiding this comment.
It seems kind of weird to be special-casing Drop here. Are there any other traits that need this kind of special handling? Are you sure that this method is called in all of the places where it needs to be?
There was a problem hiding this comment.
- No because we only need to special case
~const Dropbounds. - Yes because I have audited all usages of a method I added made for exactly this purpose. Remapping
T: ~const SomeTraittoT: SomeTraitwhen not in a const context.
See also https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/~const.20Drop which would be a better solution
|
Finished benchmarking commit (8ad3c1d): comparison url. Summary: This change led to moderate relevant regressions 😿 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 |
Rename `~const Drop` to `~const Destruct` r? `@oli-obk` Completely switching to `~const Destructible` would be rather complicated, so it seems best to add it for now and wait for it to be backported to beta in the next release. The rationale is to prevent complications such as rust-lang#92149 and rust-lang#94803 by introducing an entirely new trait. And `~const Destructible` reads a bit better than `~const Drop`. Name Bikesheddable.
Fixes #92111.