-
Notifications
You must be signed in to change notification settings - Fork 118
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
Provides implementation of Vec::extend_from_slice
optimized for T: Copy
#236
Conversation
@overlookmotel expressed a concern that unconditionally using I extended the benchmarks in this PR to test a variety of sizes between 4 bytes and 16KB. The results--in this microbenchmark, running on my machine--indicated that |
That's interesting! I wonder why we saw a slow-down in OXC (though an extremely small one) with the other change. The other explanations I can think of are:
Sorry, not trying to pick holes in your work, just trying to understand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good to me modulo nitpick below.
Regarding short strings and possible perf regressions: nothing I've read seems serious enough to warrant reverting that optimization. I'm happy to receive follow up PRs that further improve things here, if there is anything further to improve.
.gitignore
Outdated
|
||
# JetBrains IDE files (e.g. RustRover) | ||
.idea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like something that is more appropriate for your home directory's .gitignore
than every project you contribute to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 TIL that you can put a .gitignore
in your home directory. I'll remove this, thanks.
I also noticed that Vec
's impl of std::io::Write
can be updated to use this method; I'll push another commit with that change tomorrow.
Yes absolutely, the performance "regression" we saw was absolutely tiny, and for general purpose use 80x faster for long strings outweighs 0.1% slower for very short strings. I just wanted to put it on the record that there was some downside to the PR I made, rather than leave it unsaid, in case it affects anyone else. |
This seemed plausible to me too! I added a few non-power-of-2 cases to the benchmark to check it out, but it looks like the improvement holds for those as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@zslayton Thanks for tolerating my nitty comments, and for benching shorter byte ranges. I remain curious about why we saw the slight slow-down we did on similar PR for strings, but probably I should stop worrying! |
Sure thing! I appreciate having another set of eyes on this, I'd rather spend some time double-checking things than accidentally cause a regression for all of these folks 🙃😬.
I'm curious too! |
Addresses #235.
This PR is meant to be a jumping-off point for discussion. It adds a new inherent impl method to
Vec
:It moves the logic added in #229 from
String
toVec
and then re-implementspush_str
by calling the newextend_from_slice_copy
.I've duplicated the
push_str
benchmarks but forVec<'_, u8>
and the performance improves as expected: