Moving PooledRecvBufferAllocator from NIOPosix to NIOCore
#3110
Conversation
| @@ -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.
There was a problem hiding this comment.
I added your suggestion in the documentation.
| 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
There was a problem hiding this comment.
Good catch. Removed from the code.
| @@ -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
There was a problem hiding this comment.
I've added the prefix. Great catch.
| /// - 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 _.
There was a problem hiding this comment.
I've changed this and related functions/properties to be @inlinable/@usableFromInline as you suggested.
| /// Channels can read multiple times per cycle (based on `ChannelOptions.maxMessagesPerRead`), and they reuse | ||
| /// the inbound buffer for each read. If a `ChannelHandler` holds onto this buffer, then CoWing will be needed. | ||
| /// A `NIOPooledRecvBufferAllocator` cycles through preallocated buffers to avoid CoWs during the same read cycle. | ||
| public struct NIOPooledRecvBufferAllocator { |
There was a problem hiding this comment.
As a public type we should annotate whether this is Sendable or not. It looks like it is.
There was a problem hiding this comment.
I've introduced the change requested.
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.