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

Fix quadratic performance with lots of use statements #43584

Merged
merged 2 commits into from
Aug 2, 2017

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Aug 1, 2017

This fixes 2 problems that caused quadratic performance when lots of use-statements were present. After this patch, performance is linear (and very fast) even with 1M uses.

Fixes #43572.
Fixes #43573.

r? @eddyb

arielb1 added 2 commits August 1, 2017 14:18
We already had a cache for file contents, but we read the source-file
before testing the cache, causing obvious slowness, so this just avoids
loading the source-file when the cache already has the contents.
Instead of finding the next free disambiguator by incrementing it until
you find a place, store the next available disambiguator in an hash-map.

This avoids O(n^2) performance when lots of items have the same
un-disambiguated `DefPathData` - e.g. all `use` items have
`DefPathData::Misc`.
if *self.external_src.borrow() == ExternalSource::AbsentOk {
let src = get_src();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, my bad, I didn't notice that it was loading the file each time in the original PR.

@eddyb
Copy link
Member

eddyb commented Aug 1, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Aug 1, 2017

📌 Commit 70478ca has been approved by eddyb

@alexcrichton
Copy link
Member

Thanks for the quick fix @arielb1!

@froydnj
Copy link
Contributor

froydnj commented Aug 1, 2017

Could this be uplifted to beta @arielb1 ? Firefox would love having this in the 1.20 release!

@alexcrichton alexcrichton added beta-nominated Nominated 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. labels Aug 1, 2017
@eddyb
Copy link
Member

eddyb commented Aug 1, 2017

I agree with backporting to beta. cc @nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 2, 2017

⌛ Testing commit 70478ca with merge 0b5c087...

bors added a commit that referenced this pull request Aug 2, 2017
Fix quadratic performance with lots of use statements

This fixes 2 problems that caused quadratic performance when lots of use-statements were present. After this patch, performance is linear (and very fast) even with 1M uses.

Fixes #43572.
Fixes #43573.

r? @eddyb
@bors
Copy link
Contributor

bors commented Aug 2, 2017

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

@bors bors merged commit 70478ca into rust-lang:master Aug 2, 2017
@alexcrichton
Copy link
Member

Tagging as accepted for beta

@alexcrichton alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Aug 12, 2017
@alexcrichton
Copy link
Member

@arielb1 the second commit here, 70478ca, doesn't cleanly apply against beta, mind sending a PR to beta for this?

@arielb1
Copy link
Contributor Author

arielb1 commented Aug 13, 2017

@arielb1 the second commit here, 70478ca, doesn't cleanly apply against beta, mind sending a PR to beta for this?

The second commit fixes a less important edge case. If it doesn't apply cleanly, I'll prefer not to include it.

@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 23, 2017
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
5 participants