-
Notifications
You must be signed in to change notification settings - Fork 94
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
Replace tokio-uring
with io-uring
#993
Conversation
ab6b74a
to
ce225a3
Compare
Can you run and include the results some benchmarks compared to main? |
Also fleshes out all SAFETY comments
Build Succeeded 🥳 Build Id: e2e97722-c167-429b-82e2-4c22c253940e The following development images have been built, and will exist for the next 30 days: To build this version:
|
I've reviewed the code and LGTM, once we have some benchmark results so we know that it is and how much of a performance improvement it is, I'll approve |
The read-write benchmarks are non-functional even on main, so I need to figure out why they are broken and fix them first I guess. |
main:
pr:
This basically lines up with what I expected, they are very close to each other in the simplest case of 1<->1, but I would expect the difference to grow a bit with more clients and servers. It's at least not worse. |
This is a fairly major change to swap out
tokio-uring
with the lower levelio-uring
, which has some upsides and downsides.Upsides
In
tokio-uring
, every udp recv_from and send_to performs 3 heap allocations (maybe even more in other parts of the code?) which is extremely wasteful in the context of a proxy that can be sending and receiving many thousands of packets a second. Moving toio-uring
means we need to take responsibility for the lifetimes of memory being written/read by the kernel during I/O, but means we can minimize/get rid of memory allocations since we have the full context. For example, the QCMP loop now doesn't use the heap at all in favor of just reusing stack allocations.Additionally, the current code which forwards packets either downstream or upstream only ever sends 1 packet at a time per worker/session, the new code takes advantage of not being async/await by just sending up to a few thousand packets concurrently, reducing a (probably minor) throughput bottleneck.
Downsides
A lot more code, some of which is unsafe, though slightly less than it could have been, as now the session and packet_router both share the same implementation. The non-linux code is also now separated since they are no longer really compatible since the io uring loop is not async so we can't pretend the code is the same between linux and non-linux, which also contributes to code increase.
Overall, it's just simply more complicated relative to the old code, but does give us tighter control.