Skip to content

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

Merged
merged 4 commits into from
Jan 25, 2017

Conversation

nipunn1313
Copy link
Contributor

@nipunn1313 nipunn1313 commented Aug 23, 2016

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 Reviewable

@Marwes
Copy link
Contributor

Marwes commented Aug 24, 2016

Might want to call extend_slice, copy_from_slice to mimic the method onVec`

@@ -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());
Copy link
Member

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()?

Copy link
Contributor Author

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.

Copy link
Member

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.

@nipunn1313
Copy link
Contributor Author

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.

@Marwes
Copy link
Contributor

Marwes commented Aug 24, 2016

extend_from_slice is also a method on Vec except it takes &[T] where T: Clone. Even if they are not the best names it might still be best to mimic Vec for naming to keep things consistent (but then again I am not a maintainer of this lib so take it only as a suggestion).

@nipunn1313 nipunn1313 changed the title Implement extend_slice and insert_slice with memmove optimization Implement extend_from_slice and insert_from_slice with memmove optimization Aug 24, 2016
@nipunn1313
Copy link
Contributor Author

That makes sense. I agree with that.

bors-servo pushed a commit that referenced this pull request Sep 11, 2016
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 -->
@bors-servo
Copy link
Contributor

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

@nipunn1313
Copy link
Contributor Author

I went ahead and rebased this on master. Here are the benchmarks on my local machine

Nipunns-MacBook-Pro:rust-smallvec nipunn$ cargo bench bench
    Finished release [optimized] target(s) in 0.0 secs
     Running target/release/bench-760919c65e248f9d

running 7 tests
test bench_extend            ... bench:         149 ns/iter (+/- 26)
test bench_extend_from_slice ... bench:          65 ns/iter (+/- 8)
test bench_insert            ... bench:       1,411 ns/iter (+/- 440)
test bench_insert_from_slice ... bench:         119 ns/iter (+/- 22)
test bench_insert_many       ... bench:         483 ns/iter (+/- 59)
test bench_push              ... bench:         425 ns/iter (+/- 217)
test bench_pushpop           ... bench:         351 ns/iter (+/- 78)

test result: ok. 0 passed; 0 failed; 0 ignored; 7 measured

     Running target/release/deps/smallvec-82287e02aa187b8d

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured

Nipunns-MacBook-Pro:rust-smallvec nipunn$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
Nipunns-MacBook-Pro:rust-smallvec nipunn$ cargo bench bench
   Compiling smallvec v0.3.1 (file:///Users/nipunn/src/rust-smallvec)
    Finished release [optimized] target(s) in 2.54 secs
     Running target/release/bench-760919c65e248f9d

running 5 tests
test bench_extend      ... bench:         151 ns/iter (+/- 16)
test bench_insert      ... bench:       1,407 ns/iter (+/- 369)
test bench_insert_many ... bench:         480 ns/iter (+/- 75)
test bench_push        ... bench:         430 ns/iter (+/- 152)
test bench_pushpop     ... bench:         353 ns/iter (+/- 104)

test result: ok. 0 passed; 0 failed; 0 ignored; 5 measured

     Running target/release/deps/smallvec-82287e02aa187b8d

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured

Looks like there shouldn't be regressions, though we should be extra careful to make sure there aren't integer overflow / memory issues.

@jdm
Copy link
Member

jdm commented Jan 25, 2017

@bors-servo: r+
Thanks!

@bors-servo
Copy link
Contributor

📌 Commit aef2113 has been approved by jdm

@bors-servo
Copy link
Contributor

⚡ Test exempted - status

@bors-servo bors-servo merged commit aef2113 into servo:master Jan 25, 2017
bors-servo pushed a commit that referenced this pull request Jan 25, 2017
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 -->
@nox
Copy link
Contributor

nox commented Jan 25, 2017

Why does that use Copy instead of Clone like the stdlib?

@nipunn1313
Copy link
Contributor Author

unsafe ptr::copy will only work if the type is Copy.
If the type is clone, then we have to iterate over the elements and run the clone code.
You're right that it's a little bit different than the stdlib which is unfortunate.

If function specialization lands (rust-lang/rust#31844), then this naming / perf tradeoff may become moot.

@nox
Copy link
Contributor

nox commented Jan 26, 2017

The stdlib uses Clone but manages to optimise it nonetheless without specialisation AFAIK. Just because you iterate and clone doesn't mean it has to be slow.

@nox
Copy link
Contributor

nox commented Jan 26, 2017

@nipunn1313
Copy link
Contributor Author

nipunn1313 commented Jan 26, 2017 via email

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.

6 participants