Velocity icon indicating copy to clipboard operation
Velocity copied to clipboard

Add support for io_uring

Open skbeh opened this issue 1 year ago • 20 comments

skbeh avatar Apr 08 '23 16:04 skbeh

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.

hugmanrique avatar Apr 08 '23 20:04 hugmanrique

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

electronicboy avatar Apr 08 '23 20:04 electronicboy

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

clrxbl avatar Apr 08 '23 23:04 clrxbl

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

skbeh avatar Apr 09 '23 01:04 skbeh

@electronicboy Added.

skbeh avatar Apr 22 '23 06:04 skbeh

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?

clrxbl avatar Apr 28 '23 15:04 clrxbl

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

skbeh avatar Apr 28 '23 15:04 skbeh

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?

clrxbl avatar May 02 '23 18:05 clrxbl

  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

electronicboy avatar May 02 '23 20:05 electronicboy

@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).

skbeh avatar May 03 '23 02:05 skbeh

Not sure why either of those matter?

electronicboy avatar May 03 '23 02:05 electronicboy

@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.

skbeh avatar May 03 '23 03:05 skbeh

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

electronicboy avatar May 03 '23 03:05 electronicboy

@electronicboy That's what AHC had been always using: https://github.com/AsyncHttpClient/async-http-client/blob/e7edd2c378fb4487be11c47bd624034094c2ae24/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java#L158

skbeh avatar May 03 '23 03:05 skbeh

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

skbeh avatar May 03 '23 03:05 skbeh

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

electronicboy avatar May 03 '23 03:05 electronicboy

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

skbeh avatar Jun 22 '23 11:06 skbeh

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

AlfieC avatar Jul 25 '23 16:07 AlfieC

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

electronicboy avatar Jan 18 '24 11:01 electronicboy

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

astei avatar Aug 10 '24 19:08 astei