Disable the evaluation cache when in intercrate mode #88994
Conversation
It's possible to use the same `InferCtxt` with both an intercrate and non-intercrate `SelectionContext`. However, the local (inferctxt) evaluation cache is not aware of this distinction, so this kind of `InferCtxt` re-use will pollute the cache wth bad results. This commit avoids the issue by disabling the evaluation cache entirely during intercrate mode.
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@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 6d1f4d2 with merge ce935c6cd218f340d811819c3783652b0070319a... |
|
☀️ Try build successful - checks-actions |
|
Queued ce935c6cd218f340d811819c3783652b0070319a with parent 2c7bc5e, future comparison URL. |
|
Finished benchmarking commit (ce935c6cd218f340d811819c3783652b0070319a): comparison url. Summary: This change led to small relevant improvements 🎉 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. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
r? @rust-lang/wg-traits |
|
r? @nikomatsakis |
|
@bors r+ I'd like a test, but given it's a big repro, I'm not blocking on that. The other PR (#88993) is less "obviously correct" and imo "more complicated" (though not really). r? @jackh726 (Also, for the future, its rust-lang/traits for highfive) |
|
📌 Commit 6d1f4d2 has been approved by |
bors
added
S-waiting-on-bors
Mark-Simulacrum
added
the
beta-nominated
|
beta-nominating as a fix for #88969, which is a beta regression |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (35c8f26): comparison url. Summary: This change led to moderate 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 |
|
Perf regression is small enough and, given that this is a correctness fix, ultimately okay. |
jackh726
added
the
perf-regression-triaged
|
This PR seems to be the cause of #89119, somehow. |
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.
…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.
apiraino
added
the
T-compiler
rustbot
added
the
beta-accepted
Disable the evaluation cache when in intercrate mode It's possible to use the same `InferCtxt` with both an intercrate and non-intercrate `SelectionContext`. However, the local (inferctxt) evaluation cache is not aware of this distinction, so this kind of `InferCtxt` re-use will pollute the cache wth bad results. This commit avoids the issue by disabling the evaluation cache entirely during intercrate mode.
…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.
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)
It's possible to use the same
InferCtxtwith bothan intercrate and non-intercrate
SelectionContext. However,the local (inferctxt) evaluation cache is not aware of this
distinction, so this kind of
InferCtxtre-use will pollutethe cache wth bad results.
This commit avoids the issue by disabling the evaluation cache
entirely during intercrate mode.