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

Adds Vec::extend_from_slices_copy that accepts multiple slices #240

Merged
merged 6 commits into from
Mar 7, 2024

Conversation

zslayton
Copy link
Contributor

@zslayton zslayton commented Mar 1, 2024

This PR builds on #236 by introducing an extend_from_slices_copy method.

The singular extend_from_slice_copy method calls self.reserve() before copying the input slice. In many circumstances, users will have several inputs to append at once; for example:

let mut vec = Vec::new_in(&bump);
vec.extend_from_slice_copy("foo".as_bytes()); // reserve() + memcpy
vec.extend_from_slice_copy("bar".as_bytes()); // reserve() + memcpy
vec.extend_from_slice_copy("baz".as_bytes()); // reserve() + memcpy
assert_eq!("foobarbaz".as_bytes(), vec.as_slice());

The compiler is often (always?) unable to optimize away the redundant calls to reserve(), which leads to assembly like the following:

image

If there is not sufficient space already allocated, it's possible for each intermediate call to reserve to cause a reallocation that could have been avoided if the total space required were known in advance.

The new extend_from_slices_copy method takes a slice-of-slices, allowing it to precompute the space required and perform a single call to reserve.

let mut vec = Vec::new_in(&bump);
vec.extend_from_slices_copy(&[ // reserve()
  "foo".as_bytes(), // memcpy
  "bar".as_bytes(), // memcpy
  "baz".as_bytes(), // memcpy
])
assert_eq!("foobarbaz".as_bytes(), vec.as_slice());

These benchmarks show the relative performance of the two methods when there are N slices to append and there is not sufficient memory allocated in advance:

image

Note that there is a minor performance penalty for using extend_from_slices_copy when there's only one input slice.

In the case where sufficient space has already been allocated in the Bump and Vec ahead of time, extend_from_slices_copy is faster by a narrower margin.

preallocated=true benchmark results

image

Anecdotally, extend_from_slices_copy produces simpler assembly and the compiler also seems to be able to unroll the loops reliably when slice literals are being passed in (vs not-known-until-runtime and/or black_box slices, which is shown in the benchmarks above).

Copy link
Owner

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks!

All this unsafe code makes it so that I'd like some quickchecks to better exercise edge cases and give confidence that all t's are crossed and i's are dotted. Could you write the following quickchecks:

  • a quickcheck for extend_from_slice_copy that takes two slices, creates a vec from the first one, extends with the second, and asserts that the resulting vec length and contents are as expected
  • a similar quickcheck for extend_from_slices_copy that takes a list of slices, rather than just two, and creates a vec of the first one and then calls extend_from_slices_copy with the rest of them and checks that the resulting vec length and contents are as expected
  • a differential quickcheck: take a list of slices, and do two things, (1) take the first slice and turn it into a vec, then call extend_from_slice_copy n times with each of the rest of the slices, and (2) take the first slice again and turn it into another vec, then call extend_from_slices_copy once with all the rest of the slices. Finally, it should check that both vecs have the same length and contents.

src/collections/vec.rs Show resolved Hide resolved
src/collections/vec.rs Outdated Show resolved Hide resolved
src/collections/vec.rs Outdated Show resolved Hide resolved
src/collections/vec.rs Outdated Show resolved Hide resolved
@zslayton zslayton requested a review from fitzgen March 5, 2024 19:47
@zslayton
Copy link
Contributor Author

zslayton commented Mar 6, 2024

Last night I realized that I had required the caller of extend_from_slice_copy_unchecked to call set_len because I had originally (naively, incorrectly) planned to only call set_len once for the entire slice-of-slices in extend_from_slices_copy.

Commit 60b1407 moves the call to set_len into extend_from_slice_copy_unchecked and removes the corresponding invocations from the callers.

Copy link
Owner

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@fitzgen fitzgen merged commit 6a91333 into fitzgen:main Mar 7, 2024
8 checks passed
@zslayton zslayton deleted the extend_from_slices_copy branch March 7, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants