rustc_typeck: remove the "preload all impls ever" workaround in coherence. #25323

Merged
merged 4 commits into from May 12, 2015

Conversation

Projects
None yet
8 participants
@eddyb
Member

eddyb commented May 12, 2015

The loop to load all the known impls from external crates seems to have been used because ty::populate_implementations_for_trait_if_necessary wasn't doing its job, and solely relying on it resulted in loading only impls in the same crate as the trait.

Coherence for librustc was reduced from 18.310s to 0.610s, from stage1 to stage2.
Interestingly, type checking also went from 46.232s to 42.003s, though that could be noise or unrelated improvements.

On a smaller scale, fn main() {} now spends 0.003s in coherence instead of 0.368s, which fixes #22068.
It also peaks at only 1.2MB, instead of 16MB of heap usage.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive May 12, 2015

Collaborator

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented May 12, 2015

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb May 12, 2015

Member

@brson Any chance this can make it into 1.0? I don't mind it personally, but I've seen some interest on IRC for having the (minor IMO) compile-time wins here in 1.0.

Member

eddyb commented May 12, 2015

@brson Any chance this can make it into 1.0? I don't mind it personally, but I've seen some interest on IRC for having the (minor IMO) compile-time wins here in 1.0.

@bstrie

This comment has been minimized.

Show comment
Hide comment
@bstrie

bstrie May 12, 2015

Contributor

@eddyb, you're my hero 😻

Contributor

bstrie commented May 12, 2015

@eddyb, you're my hero 😻

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix May 12, 2015

Commit lgtm

Commit lgtm

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix May 12, 2015

Commit lgtm

Commit lgtm

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix May 12, 2015

Commit lgtm

Commit lgtm

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix May 12, 2015

Commit lgtm

Commit lgtm

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
Member

pnkfelix commented May 12, 2015

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 12, 2015

Contributor

📌 Commit 75cd8f9 has been approved by pnkfelix

Contributor

bors commented May 12, 2015

📌 Commit 75cd8f9 has been approved by pnkfelix

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
Member

pnkfelix commented May 12, 2015

@bors p=5

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix May 12, 2015

Member

(Regarding 1.0, I'm inclined to say the risk/reward here does not pay off; there are other lower risk PR's that also have UX impact that we declined much earlier in beta cycle)

Member

pnkfelix commented May 12, 2015

(Regarding 1.0, I'm inclined to say the risk/reward here does not pay off; there are other lower risk PR's that also have UX impact that we declined much earlier in beta cycle)

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
Member

pnkfelix commented May 12, 2015

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 12, 2015

Contributor

⌛️ Testing commit 75cd8f9 with merge 67dfc17...

Contributor

bors commented May 12, 2015

⌛️ Testing commit 75cd8f9 with merge 67dfc17...

bors added a commit that referenced this pull request May 12, 2015

Auto merge of #25323 - eddyb:coherent-coherence, r=pnkfelix
The loop to load all the known impls from external crates seems to have been used because `ty::populate_implementations_for_trait_if_necessary` wasn't doing its job, and solely relying on it resulted in loading only impls in the same crate as the trait.

Coherence for `librustc` was reduced from 18.310s to 0.610s, from stage1 to stage2.
Interestingly, type checking also went from 46.232s to 42.003s, though that could be noise or unrelated improvements.

On a smaller scale, `fn main() {}` now spends 0.003s in coherence instead of 0.368s, which fixes #22068.
It also peaks at only 1.2MB, instead of 16MB of heap usage.
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 12, 2015

Contributor

Without reading too closely, this looks great to me, and I agree with @pnkfelix's assessment that this is not a good candidate for backporting.

Contributor

nikomatsakis commented May 12, 2015

Without reading too closely, this looks great to me, and I agree with @pnkfelix's assessment that this is not a good candidate for backporting.

@bors bors merged commit 75cd8f9 into rust-lang:master May 12, 2015

2 checks passed

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

@eddyb eddyb deleted the eddyb:coherent-coherence branch May 12, 2015

@killercup

This comment has been minimized.

Show comment
Hide comment
@killercup

killercup May 12, 2015

Member

Nice, those are some serious speedups! Looks like 1.1 will already improve compile speed.

Member

killercup commented May 12, 2015

Nice, those are some serious speedups! Looks like 1.1 will already improve compile speed.

@o01eg o01eg referenced this pull request in rust-lang-nursery/rust-clippy May 18, 2017

Closed

Clippy 0.0.133 doesn't build on nightly rust. #1767

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