limit and clear cache obligations opportunistically #44269
Conversation
Keep **all** the obligations for every projection is wasteful of memory and compilation time. We only really care about those subobligations that may inform the result of the projection (i.e., may help to resolve any inference variables that appear within). Therefore, we can clear the subobligations from the cache that don't potentially affect the result of the projection. On every cache hit, we also take the opportunity to check if the type variables have been resolved *yet* and, if so, clear out the pending obligations. Fixes rust-lang#43613
arielb1
added
the
beta-nominated
|
Sure. We probably want to add the test case to perf-collector. I think my real worry here soundness-wise is that we could look up and "confirm" a Maybe just add an additional We need to have a better model of soundness + caching. |
|
I think the argument for why that cannot happen is that
This makes a sort of inductive argument. Are there cases you're thinking of that don't fall into these two categories? In any case, I'm not quite sure what you're proposing regarding this "extra" |
I don't disagree; I still think we can move towards a more chalk-like model, where the "trait system impl" doesn't handle things like "traits and projections" directly, but rather just interprets goals that it doesn't understand as deeply. We can then uniformly insert caching here that would replace and subsume the existing patchwork of caching. It'd be nice to prototype this more directly in chalk -- right now the impl is intentionally kept "naive", but I think it'd be good to have a less naive, caching variant to show how it could work. |
arielb1
added
the
S-waiting-on-author
|
@arielb1 does that last commit look like what you had in mind? I tested it -- it still preserves the memory/performance benefits on the test case in question, at least: |
src/librustc/traits/project.rs
Outdated
| /// may or may not be necessary -- in principle, all the obligations | ||
| /// that must be proven to show that `T: Trait` were also returned | ||
| /// when the cache was first populated. But there is a vague concern | ||
| /// that perhaps someone would not have proven those, but also not |
There was a problem hiding this comment.
My concern wasn't about snapshot misuse, it was more about cache(/other "concurrent" operation) pollution.
|
@bors r=arielb1 |
|
📌 Commit c1dddce has been approved by |
nikomatsakis
added
the
T-compiler
|
@bors p=1 I'm marking this as high priority because it fixes a P-high performance regression and because I would like to backport it. |
…ns, r=arielb1 limit and clear cache obligations opportunistically Keeping **all** the obligations for every projection is wasteful of memory and compilation time. We only really care about those subobligations that may inform the result of the projection (i.e., may help to resolve any inference variables that appear within). Therefore, we can clear the subobligations from the cache that don't potentially affect the result of the projection. On every cache hit, we also take the opportunity to check if the type variables have been resolved *yet* and, if so, clear out the pending obligations. Fixes #43613. r? @arielb1 NB -- not sure how to test for this. Probably we should add the #43613 test case to perf.
|
☀️ Test successful - status-appveyor, status-travis |
|
Marking as beta-accepted. Fairly targeted patch, and fixes a major regression. It is my PR though so it wouldn't hurt to have people check it out. cc @rust-lang/compiler |
nikomatsakis
added
the
beta-accepted
|
Backporting now. |
alexcrichton
removed
the
beta-nominated
Beta 20170928 Backports of: - Allow unused extern crate again #44825 - macros: fix bug in collecting trait and impl items with derives. #44757 - `--cap-lints allow` switches off `can_emit_warnings` #44627 - Update the libc submodule #44116 - limit and clear cache obligations opportunistically #44269 - clear out projection subobligations after they are processed #43999
We want to retain obligations that *contain* inference variables, not obligations that *don't contain* them, in order to fix rust-lang#43132. Because of surrounding changes to inference, the ICE doesn't occur in its original case, but I believe it could still be made to occur on master. Maybe I should try to write a new test case? Certainly not right now (I'm mainly trying to get us a beta that we can ship) but maybe before we land this PR on nightly? This seems to cause a 10% performance regression in my imprecise attempt to benchmark item-body checking for rust-lang#43613, but it's better to be slow and right than fast and wrong. If we want to recover that, I think we can change the constrained-type-parameter code to actually give a list of projections that are important for resolving inference variables and filter everything else out.
We want to retain obligations that *contain* inference variables, not obligations that *don't contain* them, in order to fix rust-lang#43132. Because of surrounding changes to inference, the ICE doesn't occur in its original case, but I believe it could still be made to occur on master. Maybe I should try to write a new test case? Certainly not right now (I'm mainly trying to get us a beta that we can ship) but maybe before we land this PR on nightly? This seems to cause a 10% performance regression in my imprecise attempt to benchmark item-body checking for rust-lang#43613, but it's better to be slow and right than fast and wrong. If we want to recover that, I think we can change the constrained-type-parameter code to actually give a list of projections that are important for resolving inference variables and filter everything else out.
fix logic error in #44269's `prune_cache_value_obligations` We want to retain obligations that *contain* inference variables, not obligations that *don't contain* them, in order to fix #43132. Because of surrounding changes to inference, the ICE doesn't occur in its original case, but I believe it could still be made to occur on master. Maybe I should try to write a new test case? Certainly not right now (I'm mainly trying to get us a beta that we can ship) but maybe before we land this PR on nightly? This seems to cause a 10% performance regression in my imprecise attempt to benchmark item-body checking for #43613, but it's better to be slow and right than fast and wrong. If we want to recover that, I think we can change the constrained-type-parameter code to actually give a list of projections that are important for resolving inference variables and filter everything else out.
Beta 20170928 Backports of: - Allow unused extern crate again #44825 - macros: fix bug in collecting trait and impl items with derives. #44757 - `--cap-lints allow` switches off `can_emit_warnings` #44627 - Update the libc submodule #44116 - limit and clear cache obligations opportunistically #44269 - clear out projection subobligations after they are processed #43999 - fix logic error in #44269's `prune_cache_value_obligations` #45065 - REVERTS #43543: Cleanup some remains of `hr_lifetime_in_assoc_type` compatibility lint:
This effectively reverts rust-lang#43546 as it seems that it does affect performance more than the PR has anticipated. Follow-up changes from rust-lang#44269 are also reverted. This also removes the deduplication code from rust-lang#48296 as duplications were primarily coming from rust-lang#43546 and after removing that code it probably doesn't worth paying the cost of using a hash map.
Keeping all the obligations for every projection is wasteful of
memory and compilation time. We only really care about those
subobligations that may inform the result of the projection (i.e., may
help to resolve any inference variables that appear within).
Therefore, we can clear the subobligations from the cache that don't
potentially affect the result of the projection. On every cache hit,
we also take the opportunity to check if the type variables have been
resolved yet and, if so, clear out the pending obligations.
Fixes #43613.
r? @arielb1
NB -- not sure how to test for this. Probably we should add the #43613 test case to perf.