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


 [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

    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

 /// ```
 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

12 tasks

bruceg mentioned this pull request Oct 22, 2021

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

Merged

Conversation

arthurprs commented May 10, 2018 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eira-fransham commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurprs commented May 22, 2018

Uh oh!

bors-servo commented May 22, 2018

Uh oh!

arthurprs commented May 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthurprs commented May 31, 2018

Uh oh!

jdm commented May 31, 2018

Uh oh!

mbrubeck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbrubeck commented Jun 2, 2018

Uh oh!

mbrubeck commented Jun 6, 2018

Uh oh!

bors-servo commented Jun 6, 2018

Uh oh!

bors-servo commented Jun 6, 2018

Uh oh!

bors-servo commented Jun 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

arthurprs commented May 10, 2018 •

edited by larsbergstrom

Loading

eira-fransham commented May 22, 2018 •

edited

Loading