Skip to content

make for_all_relevant_impls O(1) again #43723

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Aug 8, 2017

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Aug 7, 2017

A change in #41911 had made for_all_relevant_impls do a linear scan over
all impls, instead of using an HashMap. Use an HashMap again to avoid
quadratic blowup when there is a large number of structs with impls.

I think this fixes #43141 completely, but I want better measurements in
order to be sure. As a perf patch, please don't roll this up.

r? @eddyb
beta-nominating because regression

@arielb1 arielb1 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 7, 2017
@arielb1 arielb1 force-pushed the nonincremental-fast-reject branch from 82fdfaa to 4c8683a Compare August 7, 2017 22:39
@Mark-Simulacrum
Copy link
Member

@bors p=100 (no rolling up)

@Mark-Simulacrum
Copy link
Member

Presumably updates to submodules are unintentional?

A change in rust-lang#41911 had made `for_all_relevant_impls` do a linear scan over
all impls, instead of using an HashMap. Use an HashMap again to avoid
quadratic blowup when there is a large number of structs with impls.

I think this fixes rust-lang#43141 completely, but I want better measurements in
order to be sure. As a perf patch, please don't roll this up.
@arielb1 arielb1 force-pushed the nonincremental-fast-reject branch from 4c8683a to 453ad81 Compare August 8, 2017 08:18
@arielb1
Copy link
Contributor Author

arielb1 commented Aug 8, 2017

@Mark-Simulacrum

Yup. Fixed

@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 8, 2017
@eddyb
Copy link
Member

eddyb commented Aug 8, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 8, 2017

📌 Commit 453ad81 has been approved by eddyb

@bors
Copy link
Collaborator

bors commented Aug 8, 2017

⌛ Testing commit 453ad81 with merge 6d84a35...

bors added a commit that referenced this pull request Aug 8, 2017
make `for_all_relevant_impls` O(1) again

A change in #41911 had made `for_all_relevant_impls` do a linear scan over
all impls, instead of using an HashMap. Use an HashMap again to avoid
quadratic blowup when there is a large number of structs with impls.

I think this fixes #43141 completely, but I want better measurements in
order to be sure. As a perf patch, please don't roll this up.

r? @eddyb
beta-nominating because regression
@bors
Copy link
Collaborator

bors commented Aug 8, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 6d84a35 to master...

@bors bors merged commit 453ad81 into rust-lang:master Aug 8, 2017
@alexcrichton
Copy link
Member

cc @rust-lang/compiler, any thoughts about a backport here?

@alexcrichton
Copy link
Member

This improved the memory usage of nearly all incremental-related benchmarks by up to 25%.

🌴

@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Aug 23, 2017
@nikomatsakis
Copy link
Contributor

Accepted for beta. Not so small, but very important compile time regression.

cc @rust-lang/compiler

bors added a commit that referenced this pull request Aug 23, 2017
[beta] Final round of beta backports

Includes:

* [x] #43723
* [x] #43830
* [x] #43844
* [x] #44013
* [x] #44049
* [x] #43948
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 23, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compile time regression
6 participants