-
Notifications
You must be signed in to change notification settings - Fork 151
Implement extend_from_slice and insert_from_slice with memmove optimization #29
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
Conversation
Might want to call |
@@ -315,6 +315,29 @@ impl<A: Array> SmallVec<A> { | |||
} | |||
} | |||
|
|||
impl<A: Array> SmallVec<A> where A::Item: Copy { | |||
pub fn insert_slice(&mut self, index: usize, slice: &[A::Item]) { | |||
self.reserve(slice.len()); |
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.
Hmm... I'm kind of sleepy right now, but I think here you should reserve index + slice.len()
?
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.
I believe reserve takes in an "additional" amount required and ensures the capacity is large enough, so I think this is ok.
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.
Oh, you're right, nvm, I thought reserve()
did the usual stuff, that is, ensuring capacity.
I think "extend_from_slice" and "insert_from_slice" are also reasonable names. I think "copy_from_slice" is misleading because it's not overwriting existing contents. |
extend_from_slice is also a method on |
That makes sense. I agree with that. |
Add benchmarks for push, insert, extend, and pushpop Want to add these benchmarks so that we can more effectively detect regressions/improvements from PR's like #28 and #29. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/31) <!-- Reviewable:end -->
☔ The latest upstream changes (presumably #28) made this pull request unmergeable. Please resolve the merge conflicts. |
971f8fb
to
aef2113
Compare
I went ahead and rebased this on master. Here are the benchmarks on my local machine
Looks like there shouldn't be regressions, though we should be extra careful to make sure there aren't integer overflow / memory issues. |
@bors-servo: r+ |
📌 Commit aef2113 has been approved by |
⚡ Test exempted - status |
Implement extend_from_slice and insert_from_slice with memmove optimization Implement the performance optimized versions of insert_many and extend (from #28). These methods use memmove rather than a looped insert. If we had function specialization, we could implement these without exposing new methods. Up to the maintainers whether we want to support these new methods. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/29) <!-- Reviewable:end -->
Why does that use |
unsafe ptr::copy will only work if the type is Copy. If function specialization lands (rust-lang/rust#31844), then this naming / perf tradeoff may become moot. |
The stdlib uses |
Interesting! That looks like it uses specialization. At the time we made
this PR, specialization wasn't available.
--Nipunn
…On Thu, Jan 26, 2017 at 2:32 AM Anthony Ramine ***@***.***> wrote:
Ah no, nevermind:
https://github.com/rust-lang/rust/blob/master/src/libcollections/vec.rs#L1691-L1703
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#29 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABPXow0e9dYXtbdo2Rm412-xJaPeOGIkks5rWHYxgaJpZM4JrQG6>
.
|
Implement the performance optimized versions of insert_many and extend (from #28). These methods use memmove rather than a looped insert.
If we had function specialization, we could implement these without exposing new methods. Up to the maintainers whether we want to support these new methods.
This change is