Pool buffers for ivecs and storage refs in the event loop. #2358
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? |
Sources/NIOCore/Pool.swift
Outdated
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| public class Pool<Element> { |
There was a problem hiding this comment.
We don't want to be in the business of vending this type as public API. It can safely be moved into NIOPosix.
Sources/NIOCore/Pool.swift
Outdated
| private let dtor: (Element) -> Void | ||
| private var elements: [Element] | ||
|
|
||
| public init(maxSize: Int, ctor: @escaping () -> Element, dtor: @escaping (Element) -> Void) { |
There was a problem hiding this comment.
Rather than using separate constructor/destructor functions, we'd typically want to define a protocol to which Element conforms that implements the relevant methods. Something like:
protocol PoolElement {
init()
func evictedFromPool()
}
There was a problem hiding this comment.
I had some debates regarding such approach which is I see is common for swift, but sometimes it does not work well from my point of view. So we are going to extend some type with 'PoolElement' protocol. But is it really needed? Why the particular type should expose the 'PoolElement' functionality and why it should even know that it is going to be pooled with some pool? Often it is only the particular pool who knows what does it going to pool and how... As you can see in the PR there are 2 pools:
- pool to be used in the event loop, which just allocates buffer
- pool to be used in tests, which allocates buffers with markers and check those markers when buffer is not needed any more...
The PoolElement protocol does not give a possibility to have a different pooling logic for the same type in different pools... or at least I can't see a simple way for that...
Or am I missing something?
There was a problem hiding this comment.
The thing you're missing is that the way to pool the "same thing" in different ways is to define a new type with the new behaviour. Swift structs are essentially "free": they're trivial aggregates. That means whenever we want a different behaviour we can wrap the type we want in a new structure that adds that new behaviour.
In general if we were defining a public API I'd be a bit more cautious, but as this is purely an internal data structure I think it's probably nicer to avoid unstructured callbacks.
There was a problem hiding this comment.
So, I made another branch with a Pool<> which operates with something implementing 'PoolElement'.
I did a PoolableBuffer struct implementing default behaviour: https://github.com/ordo-one/swift-nio/blob/aa23e675eb95292f98c08391cf30bec37fed8292/Sources/NIOPosix/SelectableEventLoop.swift#L33
My understanding was we can define a different behaviour for a poolable buffer adding some bounds checks, like I did at https://github.com/ordo-one/swift-nio/blob/aa23e675eb95292f98c08391cf30bec37fed8292/Tests/NIOPosixTests/ChannelTests.swift#L76, but TestPooledBuffer can not be used as a pooled type, the code does not compile...
Am I missing something or did I get you wrong?
There was a problem hiding this comment.
When you say the code doesn't compile, what did you try to do and what happened?
There was a problem hiding this comment.
I pushed a commit with some bounds checks.
a437a52
Could you have a look?
There was a problem hiding this comment.
Do you think we can merge these changes?
What will be better, create a new PR from that branch, or apply those changes to the branch where that PR is made from?
There was a problem hiding this comment.
I think you can merge those changes into this branch.
There was a problem hiding this comment.
Branch pool-iovecs-buffers3 was created from the swift-nio/main, not from the pool-iovecs-buffers, so it would be merged with conflicts.
I applied changes we discussed to the pool-iovecs-buffers, please have a look.
Sources/NIOCore/Pool.swift
Outdated
| } | ||
|
|
||
| deinit { | ||
| while !elements.isEmpty { |
There was a problem hiding this comment.
This can just be a for loop, we don't need to deinit the elements immediately.
|
|
||
| let count = (buffer.count / (MemoryLayout<IOVector>.stride + MemoryLayout<Unmanaged<AnyObject>>.stride)) | ||
| let iovecs = (buffer.baseAddress!).bindMemory(to: IOVector.self, capacity: count) | ||
| let storageRefs = (buffer.baseAddress! + (count * MemoryLayout<IOVector>.stride)).bindMemory(to: Unmanaged<AnyObject>.self, capacity: count) |
There was a problem hiding this comment.
This code is very precise. Can we factor it out into its own method to which we can add assertions on appropriate length?
There was a problem hiding this comment.
not sure I got you...
what kind of assertion would you like to add here?
There was a problem hiding this comment.
So what you're doing here is allocating a single buffer that you then slice into the relevant buffers we want. That's fine, but I'd like to encapsulate all of that dance into a struct that we can use to hide this mess of memory binding. It will also let us add a bunch of pseudo-bounds-checks to our construction code, so we can confirm that we're not actually binding or using memory that is out of bounds of the original allocation. Does that make sense?
There was a problem hiding this comment.
well,
I would look at that with a very simple metric: number of code lines and number of extra entities (we will need to introduce an additional structure). Here we have just 3 lines of code, adding a struct with that functionality will give more than 3 lines... But if you think it is better in that particular case - let's do it.
There was a problem hiding this comment.
Generally I don't think lines of code added is the particular metric I'm most concerned about. What I want is to build abstractions that discourage misuse. A related opportunity is to actively reference count the pointers, but I think this is a one-step-at-a-time kind of deal, so I'm interested for now in giving us a sensible abstraction.
… for the pooled buffers.
|
|
||
| init() { | ||
| self.bufferSize = (MemoryLayout<IOVector>.stride + MemoryLayout<Unmanaged<AnyObject>>.stride) * Socket.writevLimitIOVectors | ||
| #if DEBUG |
There was a problem hiding this comment.
We have a debugOnly helper you can use here.
| private let buffer: UnsafeMutableRawPointer | ||
|
|
||
| init() { | ||
| self.bufferSize = (MemoryLayout<IOVector>.stride + MemoryLayout<Unmanaged<AnyObject>>.stride) * Socket.writevLimitIOVectors |
There was a problem hiding this comment.
Can we add a precondition that MemoryLayout<IOVector>.alignment >= MemoryLayout<Unmanaged<AnyObject>>.alignment? It'll allow us to confirm that this arrangement actually works.
| } | ||
|
|
||
| func get() -> (UnsafeMutableBufferPointer<IOVector>, UnsafeMutableBufferPointer<Unmanaged<AnyObject>>) { | ||
| let count = self.bufferSize / (MemoryLayout<IOVector>.stride + MemoryLayout<Unmanaged<AnyObject>>.stride) |
There was a problem hiding this comment.
This number is Socket.writevLimitIOVectors, right?
Sources/NIOPosix/Pool.swift
Outdated
| } | ||
| } | ||
|
|
||
| public func put(_ e: Element) { |
There was a problem hiding this comment.
Nit: let's strip the publics.
|
@swift-server-bot test this please |
| assert(iovecs <= (self.buffer + self.bufferSize).bindMemory(to: IOVector.self, capacity: count)) | ||
| assert(storageRefs >= self.buffer.bindMemory(to: Unmanaged<AnyObject>.self, capacity: count)) | ||
| assert(storageRefs <= (self.buffer + self.bufferSize).bindMemory(to: Unmanaged<AnyObject>.self, capacity: count)) | ||
| assert(UnsafeMutableRawPointer(iovecs + count) == UnsafeMutableRawPointer(storageRefs)) |
There was a problem hiding this comment.
Don't bind the memory in these assertions, the duplicate bind makes it hard to reason about. Erase the pointers to raw instead.
There was a problem hiding this comment.
It does not compile with swift 5.5/5.6 then, CI works fails...
We could remove bindings but will need to fix a build for 5.5/5.6 in some other way then...
#if swift(>=5.7)?
There was a problem hiding this comment.
or do you mean having it like
assert(UnsafeMutableRawPointer(iovecs) >= self.buffer)
assert(UnsafeMutableRawPointer(iovecs) <= (self.buffer + self.bufferSize))
assert(UnsafeMutableRawPointer(storageRefs) >= self.buffer)
assert(UnsafeMutableRawPointer(storageRefs) <= (self.buffer + self.bufferSize))
assert(UnsafeMutableRawPointer(iovecs + count) == UnsafeMutableRawPointer(storageRefs))
assert(UnsafeMutableRawPointer(storageRefs + count) <= (self.buffer + bufferSize))
|
@swift-server-bot test this please |
|
@swift-server-bot test this please |
|
@swift-server-bot test this please |
There was a problem hiding this comment.
For the sake of keepings things moving forward, I'm happy for us to merge this as-is. However, I'm going to provide a follow-up PR myself to refactor this a bit to make it a little safer. In particular, I want us to avoid the risk of leaking memory, so I'm going to change the layout of the PoolElement somewhat.
Pool and reuse IOVector and storage ref buffers in the event pool.
Motivation:
Having an asynchronous sockets backend (like URing on Linux) require extend a lifetime of memory buffers used as arguments for more than a code block.
Modifications:
Event loop have a pool of buffers to be used for output vector write operations.
Result:
In a case of synchronous sockets backend that pool will always have a single buffer, for asynchronous backends every async call will take buffer from the pool and release the buffer when it will not be needed.