Use a union to reduce the size of SmallVec [v2] #94

arthurprs · 2018-05-10T20:35:51Z

Building on top of #92 by @Amanieu

I introduced triple() and triple_mut() which removed almost all of the runtime overhead. Performance is very comparable.

 name                     master:: ns/iter  union:: ns/iter  diff ns/iter   diff %  speedup 
 bench_extend             45                47                          2    4.44%   x 0.96 
 bench_extend_from_slice  45                43                         -2   -4.44%   x 1.05 
 bench_from_slice         45                44                         -1   -2.22%   x 1.02 
 bench_insert             615               622                         7    1.14%   x 0.99 
 bench_insert_from_slice  101               99                         -2   -1.98%   x 1.02 
 bench_insert_many        309               266                       -43  -13.92%   x 1.16 
 bench_macro_from_elem    41                37                         -4   -9.76%   x 1.11 
 bench_macro_from_list    40                42                          2    5.00%   x 0.95 
 bench_push               381               370                       -11   -2.89%   x 1.03 
 bench_pushpop            404               420                        16    3.96%   x 0.96 
 bench_remove             458               436                       -22   -4.80%   x 1.05

This change is

eira-fransham · 2018-05-22T09:50:56Z

Your benchmark changes will definitely cause conflicts with #97, I don't know if you want to add your bench changes as a seperate PR to avoid that (I don't mind rolling them into #97 instead if you like).

arthurprs · 2018-05-22T09:58:53Z

@Vurich fell free to put them into #97 and I'll rebase this.

bors-servo · 2018-05-22T15:30:02Z

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

arthurprs · 2018-05-22T17:51:08Z

I rebased on master now, the only change I kept was to add the return type to remove_no_inline (to prevent over optimization, specially for Vec).

psii · 2018-05-24T12:13:59Z

lib.rs

+//! Note that `smallvec` can still be larger than `Vec` if the inline buffer is larger than two
+//! machine words.
+//!
+//! To use this feature add `features = ["uuid"]` in the `smallvec` section of Cargo.toml.


Should be union instead of uuid, right?

Ops, will fix.

arthurprs · 2018-05-31T18:05:52Z

It'd be nice to get this moving. It may be interesting for rustc as well.

jdm · 2018-05-31T19:27:30Z

@mbrubeck Do you have time to review these changes?

mbrubeck

This is great, thanks! Some requests/suggestions below.

mbrubeck · 2018-06-01T20:50:58Z

Cargo.toml

 default = ["std"]

 [lib]
 name = "smallvec"
 path = "lib.rs"

 [dependencies]
+unreachable = "1.0.0"
+debug_unreachable = "0.1"


We may not want to use debug_unreachable, because it is unmaintained and doesn't work correctly in Rust > 1.0: reem/rust-debug-unreachable#6

We could just use unreachable all the time, or write our own debug_unreachable macro, or write a new debug_unreachable-like crate...

Makes sense, I'll remove the dependency and add the equivalent macro.

mbrubeck · 2018-06-01T21:00:44Z

lib.rs

@@ -350,14 +419,25 @@ impl<A: Array> SmallVec<A> {
    /// ```
    #[inline]
    pub fn from_vec(mut vec: Vec<A::Item>) -> SmallVec<A> {
-        let (ptr, cap, len) = (vec.as_mut_ptr(), vec.capacity(), vec.len());
-        mem::forget(vec);
+        if vec.capacity() <= A::size() {


The documentation says that from_vec does not copy the elements. I think we should keep the current behavior of keeping the items on the heap. Users who want to move them inline if possible can use shrink_to_fit or other methods.

This is unfortunately not possible with the new design: there is no way of representing a heap-allocated buffer with a capacity less than A::size(). If self.capacity <= A::size() then the code assumes that the data is in the inline buffer.

Oh, right. In that case the documentation should be updated.

mbrubeck · 2018-06-01T21:05:33Z

lib.rs

@@ -301,7 +370,7 @@ impl<A: Array> Drop for SmallVecData<A> {
 /// assert!(v.spilled());
 /// ```
 pub struct SmallVec<A: Array> {
-    len: usize,
+    capacity: usize,


Using capacity here is a little confusing to me, but I'm not sure what a better name would be. Maybe something like len_cap? Maybe capacity is okay, but there should be a comment here describing the semantics.

Good point.

mbrubeck · 2018-06-01T21:06:32Z

lib.rs

+    }
+
+    #[inline]
+    fn triple(&self) -> (*const A::Item, usize, usize) {


Please add a comment to say what the return value means.

mbrubeck · 2018-06-01T21:31:38Z

lib.rs

+            while len < *len_ptr {
+                let last_index = *len_ptr - 1;
+                *len_ptr = last_index;
+                ptr::read(ptr.offset(last_index as isize));


This could use ptr::drop_in_place instead of ptr::read.

mbrubeck · 2018-06-01T21:31:47Z

lib.rs

+            } else {
+                let ptr = self.as_ptr();
+                for i in 0..self.len() {
+                    ptr::read(ptr.offset(i as isize));


This could use ptr::drop_in_place instead.

mbrubeck · 2018-06-02T16:32:52Z

Great, thanks! I think this is ready but I just want to double-check all the unsafe code before merging it (and give a chance for others to do the same).

mbrubeck · 2018-06-06T16:12:09Z

@bors-servo r+

bors-servo · 2018-06-06T16:12:10Z

📌 Commit cfa1f0c has been approved by mbrubeck

bors-servo · 2018-06-06T16:12:14Z

⌛ Testing commit cfa1f0c with merge 93c530f...

@Amanieu

Use a union to reduce the size of SmallVec [v2] Building on top of #92 by @Amanieu I introduced `triple()` and `triple_mut()` which removed almost all of the runtime overhead. Performance is very comparable. ``` name master:: ns/iter union:: ns/iter diff ns/iter diff % speedup bench_extend 45 47 2 4.44% x 0.96 bench_extend_from_slice 45 43 -2 -4.44% x 1.05 bench_from_slice 45 44 -1 -2.22% x 1.02 bench_insert 615 622 7 1.14% x 0.99 bench_insert_from_slice 101 99 -2 -1.98% x 1.02 bench_insert_many 309 266 -43 -13.92% x 1.16 bench_macro_from_elem 41 37 -4 -9.76% x 1.11 bench_macro_from_list 40 42 2 5.00% x 0.95 bench_push 381 370 -11 -2.89% x 1.03 bench_pushpop 404 420 16 3.96% x 0.96 bench_remove 458 436 -22 -4.80% x 1.05 ```  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/94)

bors-servo · 2018-06-06T16:15:51Z

☀️ Test successful - status-travis
Approved by: mbrubeck
Pushing 93c530f to master...

Changes in this release: * Fix possible double-free in `insert_many` when passed an iterator that panics in `next` (servo#103) * Add a new `union` feature (disabled by default) that reduces the size of the SmallVec struct (servo#94) * Improve performance of `extend` and `from_elem` (servo#93) * Improve performance of `drop` (servo#100) * Update dev-dependency on `bincode` (servo#102) * Update to build without `std` on current Rust nightly (servo#104) * Additional benchmarks (servo#95, servo#97).

Version 0.6.3 Changes in this release: * Fix possible double-free in `insert_many` when passed an iterator that panics in `next` (#103) * Add a new `union` feature (disabled by default) that reduces the size of the SmallVec struct (#94) * Improve performance of `extend` and `from_elem` (#93) * Improve performance of `drop` (#100) * Update to build without `std` feature on current Rust nightly (#104) * Additional benchmarks (#95, #97) * Update dev-dependency on `bincode` (#102)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/105)

This is what the standard Vec does; see rust-lang/rust#32012.

arthurprs force-pushed the union branch from 14d49b6 to 2f677c7 Compare May 10, 2018 20:50

eira-fransham added a commit to eira-fransham/rust-smallvec that referenced this pull request May 22, 2018

Roll in improvements from servo#94

25255c8

Use a union to reduce the size of SmallVec

6ff944d

arthurprs force-pushed the union branch from 2f677c7 to 61d1e53 Compare May 22, 2018 17:33

Add return type to remove_noinline

4f968c9

arthurprs force-pushed the union branch from 61d1e53 to 4f968c9 Compare May 22, 2018 17:37

psii reviewed May 24, 2018

View reviewed changes

Add union feature to documentation

35b43c0

arthurprs force-pushed the union branch from 8c76eeb to 35b43c0 Compare May 24, 2018 16:32

mbrubeck reviewed Jun 1, 2018

View reviewed changes

+docs -debug_unreachable +drop_in_place

cfa1f0c

bors-servo merged commit cfa1f0c into servo:master Jun 6, 2018

arthurprs mentioned this pull request Jun 19, 2018

Use optimized SmallVec crate rust-lang/rust#51640

Closed

mbrubeck mentioned this pull request Jul 19, 2018

Version 0.6.3 #105

Merged

mbrubeck mentioned this pull request Jul 30, 2018

[meta] Wishlist for smallvec 1.0 #73

Closed

12 tasks

jesskfullwood referenced this pull request Jun 30, 2019

Use a single ptr::drop_in_place call instead of a loop

b24b3d2

This is what the standard Vec does; see rust-lang/rust#32012.

mbrubeck mentioned this pull request Nov 19, 2019

[meta] Wishlist for smallvec 2.0 #183

Open

11 tasks

bruceg mentioned this pull request Oct 22, 2021

chore(performance): Enable the "union" feature on SmallVec vectordotdev/vector#9772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a union to reduce the size of SmallVec [v2] #94

Use a union to reduce the size of SmallVec [v2] #94

arthurprs commented May 10, 2018 •

edited by larsbergstrom

Loading

eira-fransham commented May 22, 2018 •

edited

Loading

arthurprs commented May 22, 2018

bors-servo commented May 22, 2018

arthurprs commented May 22, 2018

psii May 24, 2018

arthurprs May 24, 2018

arthurprs commented May 31, 2018

jdm commented May 31, 2018

mbrubeck left a comment

mbrubeck Jun 1, 2018

arthurprs Jun 2, 2018

mbrubeck Jun 1, 2018

Amanieu Jun 1, 2018

mbrubeck Jun 1, 2018

mbrubeck Jun 1, 2018

arthurprs Jun 2, 2018

mbrubeck Jun 1, 2018

mbrubeck Jun 1, 2018

mbrubeck Jun 1, 2018

mbrubeck commented Jun 2, 2018

mbrubeck commented Jun 6, 2018

bors-servo commented Jun 6, 2018

bors-servo commented Jun 6, 2018

bors-servo commented Jun 6, 2018

Use a union to reduce the size of SmallVec [v2] #94

Use a union to reduce the size of SmallVec [v2] #94

Conversation

arthurprs commented May 10, 2018 • edited by larsbergstrom Loading

eira-fransham commented May 22, 2018 • edited Loading

arthurprs commented May 22, 2018

bors-servo commented May 22, 2018

arthurprs commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurprs commented May 31, 2018

jdm commented May 31, 2018

mbrubeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrubeck commented Jun 2, 2018

mbrubeck commented Jun 6, 2018

bors-servo commented Jun 6, 2018

bors-servo commented Jun 6, 2018

bors-servo commented Jun 6, 2018

arthurprs commented May 10, 2018 •

edited by larsbergstrom

Loading

eira-fransham commented May 22, 2018 •

edited

Loading