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

Make try_unsplit method public #513

Closed
wants to merge 1 commit into from

Conversation

stevenengler
Copy link

The BytesMut::unsplit method can be expensive if the two ranges aren't contiguous and if it requires a memory allocation for a new buffer. This PR makes the existing BytesMut::try_unsplit public so that the user can choose the behaviour if they aren't contiguous.

Related discussion: #287

Copy link
Contributor

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

Seems good to me.

@Darksonn
Copy link
Contributor

Actually, I underestimated the amount of prior discussion about this, though nothing appears to have happened on the topic in the past year.

@stevenengler
Copy link
Author

stevenengler commented Sep 29, 2021

Actually, I underestimated the amount of prior discussion about this, though nothing appears to have happened on the topic in the past year.

No worries, and I understand if you want to keep that discussion open. I originally made this small change before I read that discussion, and it seemed like the most straightforward way of having this functionality in the library as it's already implemented and has a similar interface to BytesMut::unsplit.

@xonatius
Copy link

@Darksonn Isn't the discussion about unsplit method for Bytes? BytesMut::try_unsplit should be safe to made public:

  1. BytesMut doesn't use VTable like Bytes does - there are only 2 possible underlying storages - Vec and Arc, both of which are handled in try_unsplit
  2. try_unsplit is already used through public unsplit with some additions to copy data if try_unsplit failed.

@Darksonn
Copy link
Contributor

Just because it doesn't do that today doesn't mean it couldn't in the future.

I don't really have the time to work through the vtable situation any time soon.

@stevenengler
Copy link
Author

Closing this since I don't see a reason to keep it open. We're now just using a custom fork of the bytes library with this patch.

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.

3 participants