BytesMut fails to reuse buffer in trivial case where all data has been consumed

[bdonlan]

The following test fails with bytes 0.5.5:

    #[test]
    fn bytes_mut_reuse() {
        use bytes::{BytesMut, Buf};
        let mut buf = BytesMut::new();
        buf.reserve(8192);
        buf.extend_from_slice(&[0u8;100][..]);
        
        let p = &buf[0] as *const u8;
        buf.advance(100);

        buf.reserve(8192);
        buf.extend_from_slice(b" ");

        assert_eq!(&buf[0] as *const u8, p);
    }

I would expect this test to pass, as we should be able to reuse the entire buffer given that there is only one reference, and that since we've consumed the entire buffer there should be no data to copy.

The issue appears to come from this condition here:

https://github.com/tokio-rs/bytes/blob/master/src/bytes_mut.rs#L562-L564

Here, additional refers to the value passed to reserve - this is the amount of data above the current length, but here we require that the current offset be beyond that, for some reason - perhaps this is meant to be checking against additional - capacity? Additionally, this does not take into account the amount of data that we need to copy - if self.len() == 0 then the copy is free and we should always take this option.

[seanmonstar]

Yea, this does seem wrong. In this test, off would be 100, and additional is 8192. We shouldn't be checking 100 > 8192, but rather checking that cap - (len - off) > additional, I think. Does that sound right?

[bdonlan]

For the first half of the condition, we want to be checking if there's enough space to satisfy the allocation: self.cap + off >= additional.

For the second half, we shouldn't actually do the cap/2 test. The reason is that, if we fail this test we're going to end up copying the vector anyway, so the copy is a sunk cost either way. We might as well see if we need to reallocate or if we can do it in place (though an overlapping copy might be slightly more expensive than a nonoverlapping copy, in theory).

[seanmonstar]

I don't know why the second half of that branch exists at all (Maybe @carllerche remembers?).

We can't use just cap + off, cap is the entire capacity of the allocated buffer, so cap + off would overflow. We specifically want to know if additional bytes would fit if we shifted over off..len to the beginning.

[bdonlan]

I don't believe cap counts the bytes prior to the beginning of the usable part of the buffer. If it did we wouldn't be in reserve_inner in the first place: https://github.com/tokio-rs/bytes/blob/master/src/bytes_mut.rs#L538-L542

[seanmonstar]

I just wrote up a comment describing ptr and cap and so on, then double checked, and woah, I had completely forgotten, cap is modified when calling advance. I can't remember why I decided to do that... Maybe it's related to needing an offset but didn't have another usize...

[bdonlan]

Requiring cap - off to be computed in capacity() would impact the more common path of checking whether capacity is immediately available, I'd think, so these semantics for cap make sense to me.