POC output with URring #2357
POC output with URring #2357
Conversation
|
Can one of the admins verify this patch? |
11 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Thanks for this patch! I've left a few structural notes in the diff. Note that I haven't done a detailed code review yet, I'm just laying out some thoughts. As we work on the structure I'll continue to re-review the implementation. I've also not deeply looked into the changes to SelectorUring and LinuxUring yet, those are my next round of review. Generally though, this is looking really solid!
Sources/NIOPosix/SocketChannel.swift
Outdated
| let storageRefs = eventLoop.storageRefs | ||
| let controlMessageStorage = eventLoop.controlMessageStorage | ||
| #endif | ||
| self.pendingWrites = PendingDatagramWritesManager(msgs: msgs, |
There was a problem hiding this comment.
I don't love having the pending writes managers take ownership of pointers allocated outside their scope. Can we have two different initialisers, one where they allocate the pointers themselves and one where they don't, and then keep track of what happened with a flag? Then we can drop most of the compile time conditionals, which will make our lives a lot easier.
There was a problem hiding this comment.
I do not love that either...
Had an impression having a different initialisers will require bigger changes in tests...
But may be we could use something different?
For example we could introduce a cache of IOVector buffers in the EventLoop, and then each channel could use that cache. Without URing that cache will always have a one buffer. With URing - probably more than one but less than total number of channels. We could save some memory in that case. And the buffer management logic will not require any conditional compilation at all in that case. What do you think?
There was a problem hiding this comment.
I’m certainly open to that approach. I wonder if it’s worth landing that cache as a separate PR by itself, actually.
There was a problem hiding this comment.
I could probably try to implement that cache in a separate branch,
then we will merge it, and then I will merge into uring-out branch, and then we will continue with it.
What do you think?
There was a problem hiding this comment.
Yeah, that seems like a good idea to me.
There was a problem hiding this comment.
So that's #2358 - just commenting to tie it together with this comment chain.
| } | ||
| } | ||
|
|
||
| private func _debugPrint(_ s: @autoclosure () -> String) { | ||
| #if SWIFTNIO_IO_URING_DEBUG |
There was a problem hiding this comment.
Any reason we need this conditional?
There was a problem hiding this comment.
That condition was before I started to work on it, just left it...
There was a problem hiding this comment.
I think it's a remnant of the original bringup to not having to define out every debugPrint that is in place IIRC (~40).
There are additionally three more specific defines for more in-depth debugging, but they were in few places and are completely defined out (https://github.com/search?q=repo%3Aapple%2Fswift-nio%20SWIFTNIO_IO_URING_DEBUG&type=code) .
Perhaps there's a better way to do this (maybe with the macro support coming up? Basically, one would like to be able to build with debug logging during development but to completely remove traces of it for production as this may be a fairly performance sensitive thing).
Sources/NIOPosix/SocketChannel.swift
Outdated
| let storageRefs = eventLoop.storageRefs | ||
| let controlMessageStorage = eventLoop.controlMessageStorage | ||
| #endif | ||
| self.pendingWrites = PendingDatagramWritesManager(msgs: msgs, |
There was a problem hiding this comment.
I’m certainly open to that approach. I wonder if it’s worth landing that cache as a separate PR by itself, actually.
|
I got the point regarding the conditional compilation. |
|
I'm inclined to suggest that we have most backends implement the functions with |
|
Could you have a look at PR? |
|
Yes, I think that works fine. |
Remove conditional compilation.
| private var bufferPool: Pool<PooledBuffer> | ||
| private var buffer: PooledBuffer? |
There was a problem hiding this comment.
Why do we need the local buffer reference?
There was a problem hiding this comment.
There are 2 reasons for that:
- we will need to put that pooled buffer back to the pool when async operation will be completed
- we should keep a memory block which is used for the async operation while the operation will not be completed
And we also have 2 options how to do it:
- tie that pooled buffer to the asynchronous write request
- keep it locally, so when async request will be completed use local stored object to put it back to the pool
|
Could you have a look at PR? |
|
I'm entirely happy with us adding this conditional compilation into the tests, as it's small and clear. 👍 |
Disable possibility to send files with URing.
|
So what would be good next steps if we want to move this forward? |
|
I merged most recent changes from the main into the feature branch and stabilised tests, so all tests (except skipped are passed now). |
Use URing on Linux.
URing on Linux provides an API to demultiplex events and perform socket operations.
NIO originally designed to demultiplex events and perform all IO in a single thread, while URing socket operations are asynchronous. It requires more significant changes in the NIO taking into account the requirement to additionally manage memory buffers for pending read and write operations.
The one idea was to try to minimise PR as possible, so that PR send outbound data using URing, but still read inbound data using base socket API. Reading inbound data with URing is a next step.
Some highlights regarding changes in the PR
The URing API using on Linux can be enabled by the conditional compilation.
In some places I saw '#if defined(SWIFTNIO_USE_IO_URING) && (os(Linux) || os(Android))
As of now we have no possibility even to compile it for Android, not saying to test, so all URing related code is under just os(Linux). Let's decide later what can we do with Android.
NIO used some buffers stored in the EventLoop (iovecs, storageRefs etc) which are shared across all channels registered in the event loop. That approach does not work with URing because of its asynchronous nature. There are not too many ways to workaround it for URing, so I moved them to the channel, so with URing each channel has own iovecs and storageRefs buffers. Probably we could cache them... not clear what is better... let's discuss...
URing has no analogue for the sendfile() system call.
Instead they suggest to splice() the file via intermediate pipe. It works, but requires an intermediate pipe. There are not too many options how to manage intermediate pipes, but I can't see a good one. So, as of now we just cache intermediate pipes and reuse them...
URing has no analogue for sendmmsg() system call.
Unfortunately did not found a good way to workaround that. Currently just send messages one by one. One possible optimisation here is to collect all messages with the same destination address and same ancillary data and send them with one sendmsg(), but did not found such obvious optimisation in the NIO, so probably it is not a case.
I had to disable few tests which checks the order of system calls. Have no idea how we could make them work for URing, because URing is asynchronous. Let's discuss them later. All other tests works on my environment, but I will not be surprised if they will not work somewhere else.
Will add some information later within comments to PR if will recall something important.