-
Notifications
You must be signed in to change notification settings - Fork 278
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
Refactor Bytes to use an internal vtable #298
Conversation
Note, I didn't actually make the vtable public yet... |
c8b5531
to
eb6eeb6
Compare
The benchmarks (below) are promising! They show that The main regression is when the data would previously have fit in the "inline" variant. The assumption is that the likelihood of data fitting inline is sufficiently low compared to the amount of times we do the other operations. Before
After
|
eb6eeb6
to
230cf98
Compare
Also, the added flexibility from the dynamism will allow handling those cases using other strategies. |
230cf98
to
ae6ac55
Compare
|
||
impl PartialEq<Bytes> for Vec<u8> { | ||
f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this can be compare_exchange
with Acquire
ordering on failure.
1 << (repr + (MIN_ORIGINAL_CAPACITY_WIDTH - 1)) | ||
} | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete or uncomment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! The only feedback I'd like to see in before merging is switching the drop
vtable fn to take &mut AtomicPtr<()>
. This will allow skipping the atomic op in drop fns.
c114ba6
to
78534db
Compare
Ah woops, loom's |
Loom release coming: tokio-rs/loom#81 |
5089be1
to
493b37a
Compare
Bytes is a useful tool for managing multiple slices into the same region of memory, and the other things it used to have been removed to reduce complexity. The exact strategy for managing the multiple references is no longer hard-coded, but instead backing by a customizable vtable. - Removed ability to mutate the underlying memory from the `Bytes` type. - Removed the "inline" (SBO) mechanism in `Bytes`. The reduces a large amount of complexity, and improves performance when accessing the slice of bytes, since a branch is no longer needed to check if the data is inline. - Removed `Bytes` knowledge of `BytesMut` (`BytesMut` may grow that knowledge back at a future point.)
493b37a
to
224c9ab
Compare
pub fn truncate(&mut self, len: usize) { | ||
self.inner.truncate(len); | ||
if len >= self.len { | ||
self.len = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct. The documentation says truncate
should have no effect in this case.
- Removes the `bytes-preview` feature from `zeroize` - Upgrades `secrecy` to use `bytes` v0.5 Now that `bytes` v0.5 is out, I've opened a PR to upstream the `Zeroize` impl for `BytesMut`: tokio-rs/bytes#335 Unfortunately it's no-longer possible to impl `Zeroize` for `Bytes` as of `bytes` v0.5, as the `try_mut` method was dropped in this PR: tokio-rs/bytes#298 I have brought this up on the first PR. In the meantime, this vendors the previous `BytesMut` impl of `Zeroize` into `secrecy`'s `SecretBytesMut` type, and drops `SecretBytes` as it's no-longer possible to implement.
- Removes the `bytes-preview` feature from `zeroize` - Upgrades `secrecy` to use `bytes` v0.5 Now that `bytes` v0.5 is out, I've opened a PR to upstream the `Zeroize` impl for `BytesMut`: tokio-rs/bytes#335 Unfortunately it's no-longer possible to impl `Zeroize` for `Bytes` as of `bytes` v0.5, as the `try_mut` method was dropped in this PR: tokio-rs/bytes#298 I have brought this up on the first PR. In the meantime, this vendors the previous `BytesMut` impl of `Zeroize` into `secrecy`'s `SecretBytesMut` type, and drops `SecretBytes` as it's no-longer possible to implement.
These seem to have been commented by accident in tokio-rs#298, and are still passing.
These seem to have been commented by accident in tokio-rs#298, and are still passing.
These seem to have been commented by accident in #298, and are still passing.
- Removes the `bytes-preview` feature from `zeroize` - Upgrades `secrecy` to use `bytes` v0.5 Now that `bytes` v0.5 is out, I've opened a PR to upstream the `Zeroize` impl for `BytesMut`: tokio-rs/bytes#335 Unfortunately it's no-longer possible to impl `Zeroize` for `Bytes` as of `bytes` v0.5, as the `try_mut` method was dropped in this PR: tokio-rs/bytes#298 I have brought this up on the first PR. In the meantime, this vendors the previous `BytesMut` impl of `Zeroize` into `secrecy`'s `SecretBytesMut` type, and drops `SecretBytes` as it's no-longer possible to implement.
Bytes is a useful tool for managing multiple slices into the same region
of memory, and the other things it used to have been removed to reduce
complexity. The exact strategy for managing the multiple references is
no longer hard-coded, but instead backing by a customizable vtable.
Bytes
type.Bytes
. The reduces a largeamount of complexity, and improves performance when accessing the
slice of bytes, since a branch is no longer needed to check if the
data is inline.
Bytes
knowledge ofBytesMut
(BytesMut
may grow thatknowledge back at a future point.)
Closes #294