Implement extend_from_slice and insert_from_slice with memmove optimization

[nipunn1313]

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

[Marwes]

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

[emilio]

Hmm... I'm kind of sleepy right now, but I think here you should reserve index + slice.len()?

[nipunn1313]

I believe reserve takes in an "additional" amount required and ensures the capacity is large enough, so I think this is ok.

[emilio]

Oh, you're right, nvm, I thought reserve() did the usual stuff, that is, ensuring capacity.

[nipunn1313]

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]

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]

That makes sense. I agree with that.

[bors-servo]

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

[nipunn1313]

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]

@bors-servo: r+
Thanks!

[bors-servo]

📌 Commit aef2113 has been approved by jdm

[bors-servo]

⚡ Test exempted - status

[nox]

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

[nipunn1313]

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]

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]

Ah no, nevermind: https://github.com/rust-lang/rust/blob/master/src/libcollections/vec.rs#L1691-L1703

[nipunn1313]

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> .