-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use EvaluatedToOkModuloRegions
whenever we erase regions
#83220
Conversation
r? @davidtwco (rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 8f40b09ff6658df40bce0f4d2f69405d9359fd31 with merge ae47118b087e92eb4278a213096c4912eb626f06... |
☀️ Try build successful - checks-actions |
1 similar comment
☀️ Try build successful - checks-actions |
Queued ae47118b087e92eb4278a213096c4912eb626f06 with parent 0ce0fed, future comparison URL. |
Finished benchmarking try commit (ae47118b087e92eb4278a213096c4912eb626f06): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
This makes sense to me. Am I correct in that this should only cause us to not be able to cache things that we were before? (I.e. we could be more conservative here than we need and its fine; it's only going the other way that we have to be extra careful). I haven't really looked at this code, so I don't know what the effect of module regions or not. |
We still cache something in every case where we were previously caching - however, we now store
That's my understanding, though I'd like to get someone more familiar with this code to confirm that. IIUC, it would be sound to delete |
8f40b09
to
31b3533
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 31b35337591568f47e9580d36d931348584b8131 with merge eeb5a70d1b31f72580d95160f43b16018ac19482... |
☀️ Try build successful - checks-actions |
Finished benchmarking try commit (eeb5a70d1b31f72580d95160f43b16018ac19482): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
Performance seems reasonable. |
Fixes rust-lang#80691 When we evaluate a trait predicate, we convert an `EvaluatedToOk` result to `EvaluatedToOkModuloRegions` if we erased any regions. We cache the result under a region-erased 'freshened' predicate, so `EvaluatedToOk` may not be correct for other predicates that have the same cache key.
31b3533
to
102b578
Compare
@bors r=nikomatsakis |
📌 Commit 102b578 has been approved by |
☀️ Test successful - checks-actions |
@nikomatsakis @Aaron1011 Can you talk a bit more about why the performance regression seems reasonable? A performance hit of 2.8%, while not terrible, is certainly something we would want to discuss before writing off. |
@rylev fair! Perhaps my thresholds are off. I was motivated to land this mainly because it seemed reasonable and there were a number of problems being reported that it aims to address. However I'd be ok to revert and dig a bit deeper to see if there's an alternate solution with less performance impact. One question I do have is that I'm not sure what gave rise to those problems, as this doesn't seem to be related to any new change (i.e., this caching logic etc has been in place a long time). |
I think this issue has always exited - when #83007 landed, we started getting ICEs for a lot of issues that were silently causing results to change in incremental compilation. |
Yeah, as a correctness fix I think hit here is OK |
#83115 is now on stable, so nominating this for backport |
@jonas-schievink: I think the issue people are hitting on stable is #83538 (but I'm not sure). |
Yeah, I'm also hitting the issue on beta... |
Fixes #80691
When we evaluate a trait predicate, we convert an
EvaluatedToOk
result toEvaluatedToOkModuloRegions
if we erased anyregions. We cache the result under a region-erased 'freshened'
predicate, so
EvaluatedToOk
may not be correct for other predicatesthat have the same cache key.