Skip to content
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

add a per-param-env cache to impls_bound #37152

Merged
merged 1 commit into from
Oct 16, 2016
Merged

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Oct 13, 2016

There used to be only a global cache, which led to uncached calls to
trait selection when there were type parameters.

This causes a 20% decrease in borrow-checking time and an overall 0.5% performance increase during bootstrapping (as borrow-checking tends to be a tiny part of compilation time).

Fixes #37106 (drop elaboration times are now ~half of borrow checking,
so might still be worthy of optimization, but not critical).

r? @pnkfelix

@arielb1 arielb1 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 13, 2016
@pnkfelix
Copy link
Member

@arielb1 r=me once you finish confirming there do not seem to be adverse performance consequences.

There used to be only a global cache, which led to uncached calls to
trait selection when there were type parameters.

I'm running a check that there are no adverse performance effects.

Fixes rust-lang#37106 (drop elaboration times are now ~half of borrow checking,
so might still be worthy of optimization, but not critical).
@arielb1
Copy link
Contributor Author

arielb1 commented Oct 14, 2016

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Oct 14, 2016

📌 Commit a61d85b has been approved by pnkfelix

@dtolnay
Copy link
Member

dtolnay commented Oct 14, 2016

I tried this out on Serde and compile time dropped from 10 seconds using 1.14.0-nightly (a3bc191 2016-10-10) to 5.3 seconds using this commit. Thanks for taking care of this so quickly!

@bors
Copy link
Contributor

bors commented Oct 15, 2016

⌛ Testing commit a61d85b with merge 98a3502...

bors added a commit that referenced this pull request Oct 15, 2016
add a per-param-env cache to `impls_bound`

There used to be only a global cache, which led to uncached calls to
trait selection when there were type parameters.

This causes a 20% decrease in borrow-checking time and an overall 0.5% performance increase during bootstrapping (as borrow-checking tends to be a tiny part of compilation time).

Fixes #37106 (drop elaboration times are now ~half of borrow checking,
so might still be worthy of optimization, but not critical).

r? @pnkfelix
@bors bors merged commit a61d85b into rust-lang:master Oct 16, 2016
@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 24, 2016
@nikomatsakis
Copy link
Contributor

Seems harmless enough. I guess that this is marked for beta-backport because the elaboration code is relatively new?

cc @rust-lang/compiler -- should we backport? compile-time only.

@eddyb
Copy link
Member

eddyb commented Oct 25, 2016

SGTM as this seems to noticeably push back on a compile times regression.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 1, 2016

Marking as beta-accepted.

@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 1, 2016
@brson brson mentioned this pull request Nov 3, 2016
@brson brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants