-
Notifications
You must be signed in to change notification settings - Fork 287
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
Bytes: Add try_into_mut. #687
Conversation
Add try_into_mut that allows getting a BytesMut from an unique Bytes.
/// ``` | ||
pub fn try_into_mut(self) -> std::result::Result<BytesMut, Bytes> { | ||
if self.is_unique() { | ||
Ok(BytesMut::from_vec(self.into())) |
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.
Although this does avoid allocating a new vector, it can still result in a copy of the data when it's not stored at the beginning of the allocation. For example, this happens here:
Lines 996 to 997 in 4eb62b9
// Copy back buffer | |
ptr::copy(ptr, buf, len); |
and here:
Lines 1127 to 1128 in 4eb62b9
// Copy back buffer | |
ptr::copy(ptr, buf, len); |
Actually avoiding the copy will be a more involved change.
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.
This is probably unavoidable right? Or are the slices of bytes aware of their parents?
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.
The BytesMut
type is also able to reference only part of the allocation. So, it's not unavoidable. (But it will be rather involved.)
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.
Ha I read the code and I understand, so essentially if the user did a slice, dropped the other references and then calls try_into_mut
then it will shift left the data so it can fit into a Vec. But we don't really need this shift since the BytesMut
could take the same pointer with the offset and length.
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.
Yeah, exactly.
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.
Re-reading this, ptr::copy
offers no guarantee that it won't copy even if ptr == buf
(at least I could not find it). So we should treat it as if it was always copying (or add an if clause).
/// let bytes = Bytes::from(b"hello".to_vec()); | ||
/// assert_eq!(bytes.try_into_mut(), Ok(BytesMut::from(&b"hello"[..]))); | ||
/// ``` | ||
pub fn try_into_mut(self) -> std::result::Result<BytesMut, Bytes> { |
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.
This use of std
fails in builds without the standard library.
pub fn try_into_mut(self) -> std::result::Result<BytesMut, Bytes> { | |
pub fn try_into_mut(self) -> Result<BytesMut, Bytes> { |
@Darksonn we can close this |
Add try_into_mut that allows getting a BytesMut from an unique Bytes.
Implements #611