Skip to content

Add extend_from_array #79892

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

Closed
wants to merge 4 commits into from
Closed

Conversation

scottmcm
Copy link
Member

This PR inspired by noticing that the following, while it seems reasonable, actually contains 4 calls to reserve in the ASM:

pub fn push3_demo(v: &mut Vec<i32>, a: i32, b: i32, c: i32) {
    v.reserve(3);
    v.push(a);
    v.push(b);
    v.push(c);
}

https://rust.godbolt.org/z/EWExYP

Like there's extend_from_slice for cloning, it seems like it would be logical to have an extend_from_array to encapsulate the obvious unsafe code for moving the elements to the end of the vector. And, indeed, that turns out to generate tighter assembly than any of the other options I tried, including array::IntoIter and chains-of-onces: https://rust.godbolt.org/z/Wq7qah

I ended up putting this on Extend, as it makes particular sense on anything that can take advantage of the memory continuity of the array -- this PR implements it for Vec and VecDeque -- but would also be handy as a way to tersely add a known number of elements to any collection by move. It has the nice default implementation in terms of the array IntoIter, so that things without special overrides for it can still do something reasonable, as that provides an exact size_hint.

But touching a trait means it should probably get some libs eyes on it even to go in unstable, so let's try
r? @m-ou-se

(A different approach for this could be something like push_chunk, in the sense of as_chunks.)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 10, 2020
@jyn514 jyn514 added I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 10, 2020
@scottmcm
Copy link
Member Author

(Hmm, that codegen test passed locally. I'll leave it for now, and fix it along with adding a tracking issue if this gets a "seems plausible".)

@scottmcm
Copy link
Member Author

Thanks, @erikdesjardins! That sounds plausible.

And because that means this might fail in bors even after passing CI,
@bors rollup=iffy

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me, but once IntoIterator is implemented for arrays (#65819), v.extend_from_array([1, 2, 3]) would be equivalent to just v.extend([1, 2, 3]), making this new api redundant, right? (Maybe with a specialization or two in its implementation.)

Comment on lines +2291 to +2292
unsafe {
let end = self.as_mut_ptr().add(self.len);
Copy link
Member

Choose a reason for hiding this comment

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

There's a safe way to get the end pointer: self.as_mut_ptr_range().end

@bors
Copy link
Collaborator

bors commented Dec 18, 2020

☔ The latest upstream changes (presumably #80138) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 30, 2020
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2021
@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 12, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2021
@scottmcm
Copy link
Member Author

scottmcm commented Mar 1, 2021

I'm not going to be able to look at this for a bit, with edition stuff being higher priority. I'll close for now, and maybe will reopen as not a trait method in the future.

@scottmcm scottmcm closed this Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants