Implementation of RFC #240 #284

sarah-quinones · 2022-08-01T16:06:18Z

Implementation of the API described in #240
The API and tests were modified and all the tests pass.
So far the docs aren't written yet. But I wanted to hear the feedback on the current progress before going further.

current benchmark results: there are a few regressions but i believe i can handle them

                                                                       master                 this PR       relative difference (lower is better)
test bench_extend                      ... bench:          33 ns/iter (+/- 1)      29 ns/iter (+/- 0)                    -0.121
test bench_extend_from_slice           ... bench:          25 ns/iter (+/- 0)      23 ns/iter (+/- 0)                    -0.079
test bench_extend_from_slice_small     ... bench:           9 ns/iter (+/- 0)       8 ns/iter (+/- 0)                    -0.111
test bench_extend_from_slice_vec       ... bench:          53 ns/iter (+/- 3)      52 ns/iter (+/- 3)                    -0.018
test bench_extend_from_slice_vec_small ... bench:          16 ns/iter (+/- 0)      15 ns/iter (+/- 0)                    -0.062
test bench_extend_small                ... bench:          10 ns/iter (+/- 0)      11 ns/iter (+/- 0)                    +0.100
test bench_extend_vec                  ... bench:          53 ns/iter (+/- 1)      51 ns/iter (+/- 2)                    -0.037
test bench_extend_vec_small            ... bench:          12 ns/iter (+/- 0)      12 ns/iter (+/- 0)                    +0.0
test bench_from_iter                   ... bench:          17 ns/iter (+/- 0)      35 ns/iter (+/- 0)                    +1.058
test bench_from_iter_small             ... bench:           3 ns/iter (+/- 0)      11 ns/iter (+/- 1)                    +2.666
test bench_from_iter_vec               ... bench:          18 ns/iter (+/- 1)      17 ns/iter (+/- 0)                    -0.055
test bench_from_iter_vec_small         ... bench:          12 ns/iter (+/- 0)      12 ns/iter (+/- 0)                    +0.0
test bench_from_slice                  ... bench:          17 ns/iter (+/- 0)      22 ns/iter (+/- 0)                    +0.294
test bench_from_slice_small            ... bench:           3 ns/iter (+/- 0)       7 ns/iter (+/- 0)                    +1.333
test bench_from_slice_vec              ... bench:          18 ns/iter (+/- 0)      17 ns/iter (+/- 0)                    -0.055
test bench_from_slice_vec_small        ... bench:          12 ns/iter (+/- 0)      12 ns/iter (+/- 0)                    +0.0
test bench_insert                      ... bench:      1,137 ns/iter  (+/- 34)    960 ns/iter (+/- 18)                   -0.155
test bench_insert_from_slice           ... bench:         81 ns/iter  (+/- 10)     66 ns/iter (+/- 1)                    -0.185
test bench_insert_many                 ... bench:         119 ns/iter (+/- 2)      85 ns/iter (+/- 2)                    -0.285
test bench_insert_push                 ... bench:        548 ns/iter  (+/- 10)   415 ns/iter  (+/- 54)                   -0.242
test bench_insert_push_small           ... bench:          75 ns/iter (+/- 2)      54 ns/iter (+/- 1)                    -0.28
test bench_insert_push_vec             ... bench:         505 ns/iter (+/- 9)     467 ns/iter (+/- 13                    -0.075
test bench_insert_push_vec_small       ... bench:          75 ns/iter (+/- 1)      72 ns/iter (+/- 0)                    -0.040
test bench_insert_small                ... bench:        115 ns/iter  (+/- 34)     93 ns/iter (+/- 3)                    -0.191
test bench_insert_vec                  ... bench:        941 ns/iter  (+/- 13)    958 ns/iter (+/- 17)                   +0.018
test bench_insert_vec_small            ... bench:         134 ns/iter (+/- 1)     143 ns/iter (+/- 2)                    +0.067
test bench_macro_from_elem             ... bench:          18 ns/iter (+/- 0)      18 ns/iter (+/- 0)                    +0.0
test bench_macro_from_elem_small       ... bench:           8 ns/iter (+/- 0)       8 ns/iter (+/- 0)                    +0.0
test bench_macro_from_elem_vec         ... bench:          18 ns/iter (+/- 0)      18 ns/iter (+/- 0)                    +0.0
test bench_macro_from_elem_vec_small   ... bench:          14 ns/iter (+/- 0)      14 ns/iter (+/- 0)                    +0.0
test bench_macro_from_list             ... bench:          10 ns/iter (+/- 0)      10 ns/iter (+/- 0)                    +0.0
test bench_macro_from_list_vec         ... bench:          10 ns/iter (+/- 0)      10 ns/iter (+/- 0)                    +0.0
test bench_push                        ... bench:         441 ns/iter (+/- 9)     355 ns/iter (+/- 52)                   -0.195
test bench_push_small                  ... bench:          63 ns/iter (+/- 0)      43 ns/iter (+/- 0)                    -0.317
test bench_push_vec                    ... bench:         283 ns/iter (+/- 4)     277 ns/iter (+/- 9)                    -0.021
test bench_push_vec_small              ... bench:          51 ns/iter (+/- 15)     52 ns/iter (+/- 4)                    +0.019
test bench_pushpop                     ... bench:         752 ns/iter (+/- 2)     313 ns/iter (+/- 5)                    -0.583
test bench_pushpop_vec                 ... bench:         227 ns/iter (+/- 5)     230 ns/iter (+/- 4)                    +0.013
test bench_remove                      ... bench:         717 ns/iter (+/- 14)    781 ns/iter (+/- 11                    +0.089
test bench_remove_small                ... bench:          74 ns/iter (+/- 0)      71 ns/iter (+/- 0)                    -0.040
test bench_remove_vec                  ... bench:         793 ns/iter (+/- 5)     798 ns/iter (+/- 11                    +0.006
test bench_remove_vec_small            ... bench:          95 ns/iter (+/- 2)      91 ns/iter (+/- 3)                    -0.042

mbrubeck · 2022-08-01T16:46:03Z

This is great! I haven't had time to review the code yet, but I really appreciate you working on this.

- document unsafe code - improve perf for extend - add benchmark for extend

sarah-quinones · 2022-08-02T07:49:10Z

i made some improvements. here are the updated benchmarks.

                                                                master                      this PR             relative difference
test bench_extend                      ... bench:          33 ns/iter (+/- 1)            24 ns/iter (+/- 0)      -0.2727272727272727
test bench_extend_from_slice           ... bench:          24 ns/iter (+/- 2)            20 ns/iter (+/- 0)      -0.16666666666666663
test bench_extend_from_slice_small     ... bench:           9 ns/iter (+/- 0)             8 ns/iter (+/- 0)      -0.11111111111111116
test bench_extend_small                ... bench:          10 ns/iter (+/- 0)            10 ns/iter (+/- 0)
test bench_from_iter                   ... bench:          17 ns/iter (+/- 0)            17 ns/iter (+/- 0)
test bench_from_iter_small             ... bench:           3 ns/iter (+/- 0)             4 ns/iter (+/- 0)      +0.33333333333333326
test bench_from_slice                  ... bench:          17 ns/iter (+/- 0)            17 ns/iter (+/- 2)
test bench_from_slice_small            ... bench:           3 ns/iter (+/- 0)             4 ns/iter (+/- 0)      +0.33333333333333326
test bench_insert                      ... bench:        1208 ns/iter (+/- 21)          976 ns/iter (+/- 42)     -0.1920529801324503
test bench_insert_from_slice           ... bench:          80 ns/iter (+/- 1)            94 ns/iter (+/- 11)     +0.17500000000000004
test bench_insert_many                 ... bench:         119 ns/iter (+/- 1)            84 ns/iter (+/- 3)      -0.2941176470588235
test bench_insert_push                 ... bench:         538 ns/iter (+/- 15)          408 ns/iter (+/- 21)     -0.241635687732342
test bench_insert_push_small           ... bench:          75 ns/iter (+/- 3)            54 ns/iter (+/- 5)      -0.28
test bench_insert_small                ... bench:         115 ns/iter (+/- 3)           104 ns/iter (+/- 10)     -0.09565217391304348
test bench_macro_from_elem             ... bench:          18 ns/iter (+/- 0)            18 ns/iter (+/- 0)
test bench_macro_from_elem_small       ... bench:           9 ns/iter (+/- 0)             8 ns/iter (+/- 0)      -0.11111111111111116
test bench_macro_from_list             ... bench:          10 ns/iter (+/- 0)            10 ns/iter (+/- 0)
test bench_push                        ... bench:         440 ns/iter (+/- 10)          369 ns/iter (+/- 7)      -0.16136363636363638
test bench_push_small                  ... bench:          63 ns/iter (+/- 0)            42 ns/iter (+/- 0)      -0.33333333333333337
test bench_pushpop                     ... bench:         754 ns/iter (+/- 5)           526 ns/iter (+/- 6)      -0.3023872679045093
test bench_remove                      ... bench:         714 ns/iter (+/- 26)          869 ns/iter (+/- 9)      +0.21708683473389345
test bench_remove_small                ... bench:          75 ns/iter (+/- 1)            71 ns/iter (+/- 4)      -0.053333333333333344

the one regression that isn't just benchmark noise is bench_remove. and most of the others got noticeable improvements

sarah-quinones · 2022-08-02T07:55:05Z

actually, it's strange. i'm seeing a regression for bench_remove_vec as well with similar timings. i think this is some benchmark weirdness happening here like code alignment for remove_noinline, since my remove implementation is essentially the same as the one on master.

mbrubeck · 2022-08-20T12:32:15Z

Note: I am beginning to review this code, and should have any feedback ready within a few days.

bors-servo · 2022-09-12T18:33:46Z

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

mbrubeck

This looks great! Sorry it took so long to review.

If you would like, we can rebase and merge this as-is, and then deal with remaining issues (like doc comments, performance work, and filling in any missing features) in follow-up patches, which I'm happy to do myself or to accept PRs for.

mbrubeck · 2022-09-12T18:47:07Z

src/lib.rs

+    }
+
+    #[inline]
+    pub const fn value(self, is_zst: bool) -> usize {


What do you thank of making TaggedLen be generic (TaggedLen<T>), so we wouldn't need to pass is_zst arguments to its methods?

there's the issue of the variance of TaggedLen with respect to T. it has to be covariant so that the vector is also covariant, but i don't feel like that option makes sense to me since it doesn't really contain a T (maybe have it contain a PhantomData<fn() -> T>?). ideally i'd like to have TaggedLen<const IS_ZST: bool>(...) but we run into current limitations of const generics when doing that.

i'd personally keep it as is but the decision is up to you

Okay, that makes sense.

so what's the next step? should i port over the additions from upstream or do we merge this as is?

Sorry again for the radio silence. My plan is to merge this as-is, effectively making this branch the new mainline, and open issues for porting the remaining changes and any other missing pieces. The current master branch will become a v1 branch, for bug fixes only. I'm not sure when I'll have time to get to this, but it's still on my to-do list.

no problem! let me know if i can be of any help

corvusrabus · 2022-11-28T20:31:28Z

Hi, I might be missing something but it looks to me as if
unsafe impl<A: Array> Send for SmallVec<A> where A::Item: Send {}
is missing. I expect that the vector should be Send if the items are Send

mbrubeck · 2023-06-17T00:13:26Z

Sorry, it has been a very long time but I have not forgotten this. I will make this the main branch as soon as smallvec 1.11.0 has been released (#300).

mbrubeck · 2023-07-23T17:12:54Z

This code is now on the default branch of the repository. The next steps will be porting changes from the v1 branch since it has diverged from this branch, as well as filling in any missing documentation, tests, or other features.

We should write up an issue with a more detailed list of work remaining...

sarah el kazdadi added 2 commits August 1, 2022 17:36

feat: implement rfc240

b0e2960

cleanup

c182d10

sarah el kazdadi and others added 4 commits August 1, 2022 19:42

test: fix fuzzing

c8edfa2

perf: implement specialization based dispatch for improved perf

3e2eeca

chore: import core::ptr::copy instead of qualifying it

0951617

doc,perf,bench:

741ab83

- document unsafe code - improve perf for extend - add benchmark for extend

mbrubeck approved these changes Sep 12, 2022

View reviewed changes

impl Send/Sync for SmallVec

731f6bc

mbrubeck mentioned this pull request Jun 17, 2023

Version 1.11.0 #300

Merged

This was referenced Jul 5, 2023

Add from_const_with_len #301

Closed

Make the v2 branch the default branch #302

Closed

mbrubeck deleted the branch servo:master September 20, 2023 17:19

mbrubeck closed this Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of RFC #240 #284

Implementation of RFC #240 #284

sarah-quinones commented Aug 1, 2022

mbrubeck commented Aug 1, 2022

sarah-quinones commented Aug 2, 2022

sarah-quinones commented Aug 2, 2022 •

edited

Loading

mbrubeck commented Aug 20, 2022

bors-servo commented Sep 12, 2022

mbrubeck left a comment

mbrubeck Sep 12, 2022

sarah-quinones Sep 12, 2022

mbrubeck Sep 13, 2022

sarah-quinones Oct 6, 2022

mbrubeck Oct 16, 2022

sarah-quinones Oct 17, 2022

corvusrabus commented Nov 28, 2022

mbrubeck commented Jun 17, 2023

mbrubeck commented Jul 23, 2023

Implementation of RFC #240 #284

Implementation of RFC #240 #284

Conversation

sarah-quinones commented Aug 1, 2022

mbrubeck commented Aug 1, 2022

sarah-quinones commented Aug 2, 2022

sarah-quinones commented Aug 2, 2022 • edited Loading

mbrubeck commented Aug 20, 2022

bors-servo commented Sep 12, 2022

mbrubeck left a comment

Choose a reason for hiding this comment

mbrubeck Sep 12, 2022

Choose a reason for hiding this comment

sarah-quinones Sep 12, 2022

Choose a reason for hiding this comment

mbrubeck Sep 13, 2022

Choose a reason for hiding this comment

sarah-quinones Oct 6, 2022

Choose a reason for hiding this comment

mbrubeck Oct 16, 2022

Choose a reason for hiding this comment

sarah-quinones Oct 17, 2022

Choose a reason for hiding this comment

corvusrabus commented Nov 28, 2022

mbrubeck commented Jun 17, 2023

mbrubeck commented Jul 23, 2023

sarah-quinones commented Aug 2, 2022 •

edited

Loading