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

arthurprs · May 10, 2018

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

Vurich · May 22, 2018

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 · May 22, 2018

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

bors-servo · May 22, 2018

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

arthurprs · May 22, 2018

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 · May 24, 2018

psii reviewed May 24, 2018

View changes

psii · May 24, 2018

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 · May 31, 2018

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

jdm · May 31, 2018

@mbrubeck Do you have time to review these changes?

mbrubeck · Jun 1, 2018

mbrubeck reviewed Jun 1, 2018

View changes

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

mbrubeck · Jun 1, 2018

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 · Jun 1, 2018

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 · Jun 1, 2018

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 · Jun 1, 2018

lib.rs

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


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

mbrubeck · Jun 1, 2018

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 · Jun 1, 2018

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 · Jun 2, 2018

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 · Jun 6, 2018

@bors-servo r+

bors-servo · Jun 6, 2018

📌 Commit cfa1f0c has been approved by mbrubeck

bors-servo · Jun 6, 2018

⌛️ Testing commit cfa1f0c with merge 93c530f...

bors-servo · Jun 6, 2018

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

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

Vurich added a commit to Vurich/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 arthurprs:union branch from 2f677c7 to 61d1e53 May 22, 2018

Add return type to remove_noinline

Loading status checks…

4f968c9

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

Add union feature to documentation

Loading status checks…

35b43c0

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

+docs -debug_unreachable +drop_in_place

Loading status checks…

cfa1f0c

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

arthurprs referenced this pull request Jun 19, 2018
Closed
Use optimized SmallVec crate #51640

mbrubeck referenced this pull request Jul 19, 2018
Merged
Version 0.6.3 #105

mbrubeck referenced this pull request Jul 30, 2018
Open
[meta] Wishlist for 1.0 #73
2 of 10 tasks complete

servo/rust-smallvec

Join GitHub today

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

Conversation

arthurprs commented May 10, 2018 • edited by larsbergstrom

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

This comment has been minimized.

Vurich commented May 22, 2018 • edited

This comment has been minimized.

arthurprs commented May 22, 2018

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

This comment has been minimized.

bors-servo commented May 22, 2018

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

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

This comment has been minimized.

arthurprs commented May 22, 2018

psii reviewed May 24, 2018 View changes

This comment has been minimized.

This comment has been minimized.

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

This comment has been minimized.

arthurprs commented May 31, 2018

This comment has been minimized.

jdm commented May 31, 2018

mbrubeck reviewed Jun 1, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mbrubeck commented Jun 2, 2018

This comment has been minimized.

mbrubeck commented Jun 6, 2018

This comment has been minimized.

bors-servo commented Jun 6, 2018

This comment has been minimized.

bors-servo commented Jun 6, 2018

bors-servo added a commit that referenced this pull request Jun 6, 2018

This comment has been minimized.

bors-servo commented Jun 6, 2018

Hide details View details bors-servo merged commit cfa1f0c into servo:master Jun 6, 2018 2 checks passed

2 checks passed

arthurprs referenced this pull request Jun 19, 2018

mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this pull request Jul 19, 2018

mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this pull request Jul 19, 2018

mbrubeck referenced this pull request Jul 19, 2018

bors-servo added a commit that referenced this pull request Jul 19, 2018

mbrubeck referenced this pull request Jul 30, 2018

arthurprs commented May 10, 2018 •

edited by larsbergstrom

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

Vurich commented May 22, 2018 •

edited

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

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

psii reviewed May 24, 2018

View changes

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

mbrubeck reviewed Jun 1, 2018

View changes

bors-servo merged commit `cfa1f0c` into servo:master Jun 6, 2018
2 checks passed