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.

Sign up for GitHub

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

Jump to bottom

10% regression in the issue-43572-unused-uses test case #44575

Closed
nikomatsakis opened this issue Sep 14, 2017 · 6 comments
Closed

10% regression in the issue-43572-unused-uses test case #44575

nikomatsakis opened this issue Sep 14, 2017 · 6 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

The perf website shows a 10% regression in the issue-43572-unused-uses test case as a result of #44435 (diff).

Maybe not a big deal, but worth doing a quick profile to see if something can be done!

cc @alexcrichton
@nikomatsakis nikomatsakis added I-nominated regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 14, 2017
@alexcrichton
Copy link
Member

graph

@alexcrichton
Copy link
Member

Looking at a profile the numbers are pretty close together, but I think it basically says that we're hashing more in blake2b

@alexcrichton alexcrichton mentioned this issue Sep 15, 2017
Closed
@alexcrichton
Copy link
Member

I'm not 100% sure, but #44584 may be the culprit here

@Mark-Simulacrum Mark-Simulacrum added C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. labels Sep 18, 2017
alexcrichton added a commit to alexcrichton/rust that referenced this issue Sep 19, 2017
@alexcrichton
rustc: Avoid HashStable for DefIndex queries
fffb99e 
I'm not 100% familiar with this trait and this location, but it looks like
there's a default implementation of `DepNodeParams` for anything that implements
`HashStable`, but types like `DefId` which have precalculated hashes bypass this
default implementation for a speedier one.

This commit applies what I believe is the same optimization to `DefIndex`,
looking up the local hash for it rather than going through the full `HashStable`
rigamarole

cc rust-lang#44575
@nikomatsakis
Copy link
Contributor Author

The fix that @alexcrichton landed improved things, as you can see from this graph, but it's not completely resolved.

@nikomatsakis
Copy link
Contributor Author

triage: P-high

Calling this P-high as we investigate.

@rust-highfive rust-highfive added P-high High priority and removed I-nominated labels Sep 21, 2017
@alexcrichton
Copy link
Member

Given this new graph looks like #44696 recovered any missing losses here, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@alexcrichton @nikomatsakis @Mark-Simulacrum @rust-highfive