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

Consider restoring our old network poller code while using IO completion ports on Windows #344

Closed
yorickpeterse opened this issue Sep 22, 2022 · 5 comments
Assignees
Labels
accepting contributions Issues that are suitable to be worked on by anybody, not just maintainers runtime Changes related to the Rust-based runtime library
Milestone

Comments

@yorickpeterse
Copy link
Collaborator

We used to have our own network polling code using epoll/kqueue/etc. While this was simple enough, it was replaced with the polling crate in bf39dbc. Since then maintenance of polling and the various smol-rs/async-std crates has slowed down considerably. In addition, we depend on wepoll-ffi through the polling crate, which requires a C compiler.

Looking at Go's network polling code for Windows (https://github.com/golang/go/blob/master/src/runtime/netpoll_windows.go) I was surprised by how little it seems to need to achieve something similar to epoll, at least on first sight. With that in mind I'm now considering it may not be a bad idea to restore the old network polling code, but use Windows IO completion ports instead of wepoll. This also gives us greater control over the polling logic and API. We should take care that when file descriptors are closed, they're also deregistered automatically. This is already the case for epoll and kqueue, but I'm not sure how this behaves when using IO completion ports.

@yorickpeterse
Copy link
Collaborator Author

With Windows support being dropped, making this change is easier as we don't need to use completion ports of wepoll.

thaodt added a commit to thaodt/inko that referenced this issue Jul 10, 2023
In the interest of reducing 3rd-party dependencies and maintaining
control over our code, I would not only like to restore our old polling
code in favour of the polling crate (inko-lang#344), but also switch to using
rustix. The benefit of using rustix is that at least on Linux we can
avoid libc calls. While this may result in a performance improvement,
it's really more about slowly reducing our dependencies on 3rd-party
libraries, including libc. The API might also be a bit more pleasant
to use compared to using the libc crate directly.

Changelog: changed
yorickpeterse pushed a commit that referenced this issue Jul 10, 2023
In the interest of reducing 3rd-party dependencies and maintaining
control over our code, I would not only like to restore our old polling
code in favour of the polling crate (#344), but also switch to using
rustix. The benefit of using rustix is that at least on Linux we can
avoid libc calls. While this may result in a performance improvement,
it's really more about slowly reducing our dependencies on 3rd-party
libraries, including libc. The API might also be a bit more pleasant
to use compared to using the libc crate directly.

Changelog: changed
yorickpeterse pushed a commit to thaodt/inko that referenced this issue Jul 10, 2023
In the interest of reducing 3rd-party dependencies and maintaining
control over our code, I would not only like to restore our old polling
code in favour of the polling crate (inko-lang#344), but also switch to using
rustix. The benefit of using rustix is that at least on Linux we can
avoid libc calls. While this may result in a performance improvement,
it's really more about slowly reducing our dependencies on 3rd-party
libraries, including libc. The API might also be a bit more pleasant
to use compared to using the libc crate directly.

Changelog: changed
yorickpeterse pushed a commit to thaodt/inko that referenced this issue Jul 10, 2023
In the interest of reducing 3rd-party dependencies and maintaining
control over our code, I would not only like to restore our old polling
code in favour of the polling crate (inko-lang#344), but also switch to using
rustix. The benefit of using rustix is that at least on Linux we can
avoid libc calls. While this may result in a performance improvement,
it's really more about slowly reducing our dependencies on 3rd-party
libraries, including libc. The API might also be a bit more pleasant
to use compared to using the libc crate directly.

Changelog: changed
yorickpeterse pushed a commit that referenced this issue Jul 10, 2023
In the interest of reducing 3rd-party dependencies and maintaining
control over our code, I would not only like to restore our old polling
code in favour of the polling crate (#344), but also switch to using
rustix. The benefit of using rustix is that at least on Linux we can
avoid libc calls. While this may result in a performance improvement,
it's really more about slowly reducing our dependencies on 3rd-party
libraries, including libc. The API might also be a bit more pleasant
to use compared to using the libc crate directly.

Changelog: changed
@yorickpeterse
Copy link
Collaborator Author

An approach I'm toying with is to move the low-level socket bits to the Inko standard library, only exposing some low-level bits to register sockets with the network poller, then using rustix for that. This would also help with #594, as we'd no longer depend on Vec<u8>.

@yorickpeterse
Copy link
Collaborator Author

To add to that: the runtime still needs to keep track of some socket related data (e.g. the "registered" flag), but much of the actual IO work would happen in the standard library.

@yorickpeterse
Copy link
Collaborator Author

Today I tried to update polling to 3.x, but this results in macOS socket test failures. The root cause is that the crate changed its implementation such that you can no longer add an already added file descriptor, and that it's now expected for you to explicitly remove file descriptors before dropping them. Failing to do so results in the entry lingering, meaning that if the descriptor is reused you'll get an error. Some details are found here:

This however introduces several problems:

  1. It introduces additional overhead on kqueue platforms for really no particular reason, as kqueue is perfectly happy having duplicate entries. In our case we already ensure this doesn't happen, so the RwLock overhead is just a total waste of time and possibly serializes the registration (assuming kqueue doesn't already use locking internally)
  2. We now have to call Poller::delete when dropping a socket, increasing the cost of doing so again for no benefit
  3. If many sockets are registered this results in an increase in memory usage for the kqueue backend. Looking at the PR adding the lock, it appears to use 16 extra bytes per socket

I'm going to revert polling back to 2.8.x for the time being, but replacing it entirely is starting to look more appealing.

@yorickpeterse yorickpeterse added this to the 0.14.0 milestone Jan 13, 2024
@yorickpeterse yorickpeterse added the runtime Changes related to the Rust-based runtime library label Jan 13, 2024
@yorickpeterse
Copy link
Collaborator Author

I'm going to open up this issue for contributions. While the work isn't necessarily easy, I think it should be suitable for somebody with decent knowledge of Rust looking to contribute to some projects.

The work to be done is roughly as follows:

  1. Take the old network polling code we had from here
  2. Adopt this into rt/src/network_poller.rs, putting the platform specific code in rt/src/network_poller/PLATFORM.rs (e.g. epoll.rs). The public interface of the NetworkPoller type is to remain as it is today
  3. Ignore the wepoll code since we don't support Windows. If at some point we do, we should use completion ports anyway instead of wepoll, as that spares us having to compile C code when compiling the runtime
  4. Make sure it's up to date according to today's Rust standards/requirements, as the code was written back in 2019/2020. For example, the code uses the "nix" crate which we should replace with the "rustix" crate instead. We can probably also merge some of the modules (e.g. having Interest defined in its own module isn't useful). In other words, we need to get rid of the dust and mothballs rather than just take the code as-is.
  5. Drop the "polling" crate

@yorickpeterse yorickpeterse added the accepting contributions Issues that are suitable to be worked on by anybody, not just maintainers label Jan 13, 2024
@yorickpeterse yorickpeterse modified the milestones: 0.14.0, 0.15.0 Jan 29, 2024
@yorickpeterse yorickpeterse self-assigned this Apr 5, 2024
yorickpeterse added a commit that referenced this issue Apr 5, 2024
This fixes #344.

Changelog: changed
yorickpeterse added a commit that referenced this issue Apr 5, 2024
This fixes #344.

Changelog: changed
yorickpeterse added a commit that referenced this issue Apr 5, 2024
This fixes #344.

Changelog: changed
yorickpeterse added a commit that referenced this issue Apr 5, 2024
This fixes #344.

Changelog: changed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting contributions Issues that are suitable to be worked on by anybody, not just maintainers runtime Changes related to the Rust-based runtime library
Projects
None yet
Development

No branches or pull requests

1 participant