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

Use a slice for the event list of kevent() #1044

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yorickpeterse
Copy link

This removes the need for dynamically allocating a Vec when the kevent() function is called. The documentation is also rephrased slightly as slices don't really have a "capacity", but only a fixed-size length.

This removes the need for dynamically allocating a Vec when the kevent()
function is called. The documentation is also rephrased slightly as
slices don't really have a "capacity", but only a fixed-size length.

This fixes bytecodealliance#1043.
@yorickpeterse
Copy link
Author

Ugh, I'm now reminded of why the Vec setup is used: an empty Vec turns (of course) into an empty slice, meaning no events are produced. To work around that you'd have to use the unsafe Vec::set_len as is currently done in main. Since Event doesn't implement Default (not sure it can either), you'd have to manually create a slice with the right size and the appropriate default values.

One option is to require eventlist to be &mut [MaybeUninit<Event>], because then at least you can do something like let mut events: [Event; 2] = unsafe { MaybeUninit::uninit().assume_init() };. This however isn't exactly pretty either.

@yorickpeterse
Copy link
Author

Using const generics you can do this:

pub unsafe fn kevent<'a, const N: usize>(
    kqueue: impl AsFd,
    changes: &[Event],
    timeout: Option<Duration>,
) -> io::Result<&'a [Event]> {
    let timeout = timeout.map(|timeout| backend::c::timespec {
        tv_sec: timeout.as_secs() as _,
        tv_nsec: timeout.subsec_nanos() as _,
    });

    let mut out_slice: [MaybeUninit<Event>; N] = unsafe { MaybeUninit::uninit().assume_init() };

    syscalls::kevent(kqueue.as_fd(), changes, &mut out_slice, timeout.as_ref())
        .map(|res| from_raw_parts(out_slice.as_ptr().cast(), res as usize))
}

You then use it like so:

kevent::<10>(&kq, &subs, None)

In case of the kq.rs example, you then end up using that like so:

out.extend_from_slice(unsafe { kevent::<10>(&kq, &subs, None) }?);

I'm not a fan of having to specify the size using the turbofish syntax though, but I do like the idea of kevent() maintaining control over the raw slice. If only const generics allowed something like kevent(&kg, &subs, 10, None) i.e. something like const size: N :/

@yorickpeterse
Copy link
Author

Another option is this:

We implement Default for Event like so:

impl Default for Event {
    fn default() -> Self {
        Event {
            inner: kevent_t {
                ..unsafe { zeroed() }
            },
        }
    }
}

The kevent() function then takes the input list of events in the form &mut [Event] and returns a &[Event] that has the correct length. You then use it like so:

let mut events = [Event; 10] = [Event::default(); 10];

for event in kevent(..., ..., &mut events, None) {

}

This removes the need for const generics and gives users control over the slice. The big downside is that your slice of 10 values (in this case) may only contain 3 values that are in fact initialized, so the use of this setup would very much remain unsafe.

@yorickpeterse
Copy link
Author

I pushed the const generics example so it's easier to discuss, but I'd very much like some feedback on what the best approach would be here.

@notgull
Copy link
Contributor

notgull commented Apr 5, 2024

Why not use MaybeUninit for the slice? I think it would solve this problem?

@yorickpeterse
Copy link
Author

One potential option that comes to mind is this: instead of taking raw slices for the event list, we introduce a Events type, similar to the EventVec type used in the epoll module. This type is a const generic type that stores a slice of a particular length, i.e. something like this:

use std::ptr::null_mut;

// For the sake of this example I'm just using a raw pointer here.
type Event = *mut ();

struct Events<const N: usize> {
    events: [Event; N],
}

impl<const N: usize> Events<N> {
    fn new() -> Events<N> {
        Events { events: [null_mut(); N] }
    }

    fn as_slice(&self) -> &[Event] {
        &self.events
    }
}

fn main() {
    let ev: Events<10> = Events::new();

    println!("length: {}", ev.as_slice().len());
}

You still need to specify the explicit length in the Events type, but that's similar to regular slices, and doesn't require the turbofish syntax for the call to kevent.

In this particular example I've added an as_slice method, but I think the Events type should expose very little information, and instead the kevent() function returns a slice that lives as long as the Events type, i.e. it takes events: &'a mut Events and returns &'a [Event]. This means we can completely abstract away the uninitialized data.

The downside is that Events ends up being basically an opaque type for the sole purpose of not being able to access the underlying data, which might seem a bit odd.

@yorickpeterse
Copy link
Author

yorickpeterse commented Apr 5, 2024

@notgull Taking a &mut [MaybeUninit<Event>] as input certainly works, but it would be nice if kevent() could be made more safe, at least to the extent that's possible. Using a MaybeUninit as input basically just moves that responsibility to the caller (which isn't necessarily wrong).

@yorickpeterse
Copy link
Author

yorickpeterse commented Apr 5, 2024

To illustrate things:

  • bc88181 uses const generics for just kevent(), requiring you to use it along the lines of kevent::<10>(...)
  • 022a8f4 changes this to use the mentioned Events type. The name is a bit confusing because you're not supposed to use it for input events, but I couldn't think of anything better that isn't terribly verbose (e.g. OutputEvents). This setup still needs const generics for kevent(), but the compiler can infer this through the use of the Events type. The kq.rs example is updated accordingly

In terms of usage, I quite like the second approach as it no longer requires the caller to use unsafe code (e.g. MaybeUninit), but I'm curious to see what the thoughts are of the maintainers 😃

},
}
}
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Event implements Clone I think this is technically not required, as we can just do the zero'ing in the Events type. This is in fact probably better such that users can't create a zero'd (and thus basically invalid) Event type themselves.

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.

2 participants