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

Remove Registration and SetReadiness APIs #907

Closed
carllerche opened this issue Jan 12, 2019 · 20 comments
Closed

Remove Registration and SetReadiness APIs #907

carllerche opened this issue Jan 12, 2019 · 20 comments
Labels
rfc Request for comments.
Milestone

Comments

@carllerche
Copy link
Member

The Registration and SetReadiness APIs require maintaining a custom scheduler internal to Mio. The ecosystem is moving away from using the Mio scheduler. For example, Tokio provides a number of its own schedulers.

By removing these APIs, Mio on *nix platforms will (again) truely become zero cost on top of the OS APIs.

@carllerche carllerche added the rfc Request for comments. label Jan 12, 2019
@carllerche carllerche added this to the v0.7 milestone Jan 13, 2019
@carllerche
Copy link
Member Author

The windows selector uses Registration and SetReadiness internally. So, mio would have to maintain the code, but it can keep it private and only when Mio is built for windows.

@Thomasdezeeuw
Copy link
Collaborator

@carllerche is the plan to keep the existing Registration queue implementation for Windows, or do want to use an external crate? For example crossbeam has a number of channels and queues. Not having to maintain a complicated multi-threaded queue would make maintenance easier.

@eduardosm
Copy link

As I know of, Registration / SetReadiness is the only way to wake poll from another thread. Will there be an alternative?

@Thomasdezeeuw
Copy link
Collaborator

@eduardosm The current plan (which is a work in progress, for which we're looking for feedback) is to reduce the API service of Mio. Anything that can be implemented outside of Mio, should be implemented outside mio. This includes user space notifications, which is currently implemented as Registration/SetReadiness.

If your goal is just to wake poll from another thread then your implementation of the Awakener in #909 would do the trick, which can be implemented outside the Mio crate.

@carllerche
Copy link
Member Author

@eduardosm @Thomasdezeeuw I think there will need to be some public awakener API. That is the only thing needed to impl Registration / SetReadiness outside of the Mio crate.

@carllerche
Copy link
Member Author

Thinking more about it, technically an awakener could be implemented in userland, but I am not sure if that is a good idea or not.

#788 proposes reserving token space for selector implementation. This would allow OS specific selectors to provide additional features as needed. Reserving a token for the awakener was also part of the intent for this.

I'm leaning towards keeping an awakener concept part of core mio because a) it is pretty much always needed to have a way to wake up an event loop b) the implementation it is pretty platform specific.

@eduardosm
Copy link

I'm leaning towards keeping an awakener concept part of core mio because a) it is pretty much always needed to have a way to wake up an event loop b) the implementation it is pretty platform specific.

My main concern with removing Registration / SetReadiness is lossing the possiblility of waking the event loop without platform specific code. So, I think it is a good idea to provide a public awakener, so users can implement their APIs on top of it without writing platform specific code.

@Thomasdezeeuw
Copy link
Collaborator

Thomasdezeeuw commented Jan 19, 2019

I've been thinking about this a bit lately and I came to the conclusion that an Awakener type is needed and should be in the Mio crate. @eduardosm is right that on Linux eventfd is the best solution, on platforms with kqueue we can use EVFILT_USER (user level notifications) and on other unix platforms we can use a pipe. I don't know about Windows though. So I propose the following API (not new here):

pub struct Awakener {
    inner: sys::Awakener,
}

impl Awakener {
    /// Create a new `Awakener`.
    pub fn new(poll: &Poll, token: Token) -> io::Result<Awakener> {
        Ok(Awakener { inner: sys::Awakener::new(poll, token)? })
    }

    /// Attempts to clone the `Awakener`.
    ///
    /// The `Awakener` must first be registered with a `Poller` instance before
    /// it can be cloned.
    pub fn try_clone(&self) -> io::Result<Awakener> {
        self.inner.try_clone()
    }

    /// Wake up the `Poller` associated (registered) with this `Awakener`.
    pub fn wake(&self) -> io::Result<()> {
        self.inner.wake()
    }

    /// Drain the `Awakener` of all notifications.
    pub fn drain(&self) -> io::Result<()> {
        // Only really needed on Linux to not overflow 64 bits.
        self.inner.drain()
    }
}

impl Evented for Awakener {
    // ...
}
// sys module.
pub struct sys::Awakener {
    // On linux we use eventfd.
    #[cfg(target_os = "linux")]
    efd: RawFd,
    // Kqueue platforms need a copy of the kqueue file descriptor.
    #[cfg(any(target_os = "freebsd", target_os = "macos",
          target_os = "netbsd", target_os = "openbsd"))]
    kq: RawFd,
    // Other unix platforms use a pipe.
    #[cfg(target_os = "NOT_ABOVE")]
    pfd: RawFd,
}

I'll do some experiments with kqueue, @eduardosm already implemented Linux's eventfd. Who can do Windows?

@Thomasdezeeuw
Copy link
Collaborator

I've implemented this for mio-st in pr Thomasdezeeuw/gaea#45. The API changed slightly;

  • new requires a reference to Poll and a Token. Kqueue needs to clone the file descriptor and then adds an event with the provided Token.
  • wake and drain now return an io::Result, just passed up from the underlying read/kevent system calls.

I've updated my previous comment.

@Thomasdezeeuw
Copy link
Collaborator

Ok so the pr is failing sometimes and only on Linux. @eduardosm Do you know why it could miss an event from the eventfd using PollOption::Level?

@carllerche
Copy link
Member Author

One question: Do we still want to support linux kernel 2.6.18? Last I checked (years ago) it was still in heavy use. eventfd is not available on this kernel.

@carllerche
Copy link
Member Author

I think the answer is no, we can probably just copy this as a goal for supported platforms.

FXTi added a commit to FXTi/mio that referenced this issue Apr 9, 2019
First, make Registration/SetReadiness private.
@Thomasdezeeuw
Copy link
Collaborator

#973 removes Registration and SetReadiness, #969 and #976 introduces a Waker to allow for cross-thread wakeups. So this is done.

@elinorbgr
Copy link
Contributor

Just a question, to make sure I correctly understood this change. This does effectively remove the capability to craft purely userspace event sources that would be recognized by mio, right?

So, the path forward to keep using them (like the ones provided by mio-extras) is to have them use a Waker, and then manually check the readiness of the userspace sources when the poll method returns.

Is that correct?

@Thomasdezeeuw
Copy link
Collaborator

@vberger yes, that is correct. User space events are consider out-of-scope for Mio 0.7.

@Thomasdezeeuw
Copy link
Collaborator

@vberger your suggested solution how more or less how I handle user space as well.

@nox
Copy link
Contributor

nox commented Mar 12, 2020

@Thomasdezeeuw @vberger Did any of you made a crate that replaces Registration/SetReadiness? I was looking into bumping mio in Servo, but we rely on mio-extras through ws and it uses those things.

@Thomasdezeeuw
Copy link
Collaborator

@nox I don't know how Servo used Registration/SetReadiness, but personally I used crossbeam's channel instead. The most straight forward replacement would creating your own readiness, e.g. struct ServoReadiness { mio::Token, Ready } (also using a customReady type), and poll crossbeam_channel::Receiver<ServoReadiness> when you would poll mio::Poll and process events from both sources normally. SetReadiness can then be replaced with the sending side of the channel. That is more or less the same thing Mio used to do internally.

@nox
Copy link
Contributor

nox commented Mar 12, 2020

Sorry I meant that mio-extras (which is used by ws) uses this pair of types, not Servo directly, which is why I don't particularly know how it's used either. I'll look into what you suggested some time.

@elinorbgr
Copy link
Contributor

@nox No, I instead stopped depending on mio-extras and implemented internally what I needed for managing purely userspace event sources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Request for comments.
Projects
None yet
Development

No branches or pull requests

5 participants