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

Add support for io_uring #998

Open
wants to merge 1 commit into
base: dev/3.0.0
Choose a base branch
from
Open

Add support for io_uring #998

wants to merge 1 commit into from

Conversation

skbeh
Copy link
Contributor

@skbeh skbeh commented Apr 8, 2023

No description provided.

@hugmanrique
Copy link
Contributor

This was already implemented in the https://github.com/PaperMC/Velocity/tree/experiment/io_uring branch, but I guess it didn't get merged due to the experimental status of Netty's implementation.

@electronicboy
Copy link
Member

I think somebody tried io_uring on paper recently and it went sideways, so this should probably be a configurable thing, afaik the thing is still a bit of a moving implementation and has not really been called "stable" yet

@clrxbl
Copy link
Member

clrxbl commented Apr 8, 2023

I think somebody tried io_uring on paper recently and it went sideways, so this should probably be a configurable thing, afaik the thing is still a bit of a moving implementation and has not really been called "stable" yet

zlib packet compression seems to be broken on io_uring on PaperMC having tried this last week. Should double check if that's the case here @skbeh

@skbeh
Copy link
Contributor Author

skbeh commented Apr 9, 2023

@clrxbl I tested both libdeflate and Java compression, and both worked fine.

@skbeh skbeh requested a review from derklaro April 9, 2023 01:36
@skbeh skbeh force-pushed the io-uring branch 2 times, most recently from 6344a51 to 89a5f45 Compare April 22, 2023 06:27
@skbeh skbeh changed the title Add io_uring support Add support for io_uring Apr 22, 2023
@skbeh
Copy link
Contributor Author

skbeh commented Apr 22, 2023

@electronicboy Added.

@clrxbl
Copy link
Member

clrxbl commented Apr 28, 2023

If you're gonna add kqueue, why not add support for it aswell while at it? Seems pretty trivial to add it. Unlikely to be used by most since it's macOS / FreeBSD only, but the dependency is there so why not?

@skbeh
Copy link
Contributor Author

skbeh commented Apr 28, 2023

@clrxbl The kqueue dependency will be removed once the new version of async-http-client is released.

@skbeh skbeh force-pushed the io-uring branch 2 times, most recently from d8f49f5 to 2016c1e Compare May 2, 2023 16:22
@clrxbl
Copy link
Member

clrxbl commented May 2, 2023

Updating async-http-client appears to quietly break a lot of my proxy plugins, one public one being ViaVersion. Which makes me wonder whether this is a change that should be made to a Velocity 3.2.x release?

@electronicboy
Copy link
Member

  1. definitely not comfortable bumping to an early snapshot release of a library mid release cycle
  2. We shouldn't be defaulting the proxy to use an early version of a kernel mechanism that is still not considered stable, especially when many hosts are still using older kernels in which probably have security issues still exposed with that mechanism

@skbeh
Copy link
Contributor Author

skbeh commented May 3, 2023

@electronicboy It seems that there's no sane way to configure AHC to use specific transportFactory.
Also, we can't get the server instance in an enum class, so it is inconvenient to check configure file (velocity.toml).

@electronicboy
Copy link
Member

Not sure why either of those matter?

@skbeh
Copy link
Contributor Author

skbeh commented May 3, 2023

@electronicboy
I mean that I don't know how to make it possible to configure whether to enable it via configure file.
AHC automatically checks transportFactory which will be used together with event loop group, but when the transportFactory is not in their predefined list, it will fail.

@electronicboy
Copy link
Member

We're generally not going to update to a beta release of library mid cycle, so what AHC uses in its beta release is pretty much 100% irrelevant

@skbeh
Copy link
Contributor Author

skbeh commented May 3, 2023

@skbeh
Copy link
Contributor Author

skbeh commented May 3, 2023

In their 2.x versions, they did not aware of io_uring.

@electronicboy
Copy link
Member

as it stands, this PR is pretty much blocked;

We can't justify jumping a library towards a beta release midcycle, so that's generally a no-go; We could stick to 2.x and just not use a shared worker group if using io_uring, but, not sure

io_uring should also be opt in, not opt out

@skbeh skbeh force-pushed the io-uring branch 2 times, most recently from c7eef1c to d864a00 Compare June 22, 2023 11:09
@skbeh
Copy link
Contributor Author

skbeh commented Jun 22, 2023

@electronicboy The option is made to opt in and the Kqueue dependency is removed.

@AlfieC
Copy link

AlfieC commented Jul 25, 2023

I think somebody tried io_uring on paper recently and it went sideways, so this should probably be a configurable thing, afaik the thing is still a bit of a moving implementation and has not really been called "stable" yet

zlib packet compression seems to be broken on io_uring on PaperMC having tried this last week. Should double check if that's the case here @skbeh

this is due to timing the compression packet sending incorrectly on io_uring. we use io_uring in prod without issues (both on server & velocity). will likely push a PR to Paper with this fixed later

@electronicboy
Copy link
Member

electronicboy commented Jan 18, 2024

fwiw, async http client has been removed from velocity, I have no idea what to do on the basis of this PR, I generally have little investment into IO_URING, so, I'm not sure what the other contributors thing, at the very least, having the thing opt-in at least makes it mergable to me

@astei
Copy link
Contributor

astei commented Aug 10, 2024

We'll likely do #1380 instead, where the io_uring transport becomes stable, and we make it the default for systems that support it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants