Implement remove with swap instead of rotate_left #128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
Lokathor
approved these changes
Jan 14, 2021
|
This would also benefit from a comment on why it's written in this particular way. Doing it this way is important, but someone might later try to refactor it for readability and accidentally degrade performance. Something like: // This generates far fewer instructions than calling `targets.rotate_left(1);`
// and performs much better for realistic ArrayVec lengths |
|
@Shnatsel Thanks for the call-out. I'll have another PR up in a few hours that addresses this. |
saethlin
added a commit
to saethlin/tinyvec
that referenced
this pull request
Jan 16, 2021
add comparison script Outline the drain to heap logic in TinyVec::push Outline the drain to heap logic in TinyVec::push Use #[cold] and explain the outlining in a comment change gen_remove to rebuild sample Vec with clone instead of push rename outer vec in gen_remove for clarity Swap each element instead of calling rotate_left add more loops to amortize away startup cost, tune tinyvec impls a bit swap instead of rotate_right, explain in comment drop loops again wheee Outline the drain to heap logic in TinyVec::push (Lokathor#127) * Outline the drain to heap logic in TinyVec::push * Use #[cold] and explain the outlining in a comment Swap each element instead of calling rotate_left (Lokathor#128)
saethlin
added a commit
to saethlin/tinyvec
that referenced
this pull request
Jan 19, 2021
add comparison script Outline the drain to heap logic in TinyVec::push Outline the drain to heap logic in TinyVec::push Use #[cold] and explain the outlining in a comment change gen_remove to rebuild sample Vec with clone instead of push rename outer vec in gen_remove for clarity Swap each element instead of calling rotate_left add more loops to amortize away startup cost, tune tinyvec impls a bit swap instead of rotate_right, explain in comment drop loops again wheee Outline the drain to heap logic in TinyVec::push (Lokathor#127) * Outline the drain to heap logic in TinyVec::push * Use #[cold] and explain the outlining in a comment Swap each element instead of calling rotate_left (Lokathor#128)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The codegen from
slice::rotate_leftis pretty ghastly; in my compiled benchmarks it's some 272 instructions with a number calls tomemcpyandmemmove, but only a few instructions are actually hot when it's run, and of course it isn't inlined (because it's huge?).I think this implementation brings
TinyVec::removeto parity with or possibly faster thanSmallVec::remove.