Cache projections in trans #35761
Cache projections in trans #35761
Conversation
|
🚡 |
|
☔ The latest upstream changes (presumably #35764) made this pull request unmergeable. Please resolve the merge conflicts. |
|
(I'll rebase shortly.) |
There were various places that we are invoking `drain_fulfillment_cx` with a "result" of `()`. This is kind of pointless, since it amounts to just a call to `select_all_or_error` along with some extra overhead.
I plan to put a cache on the shared context, for now at least.
nikomatsakis
force-pushed
the
faster-trans-fulfill-obligation
branch
from
6de8efe to
7057c42
Compare
| } else { | ||
| closure_ty | ||
| } | ||
| closure_ty |
There was a problem hiding this comment.
This method doesn't need to exist anymore AFAICT.
There was a problem hiding this comment.
This method doesn't need to exist anymore AFAICT.
Why do you say that? Certainly, it is still called, e.g. from the trait code -- and it seems like it still serves a purpose (drawing the types for local closures from the inference tables). But maybe I'm forgetting some subtle point here?
There was a problem hiding this comment.
It only exists because it's doing something different than self.tcx.closure_type(def_id, substs), but not after your changes. The Nevermind, that's mainly there for lifetime issues.InferTables enum might also be removable
EDIT: wait, that's also wrong, it's on tables - the tcx one is global-context-only.
EDIT2: no, there's also dynamic shenanigans with cross-crate loading. Forget about changing this.
|
@bors r+ |
|
📌 Commit 00d208e has been approved by |
…r=eddyb Cache projections in trans This introduces a cache for the results of projection and normalization in trans. This is in addition to the existing cache that is per-inference-context. Trans is an easy place to put the cache because we are guaranteed not to have type parameters and also we don't expect any failures or inference variables, so there is no need to cache or follow-up on obligations that come along with. (As evidenced by the fact that this particular code would panic if any error occurred.) That said, I am not sure this is 100% the best place for it; I sort of wanted a cache like we have in the fulfillment context for global names; but that cache only triggers when all subsequent obligations are satisfied, and since projections don't have an entry in the obligation jungle there is no easy place to put it. I considered caching both the result and obligations globally, but haven't really tried implementing it. It might be a good next step. Regardless, this cache seems to have no real effect on bootstrap time (maybe a slight improvement), but on [the futures.rs test case I was looking at](rust-lang-deprecated/rustc-benchmarks#6), it improves performance quite a bit: | phase | before | after | | ----- | ------ | ----- | | collection | 0.79s | 0.46s | | translation | 6.8s | 3.2s | | total | 11.92s | 7.15s | r? @arielb1
bluss
added
the
relnotes
This introduces a cache for the results of projection and normalization in trans. This is in addition to the existing cache that is per-inference-context. Trans is an easy place to put the cache because we are guaranteed not to have type parameters and also we don't expect any failures or inference variables, so there is no need to cache or follow-up on obligations that come along with. (As evidenced by the fact that this particular code would panic if any error occurred.)
That said, I am not sure this is 100% the best place for it; I sort of wanted a cache like we have in the fulfillment context for global names; but that cache only triggers when all subsequent obligations are satisfied, and since projections don't have an entry in the obligation jungle there is no easy place to put it. I considered caching both the result and obligations globally, but haven't really tried implementing it. It might be a good next step.
Regardless, this cache seems to have no real effect on bootstrap time (maybe a slight improvement), but on the futures.rs test case I was looking at, it improves performance quite a bit:
r? @arielb1