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.

Sign up for GitHub

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

Jump to bottom

Bytes: Add try_into_mut. #687

Closed
wants to merge 1 commit into from
Closed

Bytes: Add try_into_mut. #687

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion src/bytes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use crate::buf::IntoIter;
#[allow(unused)]
use crate::loom::sync::atomic::AtomicMut;
use crate::loom::sync::atomic::{AtomicPtr, AtomicUsize, Ordering};
use crate::Buf;
use crate::{Buf, BytesMut};

/// A cheaply cloneable and sliceable chunk of contiguous memory.
///
Expand Down Expand Up @@ -507,6 +507,28 @@ impl Bytes {
self.truncate(0);
}

/// Try to convert self into `BytesMut`.
///
/// If `self` is unique, this will succeed and return a `BytesMut` with
/// the contents of `self` without copying. If `self` is not unique, this
/// will fail and return self.
///
/// # Examples
///
/// ```
/// use bytes::{Bytes, BytesMut};
///
/// 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> {
Copy link
Contributor

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.

Suggested change
pub fn try_into_mut(self) -> std::result::Result<BytesMut, Bytes> {
pub fn try_into_mut(self) -> Result<BytesMut, Bytes> {

if self.is_unique() {
Ok(BytesMut::from_vec(self.into()))
Copy link
Contributor

@Darksonn Darksonn Apr 10, 2024 •
edited
Loading

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:

bytes/src/bytes.rs

Lines 996 to 997 in 4eb62b9

// Copy back buffer
ptr::copy(ptr, buf, len);

and here:

bytes/src/bytes.rs

Lines 1127 to 1128 in 4eb62b9

// Copy back buffer
ptr::copy(ptr, buf, len);

Actually avoiding the copy will be a more involved change.

Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, exactly.

Copy link
Contributor

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

} else {
Err(self)
}
}

#[inline]
pub(crate) unsafe fn with_vtable(
ptr: *const u8,
Expand Down
Loading