bytes icon indicating copy to clipboard operation
bytes copied to clipboard

Make `try_unsplit` method public

Open stevenengler opened this issue 4 years ago • 2 comments

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

stevenengler avatar Sep 29 '21 21:09 stevenengler

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

Darksonn avatar Sep 29 '21 21:09 Darksonn

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.

stevenengler avatar Sep 29 '21 22:09 stevenengler

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

xonatius avatar Dec 10 '23 06:12 xonatius

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.

Darksonn avatar Dec 10 '23 09:12 Darksonn

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.

stevenengler avatar Jan 01 '24 22:01 stevenengler