Add `Bytes::try_unsplit()`

[abonander]

I see APIs have been proposed like this before but all I want is a method to try to unsplit a Bytes as I think that should be possible in many cases, and if it fails the user can choose to perform a copy:

 impl Bytes {
    /// If `self` and `other` are adjacent in the underlying buffer, widen `self` to encompass both slices.
    pub fn try_unsplit(&mut self, other: Bytes) -> Result<(),  Bytes> { ... }
}

if let Err(other) = bytes.try_unsplit(other) {
    bytes.extend_from_slice(&other);
}

I'd like it to take self by mutable reference as that's generally more versatile and matches the signature of BytesMut::unsplit().

[DoumanAsh]

@abonander Shouldn't you be able to do similar thing by converting to Mut if it is possible and unsplitting?

Bytes should be kinda immutable.

[abonander]

@DoumanAsh the idea is to avoid performing a copy which that path necessitates. You're not mutating the underlying bytes, just widening the slice which should be safe as long as immutable references remain to the whole buffer.

[CodesInChaos]

@abonander

Taking the second bytes by reference would remove the need to return it in the error case.

@DoumanAsh

Shouldn't you be able to do similar thing by converting to Mut if it is possible and unsplitting?

In cases where you could unsplit (both Bytes being adjacent in a shared buffer) converting to BytesMut will always fail since the reference count is at least two.

[xTachyon]

I feel like this should be easy to implement and would be pretty useful.

My proposed function prototype is:

fn try_unsplit(&self, other: &Bytes) -> Option<Bytes>;

And it'd check if the shared data is the same and if they are adjacent, and return a Bytes which begins at self and has the len = self.len() + other.len().

I could get a shot at trying to implement it if you want.

[Matthias247]

I would have had a use for the API too. I think the API proposed in the original comment makes most sense - if the unsplit worked the original other buffer should no longer be accessible.

Regarding the implementation: With the new vtable based approach you would likely check whether both internal pointers are equivalent and the data regions are adjacent. Then adjust the data region in the first object and call the vtable drop function on the second buffer.

I'm unsure whether that would have any negative side effects on some buffer implementations, where the same data pointer does not actually indicate the same buffer but is more a handle to something else. If that would be the case then we would need vtable methods for split and try_unsplit.

[CodesInChaos]

Taking &mut self while returning a status indicator instead of the actual result saves one ref-count increment/decrement.

[Matthias247]

Whatever API is chosen, there will be 1 refcount decrement if successful and 0 if not successful. A move of a value doesn't change the refcount - only a clone or drop would. The merge here is the equivalent of a drop of the unused half.

The thing which we merge into should be taken by &mut self since it will always stay intact but change its properties. The thing which gets merged should be taken by value, since it might no longer exist if the merge is successful. If the merge isn't successful that half gets returned in order to give it back to the user. This is exactly the API that the original post proposed.

[carllerche]

@seanmonstar I think this would require an addition to the vtable?

[seanmonstar]

Perhaps it would.

I'm glad we didn't make the vtable stuff public just yet, I really don't like that it doesn't have a way to add new optional methods like a trait does. I'm thinking of trying to adjust the design a little to not be a struct Vtable but some unsafe trait Share.

[Matthias247]

@seanmonstar I think this would require an addition to the vtable?

I am not sure if it really does. data pointers being equivalent and byte ranges being adjacent or overlapping is a good enough check for all implementations of Bytes that I can think of at the moment. It could be documented that Bytes instances which fulfill that criteria must be joinable - in the same way as currently every Bytes object is spliitable - by calling clone instead of a more specific split function.

Although I guess a vtable function would be cleaner, since it provides a way out if we discover in the future that there are some Bytes implementations which really behave differently. Issues like these tell us that it might be a good idea to keep as much as possible flexible.

I think even with the vtable approach we can add optional methods: In order to do that, do not grant applications access to the vtable directly, but for force them to go throug a builder:

let vtable = VTableBuilder::new().set_clone(my_clone_fn).set_drop(my_drop_fn).build()`

That way you can default some functions which are not explicitly set in the same way as with traits.
One tricky thing is obviously that the the builder and generated vtable must run at compile time, because we need 'static. But I think builders which just set fields might already work as const fn.

And of course even with a some extension points there are only so much backward compatible changes we can make. E.g. adding a default implementation for try_unsplit and allowing users to overwrite it is strictly speaking not backward compatible, since it might crash for users where the default impl does not do the right thing.

[seanmonstar]

I am not sure if it really does. data pointers being equivalent and byte ranges being adjacent or overlapping is a good enough check for all implementations of Bytes that I can think of at the moment.

I'm not sure this is a safe assumption. If you have two separate allocations that happen to be right next to each other in the address space, and tried to un-split them, you could erroneously think they point to a single owned allocation. I tried this out with Vec and Box, and it seemed the were was always between 16 and 32 addresses between, but that may be an allocator detail that shouldn't be relied on.

VTableBuilder

Oh yea, that could work too... I just don't actually see the benefit of the raw vtable approach over some *mut dyn Share.

adding a default implementation for try_unsplit and allowing users to overwrite it is strictly speaking not backward compatible, since it might crash for users where the default impl does not do the right thing.

I'd expect a default implementation would just be pessimistic and assume unsplitting failed, so that would be backwards compatible.

[Matthias247]

I'm not sure this is a safe assumption. If you have two separate allocations that happen to be right next to each other in the address space, and tried to un-split them, you could erroneously think they point to a single owned allocation.

I think the contract is that two separate allocations also have two separate data pointers. Otherwise the underlying allocator has no chance to identify the allocation. An allocator can not use ptr to identify something, because it will not stay stable during slicing.

I think the only exception are "allocators" which use data not in order to store a pointer. E.g. the from_static vtable obviously doesn't need data and can set it to anything - in which case two adjacent ranges could indeed be merged. For things like these the vtable approach is imho a bit more sensible than trait objects, because the pointer doesn't really refer to an object. It can refer to nothing, or can be used as an index into a global array. Trait objects refer always to object instances, and making them work on "not so real instances" is a bit hacky as we had seen in the older trait object based Waker implementations.

I think if the documentation specifies that each unique byte range must have a unique data pointer it should be good as it is, and we can unsplit based on a unique pointer and adjacency.

[dcreager]

Similar in spirit to this would be a Bytes::try_slice_ref method ­— instead of panicking if the sub slice isn't actually part of the buffer, it would return None. (Or maybe it should always return a Bytes, falling back on creating a copy of the sliced data?) The serde Deserialize implementation could then use that, which should allow the following to work in a zero-copy way when you happen to be deserializing from a Bytes instance:

#[derive(Deserialize)]
struct Data {
    wrapped: Bytes,
}