Skip to content
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

Moving PooledRecvBufferAllocator from NIOPosix to NIOCore #3110

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -12,10 +12,10 @@
//
//===----------------------------------------------------------------------===//

import NIOCore

/// A receive buffer allocator which cycles through a pool of buffers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should extend this to express when users might want to use this type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 a ChannelHandler holds onto this buffer, then CoWing will be needed. A PooledRecvBufferAllocator cycles 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// The pool will either use a single buffer (i.e. `buffer`) OR store multiple buffers
// in `buffers`. If `buffers` is non-empty then `buffer` MUST be `nil`. If `buffer`
// is non-nil then `buffers` MUST be empty.
@@ -28,14 +28,19 @@ internal struct PooledRecvBufferAllocator {
private var lastUsedIndex: Int

/// Maximum number of buffers to store in the pool.
internal private(set) var capacity: Int
public private(set) var capacity: Int
/// The receive allocator providing hints for the next buffer size to use.
internal var recvAllocator: RecvByteBufferAllocator
public var recvAllocator: RecvByteBufferAllocator

/// The return value from the last call to `recvAllocator.record(actualReadBytes:)`.
private var mayGrow: Bool

init(capacity: Int, recvAllocator: RecvByteBufferAllocator) {
/// Builds a new instance of `PooledRecvBufferAllocator`
///
/// - Parameters:
/// - capacity: Maximum number of buffers to store in the pool.
/// - recvAllocator: The receive allocator providing hints for the next buffer size to use.
public init(capacity: Int, recvAllocator: RecvByteBufferAllocator) {
precondition(capacity > 0)
self.capacity = capacity
self.buffer = nil
@@ -46,7 +51,7 @@ internal struct PooledRecvBufferAllocator {
}

/// Returns the number of buffers in the pool.
var count: Int {
public var count: Int {
if self.buffer == nil {
// Empty or switched to `buffers` for storage.
return self.buffers.count
@@ -58,7 +63,10 @@ internal struct PooledRecvBufferAllocator {
}

/// Update the capacity of the underlying buffer pool.
mutating func updateCapacity(to newCapacity: Int) {
///
/// - Parameters:
/// - newCapacity: The new capacity for the underlying buffer pool.
public mutating func updateCapacity(to newCapacity: Int) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the missing documentation. Let me know if you think I should change anything else.

precondition(newCapacity > 0)

if newCapacity > self.capacity {
@@ -81,14 +89,21 @@ internal struct PooledRecvBufferAllocator {

/// Record the number of bytes which were read.
///
/// Returns whether the next buffer will be larger than the last.
mutating func record(actualReadBytes: Int) {
/// - Parameters:
/// - actualReadBytes: Number of bytes being recorded
/// - Returns: whether the next buffer will be larger than the last.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't actually returning anything so I'd get rid of this line

public mutating func record(actualReadBytes: Int) {
self.mayGrow = self.recvAllocator.record(actualReadBytes: actualReadBytes)
}

/// Provides a buffer with enough writable capacity as determined by the underlying
/// receive allocator to the given closure.
mutating func buffer<Result>(
///
/// - Parameters:
/// - 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>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 _.

allocator: ByteBufferAllocator,
_ body: (inout ByteBuffer) throws -> Result
) rethrows -> (ByteBuffer, Result) {