Don't use projection cache or candidate cache in intercrate mode #89125

Aaron1011 · 2021-09-20T19:27:46Z

It appears that just disabling the evaluation cache (in #88994)
leads to other issues involving intercrate mode caching. I suspect
that since we now always end up performing the full evaluation
in intercrate mode, we end up 'polluting' the candidate and projection
caches with results that depend on being in intercrate mode in some way.
Previously, we might have hit a cached evaluation (stored during
non-intercrate mode), and skipped doing this extra work in
intercrate mode.

The whole situation with intercrate mode caching is turning into
a mess. Ideally, we would remove intercrate mode entirely - however,
this might require waiting on Chalk.

rust-highfive · 2021-09-20T19:27:50Z

r? @matthewjasper

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

Aaron1011 · 2021-09-20T19:28:05Z

cc @rust-lang/wg-traits

jackh726 · 2021-09-20T19:33:36Z

If this is only disabling caches, the r=me with appropriate comments added

Fixes rust-lang#88969 It appears that *just* disabling the evaluation cache (in rust-lang#88994) leads to other issues involving intercrate mode caching. I suspect that since we now always end up performing the full evaluation in intercrate mode, we end up 'polluting' the candidate and projection caches with results that depend on being in intercrate mode in some way. Previously, we might have hit a cached evaluation (stored during non-intercrate mode), and skipped doing this extra work in intercrate mode. The whole situation with intercrate mode caching is turning into a mess. Ideally, we would remove intercrate mode entirely - however, this might require waiting on Chalk.

Aaron1011 · 2021-09-20T19:37:52Z

@bors r=jackh726

bors · 2021-09-20T19:37:53Z

📌 Commit 6dbb9d4 has been approved by jackh726

Aaron1011 · 2021-09-20T20:01:41Z

@bors try @rust-timer queue

rust-timer · 2021-09-20T20:01:42Z

Awaiting bors try build completion.

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

bors · 2021-09-20T20:01:42Z

🙅 Please do not try after a pull request has been r+ed. If you need to try, unapprove (r-) it first.

Aaron1011 · 2021-09-20T20:01:53Z

@bors r-

Aaron1011 · 2021-09-20T20:02:00Z

@bors try

bors · 2021-09-20T20:02:08Z

⌛ Trying commit 6dbb9d4 with merge 8d03c9c947571bf681dc63b203b921ee9f76eed5...

bors · 2021-09-20T21:20:21Z

☀️ Try build successful - checks-actions
Build commit: 8d03c9c947571bf681dc63b203b921ee9f76eed5 (8d03c9c947571bf681dc63b203b921ee9f76eed5)

bors · 2021-09-20T21:20:27Z

☀️ Try build successful - checks-actions
Build commit: 8d03c9c947571bf681dc63b203b921ee9f76eed5 (8d03c9c947571bf681dc63b203b921ee9f76eed5)

rust-timer · 2021-09-20T21:21:38Z

Queued 8d03c9c947571bf681dc63b203b921ee9f76eed5 with parent 3bb9eec, future comparison URL.

rust-timer · 2021-09-20T23:47:10Z

Finished benchmarking commit (8d03c9c947571bf681dc63b203b921ee9f76eed5): comparison url.

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

Moderate regression in instruction counts (up to 1.0% on incr-full builds of diesel)

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

Aaron1011 · 2021-09-21T00:09:09Z

This is a correctness fix, so we'll need to take the performance hit (at least until we can come up with a better solution).

@bors r=jackh726

bors · 2021-09-21T00:09:10Z

📌 Commit 6dbb9d4 has been approved by jackh726

nikomatsakis

(Also, do we have a test case?)

nikomatsakis · 2021-09-21T10:20:55Z

compiler/rustc_trait_selection/src/traits/project.rs

+ // Don't use the projection cache in intercrate mode -
+ // the `infcx` may be re-used between intercrate in non-intercrate
+ // mode, which could lead to using incorrect cache results.
+ let use_cache = !selcx.is_intercrate();


Probably the right answer is that the cache key incorporates the environment, and intercrate mode is part of the environment (this is how chalk approaches it).

bors · 2021-09-21T13:25:17Z

⌛ Testing commit 6dbb9d4 with merge 7743c9f...

bors · 2021-09-21T16:13:19Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 7743c9f to master...

apiraino · 2021-09-29T18:07:39Z

@Aaron1011 should this be nominated for beta-backport?

apiraino · 2021-09-30T14:19:40Z

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

Add regression test for issues rust-lang#88969 and rust-lang#89119 This adds a regression test to complete rust-lang#89125, and thus for issues rust-lang#88969 and rust-lang#89119, which needed a test. Used with multiple crates, [this](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f665e7e882059157e0f86cfb09c47187) minimized from `zvariant-2.8.0` reproduces the error on `nightly-2021-09-19`. The test in this PR fails on master if the commit 6dbb9d4 from rust-lang#89125 is reverted, and passes otherwise since it's now fixed. r? `@Aaron1011`

…jackh726 Don't use projection cache or candidate cache in intercrate mode Fixes rust-lang#88969 It appears that *just* disabling the evaluation cache (in rust-lang#88994) leads to other issues involving intercrate mode caching. I suspect that since we now always end up performing the full evaluation in intercrate mode, we end up 'polluting' the candidate and projection caches with results that depend on being in intercrate mode in some way. Previously, we might have hit a cached evaluation (stored during non-intercrate mode), and skipped doing this extra work in intercrate mode. The whole situation with intercrate mode caching is turning into a mess. Ideally, we would remove intercrate mode entirely - however, this might require waiting on Chalk.

[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)

rust-highfive assigned matthewjasper Sep 20, 2021

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

Aaron1011 mentioned this pull request Sep 20, 2021

zvariant fails to compile on latest nightly: size cannot be statically determined #89119

Closed

Aaron1011 force-pushed the remove-intercrate-cache branch from 8b70c8f to 6dbb9d4 Compare September 20, 2021 19:37

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 Sep 20, 2021

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

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 20, 2021

rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 20, 2021

nikomatsakis reviewed Sep 21, 2021

View reviewed changes

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

bors merged commit 7743c9f into rust-lang:master Sep 21, 2021

rustbot added this to the 1.57.0 milestone Sep 21, 2021

chipbuster mentioned this pull request Sep 22, 2021

feat: Add pulumi module starship/starship#3055

Merged

5 tasks

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 29, 2021

Aaron1011 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 29, 2021

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 30, 2021

lqd mentioned this pull request Sep 30, 2021

Add regression test for issues #88969 and #89119 #89412

Merged

ehuss mentioned this pull request Oct 4, 2021

[beta] Beta rollup #89527

Merged

ehuss removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 4, 2021

Mark-Simulacrum modified the milestones: 1.57.0, 1.56.0 Oct 6, 2021

lqd mentioned this pull request May 10, 2022

Try to speed up #[derive(ValidGrouping)] diesel-rs/diesel#3163

Closed

lqd mentioned this pull request Oct 21, 2023

Cargo check runs forever and memory usage grows unboundedly #116914

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use projection cache or candidate cache in intercrate mode #89125

Don't use projection cache or candidate cache in intercrate mode #89125

Aaron1011 commented Sep 20, 2021

rust-highfive commented Sep 20, 2021

Aaron1011 commented Sep 20, 2021

jackh726 commented Sep 20, 2021

Aaron1011 commented Sep 20, 2021

bors commented Sep 20, 2021

Aaron1011 commented Sep 20, 2021

rust-timer commented Sep 20, 2021

bors commented Sep 20, 2021

Aaron1011 commented Sep 20, 2021

Aaron1011 commented Sep 20, 2021

bors commented Sep 20, 2021

bors commented Sep 20, 2021

bors commented Sep 20, 2021

rust-timer commented Sep 20, 2021

rust-timer commented Sep 20, 2021

Aaron1011 commented Sep 21, 2021

bors commented Sep 21, 2021

nikomatsakis left a comment

nikomatsakis Sep 21, 2021

bors commented Sep 21, 2021

bors commented Sep 21, 2021

apiraino commented Sep 29, 2021

apiraino commented Sep 30, 2021

Don't use projection cache or candidate cache in intercrate mode #89125

Don't use projection cache or candidate cache in intercrate mode #89125

Conversation

Aaron1011 commented Sep 20, 2021

rust-highfive commented Sep 20, 2021

Aaron1011 commented Sep 20, 2021

jackh726 commented Sep 20, 2021

Aaron1011 commented Sep 20, 2021

bors commented Sep 20, 2021

Aaron1011 commented Sep 20, 2021

rust-timer commented Sep 20, 2021

bors commented Sep 20, 2021

Aaron1011 commented Sep 20, 2021

Aaron1011 commented Sep 20, 2021

bors commented Sep 20, 2021

bors commented Sep 20, 2021

bors commented Sep 20, 2021

rust-timer commented Sep 20, 2021

rust-timer commented Sep 20, 2021

Aaron1011 commented Sep 21, 2021

bors commented Sep 21, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Sep 21, 2021

Choose a reason for hiding this comment

bors commented Sep 21, 2021

bors commented Sep 21, 2021

apiraino commented Sep 29, 2021

apiraino commented Sep 30, 2021