Skip to content

Add zero-copy make_mut#695

Merged
Darksonn merged 10 commits intotokio-rs:masterfrom
caido:feat-make-mut
May 5, 2024
Merged

Add zero-copy make_mut#695
Darksonn merged 10 commits intotokio-rs:masterfrom
caido:feat-make-mut

Conversation

@Sytten
Copy link
Copy Markdown
Contributor

@Sytten Sytten commented Apr 14, 2024

Supersede #687
Closes #611

This is my first time contributing to bytes and this is quite a change, do let me know if you want more tests.

@Sytten Sytten mentioned this pull request Apr 14, 2024
Comment thread src/bytes_mut.rs Outdated
Comment thread src/bytes_mut.rs Outdated
Comment thread src/bytes.rs
@mattfbacon
Copy link
Copy Markdown

Why is x.make_mut() not the same as x.try_into_mut().unwrap_or_else(|| BytesMut::from(&*x))?

@Sytten
Copy link
Copy Markdown
Contributor Author

Sytten commented Apr 15, 2024

I mean I guess we could but we already have to handle the copy case in each to_mut vtable function so I dont really see the point in not using that. Plus it there is a small chance that a concurrent thread released the shared since we use acquire vs relaxed ordering.

Comment thread src/bytes.rs
@Darksonn Darksonn requested a review from braddunbar April 17, 2024 13:43
@Sytten
Copy link
Copy Markdown
Contributor Author

Sytten commented Apr 23, 2024

Sorry I didnt have to work on it more, I will try this week. @braddunbar if you have for a review I would appreciate 🙏

@braddunbar
Copy link
Copy Markdown
Contributor

Oh, I missed the notification. Thanks for the ping! I'll take a look.

@Sytten
Copy link
Copy Markdown
Contributor Author

Sytten commented Apr 30, 2024

@braddunbar @Darksonn Gentle ping :)
I added the comment and an odd test. Let me know if we want to add more tests.

Comment thread src/bytes_mut.rs Outdated
Comment thread src/bytes.rs Outdated
Comment thread src/bytes.rs Outdated
Comment thread src/bytes.rs Outdated
Comment thread src/bytes.rs
Comment thread tests/test_bytes.rs
@Sytten
Copy link
Copy Markdown
Contributor Author

Sytten commented May 1, 2024

@Darksonn Done the improvements

Comment thread src/bytes.rs Outdated
Comment thread src/bytes_mut.rs Outdated
Comment thread src/bytes_mut.rs Outdated
@Sytten Sytten requested a review from Darksonn May 4, 2024 21:21
@Sytten
Copy link
Copy Markdown
Contributor Author

Sytten commented May 4, 2024

@braddunbar @Darksonn Ready for another round :)

Copy link
Copy Markdown
Member

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Mostly LGTM.

Comment thread src/bytes_mut.rs Outdated
Comment thread src/bytes.rs Outdated
@Sytten Sytten requested a review from Darksonn May 5, 2024 14:38
Copy link
Copy Markdown
Contributor

@braddunbar braddunbar left a comment

Choose a reason for hiding this comment

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

I spent some time combing over this today and I can't find anything to complain about. Thank you @Sytten!

@Darksonn Darksonn merged commit 86694b0 into tokio-rs:master May 5, 2024
@Sytten Sytten deleted the feat-make-mut branch May 5, 2024 18:40
@braddunbar braddunbar mentioned this pull request May 6, 2024
@mox692 mox692 mentioned this pull request Jul 30, 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.

Bytes::make_mut

5 participants