tokio-rs / bytes

Utilities for working with bytes
MIT License
1.86k stars 278 forks source link

Implement `Bytes::from_raw_parts` #684

Closed JackKelly closed 5 months ago

JackKelly commented 5 months ago

Use-case

I need to construct buffers which are aligned to 512-byte boundaries, in order to read data from disk using O_DIRECT on Linux (see issue #600 for a user who has the same use-case as me).

Once the operating system has written into these buffers, I'd love to wrap the buffer in Bytes so I can:

Wrapping an aligned buffer is possible today using Bytes as it current stands (although I'm not entirely certain if this will be safely drop'd in all situations, because Bytes::drop doesn't know the alignment??):

fn get_aligned_bytes(len: usize, align: usize) -> bytes::Bytes {
    assert_ne!(len, 0);
    let layout = Layout::from_size_align(len, align)
        .expect("failed to create Layout!")
        .pad_to_align();
    let boxed_slice = unsafe {
        let ptr = alloc(layout);
        if ptr.is_null() {
            handle_alloc_error(layout);
        };
        let s = slice::from_raw_parts_mut(ptr, len);
        Box::from_raw(s as *mut [u8])
    };
    bytes::Bytes::from(boxed_slice)
}

However, that requires two unnecessary steps: slice::from_raw_parts_mut and Box::from_raw.

This PR adds a very simple Bytes::from_raw_parts method (which is mostly copied from impl From<Box[u8]> for Bytes).

With this new PR, wrapping an aligned buffer in Bytes is a little more ergonomic for users, and a little more computationally efficient:

fn get_aligned_bytes(len: usize, align: usize) -> bytes::Bytes {
    assert_ne!(len, 0);
    let layout = Layout::from_size_align(len, align)
        .expect("failed to create Layout!")
        .pad_to_align();
    let ptr = unsafe { alloc(layout) };
    if ptr.is_null() {
        handle_alloc_error(layout);
    };
    bytes::Bytes::from_raw_parts(ptr, layout.size())
}

(This is my first PR to a Rust open-source project, so please LMK if I've done anything wrong!)

TODO:

Related

JackKelly commented 5 months ago

You may be wondering: Why didn't I implement from_raw_parts for BytesMut? My understanding (which could be wrong) is that BytesMut uses Vec<u8> internally. Vec will re-allocate if users push more elements into the Vec than will fit into the current Vec::capacity. And, AFAIK, there's no easy way to safely re-allocate a Vec whilst maintaining alignment.

Also, crucially, for my use-case Bytes is perfectly sufficient.

Darksonn commented 5 months ago

I'm sorry, but due to the destructor using a different alignment, this is not sound.

Something like this will most likely require the proposed vtable api, but the bytes crate does not have enough maintainer bandwidth to review a new vtable API right now.

JackKelly commented 5 months ago

OK, no worries, thanks for the very quick review!

I'd like to keep track of progress towards a vtable API in Bytes (although I appreciate that it might take a while!). Is this the best issue to subscribe to:

Darksonn commented 5 months ago

Yes, that issue is fine.