Implement extend_from_slice and insert_from_slice with memmove optimization #29

nipunn1313 · 2016-08-23T18:40:58Z

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 · 2016-08-24T05:50:53Z

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

emilio · 2016-08-24T06:31:29Z

lib.rs

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


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

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

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

nipunn1313 · 2016-08-24T07:02:11Z

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 · 2016-08-24T07:19:50Z

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 · 2016-08-24T17:04:56Z

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

bors-servo · 2017-01-19T23:54:06Z

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

nipunn1313 · 2017-01-25T07:38:09Z

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 · 2017-01-25T14:14:28Z

@bors-servo: r+
Thanks!

bors-servo · 2017-01-25T14:14:29Z

📌 Commit aef2113 has been approved by jdm

bors-servo · 2017-01-25T14:14:32Z

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

nox · 2017-01-25T14:53:43Z

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

nipunn1313 · 2017-01-26T01:45:27Z

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 · 2017-01-26T10:30:43Z

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 · 2017-01-26T10:32:16Z

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

nipunn1313 · 2017-01-26T18:18:06Z

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

emilio reviewed Aug 24, 2016
View reviewed changes

nipunn1313 mentioned this pull request Aug 24, 2016

Migrate rlp from ElasticArray to SmallVec openethereum/parity-ethereum#1957

Closed

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 mentioned this pull request Aug 27, 2016

Add benchmarks for push, insert, extend, and pushpop #31

Merged

nipunn1313 added 4 commits January 24, 2017 23:34

Implement extend_slice and insert_slice with memcpy optimization

22bbcd2

Rename to extend_from_slice and insert_from_slice

e6e67cd

Add benchmark for extend_from_slice

0fe867e

Add bench insert_from_slice

aef2113

nipunn1313 force-pushed the extend_slice branch from 971f8fb to aef2113 Compare January 25, 2017 07:37

bors-servo merged commit aef2113 into servo:master Jan 25, 2017

Implement extend_from_slice and insert_from_slice with memmove optimization #29

Implement extend_from_slice and insert_from_slice with memmove optimization #29

Uh oh!

Conversation

nipunn1313 commented Aug 23, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Marwes commented Aug 24, 2016

Uh oh!

emilio Aug 24, 2016

Choose a reason for hiding this comment

Uh oh!

nipunn1313 Aug 24, 2016

Choose a reason for hiding this comment

Uh oh!

emilio Aug 24, 2016

Choose a reason for hiding this comment

Uh oh!

nipunn1313 commented Aug 24, 2016

Uh oh!

Marwes commented Aug 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nipunn1313 commented Aug 24, 2016

Uh oh!

bors-servo commented Jan 19, 2017

Uh oh!

nipunn1313 commented Jan 25, 2017

Uh oh!

jdm commented Jan 25, 2017

Uh oh!

bors-servo commented Jan 25, 2017

Uh oh!

bors-servo commented Jan 25, 2017

Uh oh!

nox commented Jan 25, 2017

Uh oh!

nipunn1313 commented Jan 26, 2017

Uh oh!

nox commented Jan 26, 2017

Uh oh!

nox commented Jan 26, 2017

Uh oh!

nipunn1313 commented Jan 26, 2017 via email

Uh oh!

Uh oh!

nipunn1313 commented Aug 23, 2016 •

edited by larsbergstrom

Loading

Marwes commented Aug 24, 2016 •

edited

Loading