Improve example solution of circular-buffer#1724
Merged
Conversation
5df3917 to
40dc162
Compare
This was referenced Sep 11, 2023
Contributor
Author
This was referenced Sep 11, 2023
a4d3b6b to
883e45d
Compare
Merged
40dc162 to
c406b9c
Compare
883e45d to
c4a2c76
Compare
c406b9c to
df1742b
Compare
c4a2c76 to
321824b
Compare
df1742b to
d1e235c
Compare
321824b to
08ee069
Compare
d1e235c to
9e4c8c5
Compare
08ee069 to
655c176
Compare
9e4c8c5 to
746cfd5
Compare
655c176 to
4696194
Compare
746cfd5 to
b388172
Compare
4696194 to
8f44f00
Compare
The trait bounds `Default + Clone` are semantically incorrect. A proper circular buffer needs to handle all kinds of elements. The reason these bounds are here is because it allows to avoid `unsafe` while enjoying the most efficient memory layout. However, there is another performance downside: The implementations of `Default` and `Clone` may be expensive (e.g. cause heap allocations.) As came up in this discussion, there are other weird side effects, for example for `Rc` which has a non-standard implementation of `Clone`: #1652 The approach I chose here get's rid of the trait bounds and wraps every element in an `Option`. In the worst case, this may lead to twice the memory consumption. (e.g. `Option<u64>` takes 16 bytes because of alignment) This approach is semantically correct, but not the most performant. The most performant solution would be to use `unsafe`. I'd like to avoid that in example solutions.
b388172 to
d3ed68a
Compare
ErikSchierboom
approved these changes
Sep 15, 2023
This was referenced Sep 16, 2023
This file contains hidden or 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
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 trait bounds
Default + Cloneare semantically incorrect. A proper circular buffer needs to handle all kinds of elements.The reason these bounds are here is because it allows to avoid
unsafewhile enjoying the most efficient memory layout.However, there is another performance downside:
The implementations of
DefaultandClonemay be expensive (e.g. cause heap allocations.)As came up in this discussion, there are other weird side effects, for example for
Rcwhich has a non-standard implementation ofClone: #1652The approach I chose here get's rid of the trait bounds and wraps every element in an
Option.In the worst case, this may lead to twice the memory consumption. (e.g.
Option<u64>takes 16 bytes because of alignment and padding) This approach is semantically correct, but not the most performant.The most performant solution would be to use
unsafe. I'd like to avoid that in example solutions.