Cache projections in trans #35761

Merged
merged 7 commits into from Sep 2, 2016

Conversation

Projects
None yet
6 participants
@nikomatsakis
Contributor

nikomatsakis commented Aug 17, 2016

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:

phase before after
collection 0.79s 0.46s
translation 6.8s 3.2s
total 11.92s 7.15s

r? @arielb1

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
Member

alexcrichton commented Aug 17, 2016

🚡

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 25, 2016

Contributor

☔️ The latest upstream changes (presumably #35764) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Aug 25, 2016

☔️ The latest upstream changes (presumably #35764) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 31, 2016

Contributor

r? @eddyb -- @arielb1 seems to be busy :)

Contributor

nikomatsakis commented Aug 31, 2016

r? @eddyb -- @arielb1 seems to be busy :)

@rust-highfive rust-highfive assigned eddyb and unassigned arielb1 Aug 31, 2016

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 31, 2016

Contributor

(I'll rebase shortly.)

Contributor

nikomatsakis commented Aug 31, 2016

(I'll rebase shortly.)

nikomatsakis added some commits Aug 5, 2016

remove unneccessary uses of `drain_fulfillment_cx`
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.
give `apply_param_substs` a `SharedCrateContext`
I plan to put a cache on the shared context, for now at least.
- } else {
- closure_ty
- }
+ closure_ty

This comment has been minimized.

@eddyb

eddyb Aug 31, 2016

Member

This method doesn't need to exist anymore AFAICT.

@eddyb

eddyb Aug 31, 2016

Member

This method doesn't need to exist anymore AFAICT.

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 1, 2016

Contributor

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?

@nikomatsakis

nikomatsakis Sep 1, 2016

Contributor

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?

This comment has been minimized.

@eddyb

eddyb Sep 1, 2016

Member

It only exists because it's doing something different than self.tcx.closure_type(def_id, substs), but not after your changes. The InferTables enum might also be removable Nevermind, that's mainly there for lifetime issues.

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.

@eddyb

eddyb Sep 1, 2016

Member

It only exists because it's doing something different than self.tcx.closure_type(def_id, substs), but not after your changes. The InferTables enum might also be removable Nevermind, that's mainly there for lifetime issues.

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.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Sep 1, 2016

Member

@bors r+

Member

eddyb commented Sep 1, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 1, 2016

Contributor

📌 Commit 00d208e has been approved by eddyb

Contributor

bors commented Sep 1, 2016

📌 Commit 00d208e has been approved by eddyb

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 1, 2016

Contributor

⌛️ Testing commit 00d208e with merge 497d67d...

Contributor

bors commented Sep 1, 2016

⌛️ Testing commit 00d208e with merge 497d67d...

bors added a commit that referenced this pull request Sep 1, 2016

Auto merge of #35761 - nikomatsakis:faster-trans-fulfill-obligation, …
…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

@bors bors merged commit 00d208e into rust-lang:master Sep 2, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
homu Test successful
Details

@bluss bluss added the relnotes label Sep 8, 2016

@nikomatsakis nikomatsakis deleted the nikomatsakis:faster-trans-fulfill-obligation branch Oct 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment