Moving PooledRecvBufferAllocator from NIOPosix to NIOCore
#3110
Conversation
| @@ -58,7 +61,7 @@ internal struct PooledRecvBufferAllocator { | |||
| } | |||
|
|
|||
| /// Update the capacity of the underlying buffer pool. | |||
| mutating func updateCapacity(to newCapacity: Int) { | |||
| public mutating func updateCapacity(to newCapacity: Int) { | |||
There was a problem hiding this comment.
Can we update the docs for this and the other public methods to include a list of arguments (i.e. the Parameters keyword as in the public init above)?
There was a problem hiding this comment.
I've added the missing documentation. Let me know if you think I should change anything else.
| @@ -12,10 +12,8 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| import NIOCore | |||
|
|
|||
| /// A receive buffer allocator which cycles through a pool of buffers. | |||
There was a problem hiding this comment.
We should extend this to express when users might want to use this type.
There was a problem hiding this comment.
Is this addition to the comment accurate and sufficient: This type is useful when allocating new buffers for every IO read is expensive or undesirable.?
There was a problem hiding this comment.
Allocating is always expensive, so I'm not sure this is particularly insightful. Maybe something like:
Channels can read multiple times per cycle (based on
ChannelOptions.maxMessagesPerRead), and they reuse the inbound buffer for each read. If aChannelHandlerholds onto this buffer, then CoWing will be needed. APooledRecvBufferAllocatorcycles through preallocated buffers to avoid CoWs during the same read cycle.
| mutating func record(actualReadBytes: Int) { | ||
| /// - Parameters: | ||
| /// - actualReadBytes: Number of bytes being recorded | ||
| /// - Returns: whether the next buffer will be larger than the last. |
There was a problem hiding this comment.
This isn't actually returning anything so I'd get rid of this line
| @@ -12,10 +12,8 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| import NIOCore | |||
|
|
|||
| /// A receive buffer allocator which cycles through a pool of buffers. | |||
There was a problem hiding this comment.
Allocating is always expensive, so I'm not sure this is particularly insightful. Maybe something like:
Channels can read multiple times per cycle (based on
ChannelOptions.maxMessagesPerRead), and they reuse the inbound buffer for each read. If aChannelHandlerholds onto this buffer, then CoWing will be needed. APooledRecvBufferAllocatorcycles through preallocated buffers to avoid CoWs during the same read cycle.
| internal struct PooledRecvBufferAllocator { | ||
| /// | ||
| /// This type is useful when allocating new buffers for every IO read is expensive or undesirable. | ||
| public struct PooledRecvBufferAllocator { |
There was a problem hiding this comment.
This'll need a "NIO" prefix now that it's coming into a public namespace, see https://github.com/apple/swift-nio/blob/main/docs/public-api.md#1-no-global-namespace-additions-that-arent-prefixed
| /// - allocator: `ByteBufferAllocator` used to construct a new buffer if needed | ||
| /// - body: Closure where the caller can use the new or existing buffer | ||
| /// - Returns: A tuple containing the `ByteBuffer` used and the `Result` yielded by the closure provided. | ||
| public mutating func buffer<Result>( |
There was a problem hiding this comment.
This should be @inlinable. In turn it will mean a bunch of other functions and properties will need to be as well. For anything that's private that needs to be @inlinable/@usableFromInline they can become internal and be prefixed with an _.
Moving
PooledRecvBufferAllocatorfrom NIOPosix to NIOCore, making it part of its public interface.Motivation:
We want to introduce the capability of reusing receiving buffers from a pool into swift-nio-transport-services project. To prevent duplicating this functionality there, this change is making the existing functionality from NIO part of its public API.
Modifications:
Moved the type
PooledRecvBufferAllocatorfrom NIOPosix module to NIOCore and turned it into a public type.Result:
PooledRecvBufferAllocatoris part of NIOCore public API.