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

Performance regression in 0.6.1 #182

Closed
niklasf opened this issue Mar 29, 2021 · 8 comments · Fixed by #183
Closed

Performance regression in 0.6.1 #182

niklasf opened this issue Mar 29, 2021 · 8 comments · Fixed by #183
Labels

Comments

@niklasf
Copy link
Contributor

niklasf commented Mar 29, 2021

https://github.com/niklasf/shakmaty/blob/master/benches/benches.rs are showing a major performance regression going from arravec 0.6.0 to 0.6.1. For example:

bench_shallow_perft
  Instructions:            31844284 (+153.8268%)
  L1 Accesses:             54897187 (+233.5472%)
  L2 Accesses:                  315 (+47.19626%)
  RAM Accesses:                 472 (+8.505747%)
  Estimated Cycles:        54915282 (+233.3271%)

I bisected it to 5ad4687 as the first bad commit.

The benchmarks are mostly using ArrayVec::{new,push,retain}(). So far I did not manage to reduce it further.

niklasf added a commit to niklasf/shakmaty that referenced this issue Mar 29, 2021
@bluss
Copy link
Owner

bluss commented Mar 29, 2021

That's unfortunate, it's not possible to just revert that change either. More examination is needed. Would be great if it was just a matter of #[inline], but potentially not(?) (Edit: I have ruled out inlining)

@bluss bluss added the bug label Mar 29, 2021
@bluss
Copy link
Owner

bluss commented Mar 29, 2021

Maybe we'll just revert new, add a new_const constructor (both stable) and publish 0.7.0.

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Mar 29, 2021

The compiler is emitting code for initializing the array of MaybeUninit (even though it doesn't need to):
https://rust.godbolt.org/z/5e1o53q9a

Reported here: rust-lang/rust#83657

@bluss
Copy link
Owner

bluss commented Mar 29, 2021

Thanks

@bluss
Copy link
Owner

bluss commented Mar 29, 2021

Because the 0.6 version was so new, it is a rather small cost to release 0.7 immediately to fix this, not so much installed base to bother with the upgrade. So thanks for flagging this so quickly, too.

@bluss
Copy link
Owner

bluss commented Mar 29, 2021

@niklasf If you have this info, how was the performance of arrayvec 0.6.0 or 0.7.0 compared with the previous version 0.5.2, any changes?

@niklasf
Copy link
Contributor Author

niklasf commented Mar 30, 2021

Thanks for handling this so quickly.

0.5.2 to 0.7.0 is a slight improvement (possibly within noise limits). For example, the benchmark that flagged this issue:

bench_shallow_perft
  Instructions:            12345813 (-0.052298%)
  L1 Accesses:             16236361 (-0.309209%)
  L2 Accesses:                  121 (-33.51648%)
  RAM Accesses:                 431 (-2.927928%)
  Estimated Cycles:        16252051 (-0.313559%)

@bluss
Copy link
Owner

bluss commented Mar 30, 2021

Your retain improvement is also in 0.7. Release note will be updated with that.

bors bot pushed a commit to rust-lang/rust-analyzer that referenced this issue Apr 5, 2021
bors bot added a commit to rust-lang/rust-analyzer that referenced this issue Apr 5, 2021
8346: Use arrayvec 0.7 to avoid perf regression in 0.6.1 r=lnicola a=kjeremy

See: bluss/arrayvec#182

Co-authored-by: kjeremy <kjeremy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants