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.

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

Closed
wants to merge 1 commit into from
Closed

Conversation

rklaehn
Copy link

@rklaehn rklaehn commented Apr 9, 2024

Add try_into_mut that allows getting a BytesMut from an unique Bytes.

Implements #611

Add try_into_mut that allows getting a BytesMut from an unique Bytes.
@rklaehn rklaehn mentioned this pull request Apr 9, 2024
/// ```
pub fn try_into_mut(self) -> std::result::Result<BytesMut, Bytes> {
if self.is_unique() {
Ok(BytesMut::from_vec(self.into()))
Copy link
Contributor

@Darksonn Darksonn Apr 10, 2024

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

/// 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> {

@Sytten Sytten mentioned this pull request Apr 14, 2024
@Sytten
Copy link
Contributor

Sytten commented Apr 14, 2024

@Darksonn If you have time, I made an attempt at better implementation here: #695

@Sytten
Copy link
Contributor

Sytten commented May 5, 2024

@Darksonn we can close this

@Darksonn Darksonn closed this May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants