Fix bad caching of `~const Drop` bounds #92149

fee1-dead · 2021-12-21T05:27:09Z

Fixes #92111.

rust-highfive · 2021-12-21T05:27:13Z

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

fee1-dead · 2021-12-21T05:27:25Z

r? @oli-obk

fee1-dead · 2021-12-21T05:29:32Z

This is potentially perf-sensitive:

@bors try @rust-timer queue

rust-timer · 2021-12-21T05:29:34Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-12-21T05:29:39Z

⌛ Trying commit aaaad5b with merge 5557e81be8ea74c8d6a3785b02976c3c96ec2e4f...

bors · 2021-12-21T07:04:47Z

☀️ Try build successful - checks-actions
Build commit: 5557e81be8ea74c8d6a3785b02976c3c96ec2e4f (5557e81be8ea74c8d6a3785b02976c3c96ec2e4f)

rust-timer · 2021-12-21T07:04:49Z

Queued 5557e81be8ea74c8d6a3785b02976c3c96ec2e4f with parent 99b0799, future comparison URL.

rust-timer · 2021-12-21T08:39:16Z

Finished benchmarking commit (5557e81be8ea74c8d6a3785b02976c3c96ec2e4f): comparison url.

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

Moderate regression in instruction counts (up to 2.9% on incr-patched: println builds of regression-31157)

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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

fee1-dead · 2021-12-21T09:03:29Z

Just some thoughts: perf suggests that always checking if a trait is the Drop lang item in trait system is a bad idea. I wonder if, at some stage, we could remap T: ~const Drop bound to T: ConstDrop and kept ConstDrop private to libstd and librustc.

oli-obk · 2021-12-21T09:06:20Z

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.

oli-obk · 2021-12-21T10:51:14Z

@bors r+

bors · 2021-12-21T10:51:15Z

📌 Commit aaaad5b has been approved by oli-obk

bors · 2021-12-21T12:49:12Z

⌛ Testing commit aaaad5b with merge 8ad3c1d...

MinerSebas · 2021-12-21T13:38:10Z

src/test/ui/rfc-2632-const-trait-impl/issue-92111.rs

+// Regression test for #92111.
+//
+// The issue was that we normalize trait bounds before caching
+// results of selection. Checking that `impl NoDrop for S` requires


Suggested change

// results of selection. Checking that `impl NoDrop for S` requires

// results of selection. Checking that `impl Tr for S` requires

Nice catch! Though bors has already begun testing this PR. The fix would probably have to be separated to another PR.

bors · 2021-12-21T16:04:48Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 8ad3c1d to master...

Aaron1011 · 2021-12-21T16:17:43Z

compiler/rustc_middle/src/ty/mod.rs

@@ -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()) {


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?

No because we only need to special case ~const Drop bounds.

Yes because I have audited all usages of a method I added made for exactly this purpose. Remapping T: ~const SomeTrait to T: SomeTrait when 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

rust-timer · 2021-12-21T22:17:15Z

Finished benchmarking commit (8ad3c1d): comparison url.

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

Moderate regression in instruction counts (up to 2.6% on incr-patched: println builds of regression-31157)

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

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.

RalfJung · 2024-09-08T14:37:58Z

@fee1-dead why does this have the const-hack label? The code changed here isn't even in const fn I think?

fee1-dead · 2024-09-08T14:43:40Z

well to be more specific this should be types-hack or const-traits-hack. I suppose the scope of const-hack is for mostly library functions using an uglier version of code because the more clean version can't be done in const yet? then we can remove it from this (this is already fixed anyways)

Fix bad caching of ~const Drop bounds

aaaad5b

rust-highfive assigned michaelwoerister Dec 21, 2021

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 21, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 21, 2021

rust-highfive assigned oli-obk and unassigned michaelwoerister Dec 21, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 21, 2021

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 21, 2021

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 Dec 21, 2021

fee1-dead added the const-hack label Dec 21, 2021

MinerSebas reviewed Dec 21, 2021

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 21, 2021

bors merged commit 8ad3c1d into rust-lang:master Dec 21, 2021

rustbot added this to the 1.59.0 milestone Dec 21, 2021

Aaron1011 reviewed Dec 21, 2021

View reviewed changes

fee1-dead deleted the cache-fix branch December 21, 2021 16:21

fee1-dead mentioned this pull request Mar 13, 2022

Rename ~const Drop to ~const Destruct #94901

Merged

RalfJung removed the const-hack label Sep 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bad caching of `~const Drop` bounds #92149

Fix bad caching of `~const Drop` bounds #92149

fee1-dead commented Dec 21, 2021

rust-highfive commented Dec 21, 2021

fee1-dead commented Dec 21, 2021

fee1-dead commented Dec 21, 2021

rust-timer commented Dec 21, 2021

bors commented Dec 21, 2021

bors commented Dec 21, 2021

rust-timer commented Dec 21, 2021

rust-timer commented Dec 21, 2021

fee1-dead commented Dec 21, 2021 •

edited

Loading

oli-obk commented Dec 21, 2021

oli-obk commented Dec 21, 2021

bors commented Dec 21, 2021

bors commented Dec 21, 2021

MinerSebas Dec 21, 2021

fee1-dead Dec 21, 2021

bors commented Dec 21, 2021

Aaron1011 Dec 21, 2021

fee1-dead Dec 21, 2021

rust-timer commented Dec 21, 2021

RalfJung commented Sep 8, 2024

fee1-dead commented Sep 8, 2024

	// results of selection. Checking that `impl NoDrop for S` requires
	// results of selection. Checking that `impl Tr for S` requires

Fix bad caching of ~const Drop bounds #92149

Fix bad caching of ~const Drop bounds #92149

Conversation

fee1-dead commented Dec 21, 2021

rust-highfive commented Dec 21, 2021

fee1-dead commented Dec 21, 2021

fee1-dead commented Dec 21, 2021

rust-timer commented Dec 21, 2021

bors commented Dec 21, 2021

bors commented Dec 21, 2021

rust-timer commented Dec 21, 2021

rust-timer commented Dec 21, 2021

fee1-dead commented Dec 21, 2021 • edited Loading

oli-obk commented Dec 21, 2021

oli-obk commented Dec 21, 2021

bors commented Dec 21, 2021

bors commented Dec 21, 2021

MinerSebas Dec 21, 2021

Choose a reason for hiding this comment

fee1-dead Dec 21, 2021

Choose a reason for hiding this comment

bors commented Dec 21, 2021

Aaron1011 Dec 21, 2021

Choose a reason for hiding this comment

fee1-dead Dec 21, 2021

Choose a reason for hiding this comment

rust-timer commented Dec 21, 2021

RalfJung commented Sep 8, 2024

fee1-dead commented Sep 8, 2024

Fix bad caching of `~const Drop` bounds #92149

Fix bad caching of `~const Drop` bounds #92149

fee1-dead commented Dec 21, 2021 •

edited

Loading