Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Use a union to reduce the size of SmallVec #92

Closed
wants to merge 2 commits into from

Conversation

Amanieu
Copy link
Contributor

@Amanieu Amanieu commented Apr 17, 2018

Uses a union to eliminate the space used by the enum determinant. The new layout looks like this:

struct SmallVec<A> {
    capacity: usize,
    union {
        inline: ManuallyDrop<A>,
        heap: struct {
            ptr: *mut A::Item, 
            len: usize,
        } 
    }
}

The capacity field is used to determine which of the two union variants is active:

  • If capacity <= A::size() then the inline variant is used and capacity holds the current length of the vector (number of elements actually in use).
  • If capacity > A::size() then the heap variant is used and capacity holds the size of the memory allocation.

Since unions with drop are still currently unstable, this is kept behind a "union" cargo feature, which falls back to an implementation using an enum if disabled.


This change is Reviewable

@Amanieu
Copy link
Contributor Author

Amanieu commented Apr 17, 2018

Hmm there seems to be a regression in the benchmark results. I will look into this.

 name                     old ns/iter  new ns/iter  diff ns/iter   diff %  speedup 
 bench_extend             188          328                   140   74.47%   x 0.57 
 bench_extend_from_slice  48           50                      2    4.17%   x 0.96 
 bench_from_slice         47           49                      2    4.26%   x 0.96 
 bench_insert             1,125        1,335                 210   18.67%   x 0.84 
 bench_insert_from_slice  94           104                    10   10.64%   x 0.90 
 bench_insert_many        402          385                   -17   -4.23%   x 1.04 
 bench_macro_from_elem    194          257                    63   32.47%   x 0.75 
 bench_macro_from_list    43           47                      4    9.30%   x 0.91 
 bench_push               382          351                   -31   -8.12%   x 1.09 
 bench_pushpop            282          671                   389  137.94%   x 0.42 

unsafe fn heap(&self) -> (*mut A::Item, usize) {
match *self {
SmallVecData::Heap(data) => data,
_ => unreachable!(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug_unreachable may be worth in these.

@bors-servo
Copy link
Contributor

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

@eira-fransham
Copy link
Contributor

eira-fransham commented May 8, 2018

You might want to recommit this without rustfmt. Ideally, the rustfmt would be a seperate PR.

Also, is the change from an == (fast) to a < (slower) worth the 1 byte saving (plus padding)? The benchmarks suggest that it is not.

@mbrubeck mbrubeck changed the title Use a union to reduce the size of SmallVec [WIP] Use a union to reduce the size of SmallVec May 8, 2018
@Amanieu
Copy link
Contributor Author

Amanieu commented May 9, 2018

I'm going to close this since I haven't had time to work on it lately.

@Amanieu Amanieu closed this May 9, 2018
bors-servo pushed a commit that referenced this pull request Jun 6, 2018
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
```

<!-- Reviewable:start -->
---
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)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants