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

compilation time hit from recent PR #54083

Closed
nikomatsakis opened this issue Sep 9, 2018 · 7 comments
Closed

compilation time hit from recent PR #54083

nikomatsakis opened this issue Sep 9, 2018 · 7 comments
Labels
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

Sadly, #53949 seems to have been very bad for compilation times. I think we have to back it out until we find a way to do it more efficiently. :(

cc @estebank

@nikomatsakis nikomatsakis added I-slow Issue: Problems and improvements with respect to performance of generated code. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-compiletime Issue: Problems and improvements with respect to compile times. and removed I-slow Issue: Problems and improvements with respect to performance of generated code. labels Sep 9, 2018
@nikomatsakis
Copy link
Contributor Author

And here I was, all excited about the latest nll-dashboard results, which showed e.g. html5ever at at 1.01x ratio 😛

@michaelwoerister
Copy link
Member

tuple-stress-check:
avg: 9711.2% | min: 6519.9% | max: 18808.1%
avg: 9721.3% | min: 6350.5% | max: 17849.0%
avg: 9692.6% | min: 6351.6% | max: 17734.3%

Woooooooooooooooooooooowwwwww!! :)

@estebank
Copy link
Contributor

estebank commented Sep 9, 2018

And here I was, all excited about the latest nll-dashboard results, which showed e.g. html5ever at at 1.01x ratio

Woooooooooooooooooooooowwwwww!! :)

I feel like I deserve some kind of badge for this ^_^

Sadly, #53949 seems to have been very bad for compilation times. I think we have to back it out until we find a way to do it more efficiently. :(

I'm pretty sure that the check for margins in every close delimiter is to blame. This could be moved to instead collect all the matching delimiter spans and check the left padding only in the case of mismatched braces. This should fix the performance regression for all cases, while keeping the behavior on bad code, at the expense of keeping a vec of all braces' opening and closing span (instead of only the "suspicious" ones). I will be in flight until tomorrow afternoon. If the performance regression needs to be fixed before then, please revert the PR, otherwise I'll make the appropriate change (assuming I'm correct).

bors added a commit that referenced this issue Sep 9, 2018
Don't compute padding of braces unless they are unmatched

Follow up to #53949. Attempt to fix # #54083.

r? @nikomatsakis
bors added a commit that referenced this issue Sep 11, 2018
Don't compute padding of braces unless they are unmatched

Follow up to #53949. Attempt to fix # #54083.

r? @nikomatsakis
@nagisa
Copy link
Member

nagisa commented Sep 11, 2018

The corresponding PR has been merged. Should this be closed?

@michaelwoerister
Copy link
Member

perf.rust-lang.org looks like it's back to normal again. Thanks for fixing, @estebank!

@estebank
Copy link
Contributor

@michaelwoerister sorry to cause the epic slowdown to begin with :)

@michaelwoerister
Copy link
Member

@estebank No worries!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

No branches or pull requests

4 participants