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

Add ArrayVec.extend_from_slice and capacity_left #101

Merged
merged 4 commits into from
Nov 28, 2018
Merged

Add ArrayVec.extend_from_slice and capacity_left #101

merged 4 commits into from
Nov 28, 2018

Conversation

Thomasdezeeuw
Copy link
Contributor

This add two new public methods on ArrayVec:

  • extend_from_slice: extends the vector with a slice using a memcpy.
  • capacity_left: utility function for use with extend_from_slice, e.g. see fa4e22f.

This also changes the Write implementation to use extend_from_slice and another commit that removes some whitespace and empty lines.

@Michael-F-Bryan
Copy link

I was just about to make a PR introducing exactly this. @bluss are there any things preventing this PR from being merged?

@bluss
Copy link
Owner

bluss commented Nov 25, 2018

I think this is a good change. Can we add to the extend benchmarks, and demonstrate that this is worthwhile? Also, please don't include the formatting and whitespace changes in the PR.

@Thomasdezeeuw
Copy link
Contributor Author

I've rebased on master, removed the cleanup commit and add a benchmark, for which I got the following results:

test extend_with_constant ... bench:         211 ns/iter (+/- 6) = 2426 MB/s
test extend_with_range    ... bench:         205 ns/iter (+/- 4) = 2497 MB/s
test extend_with_slice    ... bench:         191 ns/iter (+/- 4) = 2680 MB/s
test extend_with_slice_fn ... bench:           7 ns/iter (+/- 0) = 73142 MB/s

This is only a single run on a laptop, so take it with a grain of salt but I think the results are clear.

@Thomasdezeeuw
Copy link
Contributor Author

I also should note I'm not sure if I used blackbox correctly.

@bluss
Copy link
Owner

bluss commented Nov 26, 2018

Yes very nice. See my linked pr which should improve regular extend. But I think we can reuse std's reasoning here, and they haven't deprecated this method yet.

@Thomasdezeeuw
Copy link
Contributor Author

@bluss so is this acceptable to merge, or would you like to see some more changes?

@bluss
Copy link
Owner

bluss commented Nov 27, 2018

It's good, just thinking of the name of capacity left, does it have a precedent in std or otherwise? Maybe a name like remaining_capacity? Also the alternative of not exposing it.

Also the panic is a point of API decision but I think it is fine -- but it has different semantics than extend which is problematic

@Thomasdezeeuw
Copy link
Contributor Author

For the capacity_left: I don't there is any precedent as most types in std can grow. So capacity left would be inifinite (although memory limited of course). I think the function itself is very useful because ArrayVec can't grow, so I definitly make it public. I would use it atleast one of my projects.

As for the panicing in extend_from_slice: I followed the precedent set by push and related functions. Alternatively we could change the API to only copy the number of elements the ArrayVec has capacity for, something like this:

    /// Extend the `ArrayVec` from `other`. Returns the number of elements copied.
    pub fn extend_from_slice(&mut self, other: &[A::Item]) -> usize
        where A::Item: Copy,
    {
        let count = ::std::cmp::min(self.capacity_left(), other.len());
        if count == 0 {
            return 0;
        }

        // Copy `count` elements ...

        count
    }

@bluss
Copy link
Owner

bluss commented Nov 28, 2018

Ok, I think we can merge this. I can apply the following comments on my own, so that we stop the ping-ponging:

  • Let us use the same semantics as .extend(). Just like on a Vec, extend_from_slice is an "optimization" of extend, so we use the same rules. To make it (annoying?) clear to the user, we copy all the elements that fit but return an Err if there are elements left over.
  • Use the name .remaining_capacity() it seems the most regular

See also PR #112 which makes the regular .extend() compile to memcpy for a cloned slice iterator (with the caveat that benchmarking is tricky, maybe it doesn't happen in every setting, but at least in this one, which is very sparely black boxed, and that's a good thing, disturb as little as possible..).

@bluss
Copy link
Owner

bluss commented Nov 28, 2018

Thanks for adding this!

@bluss bluss merged commit d9222c0 into bluss:master Nov 28, 2018
@bluss
Copy link
Owner

bluss commented Nov 28, 2018

No, ok, that doesn't feel right.

  • Panic on too much input has the drawback that it's invisible to the user if they transition from a Vec to an ArrayVec
  • Write part then return an error has the drawback is very unintuitive

I think this should become try_extend_from_slice and return a capacity error if the slice doesn't fit.

@Thomasdezeeuw Thomasdezeeuw deleted the extend_from_slice branch November 28, 2018 19:08
@Thomasdezeeuw
Copy link
Contributor Author

Ok seems good to me.

@bluss
Copy link
Owner

bluss commented Nov 28, 2018

Next release will be whenever #97 becomes "unblocked". We have merged several breaking changes, and that's important to get into the next version too.

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.

3 participants