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

Implementation of RFC #240 #284

Closed
wants to merge 7 commits into from
Closed

Conversation

sarah-quinones
Copy link

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
Copy link
Collaborator

mbrubeck commented Aug 1, 2022

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

@sarah-quinones
Copy link
Author

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
Copy link
Author

sarah-quinones commented Aug 2, 2022

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
Copy link
Collaborator

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

@bors-servo
Copy link
Contributor

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

Copy link
Collaborator

@mbrubeck mbrubeck left a comment

Choose a reason for hiding this comment

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

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.

}

#[inline]
pub const fn value(self, is_zst: bool) -> usize {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, that makes sense.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

@corvusrabus
Copy link

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 mbrubeck mentioned this pull request Jun 17, 2023
@mbrubeck
Copy link
Collaborator

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
Copy link
Collaborator

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...

@mbrubeck mbrubeck deleted the branch servo:master September 20, 2023 17:19
@mbrubeck mbrubeck closed this Sep 20, 2023
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