Skip to content

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Feb 3, 2023

Motivation:

Channels can read ChannelOptions.maxMessagesPerRead times from a socket in each read cycle. They typically re-use the same buffer for each read and rely on it CoWing if necessary. If we read more than once in a cycle then we will always CoW the buffer. Instead of reusing one buffer we can reuse a pool of buffers limited by maxMessagesPerRead and cycle through each, reducing the chance of CoWing the buffers.

Modifications:

  • Add NIOPoolableRecvByteBufferAllocator which extends RecvByteBufferAllocator to provide the size of the next buffer.
  • Add an internal pool which uses a NIOPoolableRecvByteBufferAllocator
  • Add a wrapper which is either backed by poolable allocator or non-poolable allocator.

Results:

Fewer allocations

@Lukasa
Copy link
Contributor

Lukasa commented Feb 3, 2023

If we read more than once in a cycle then we will always CoW the buffer.

Side note: I don't think this is entirely true. It's only true if a ChannelHandler holds onto that buffer. Most don't: if they have swift-nio-ssl in the way then it will always drop the inbound buffer.

@tomerd
Copy link
Member

tomerd commented Feb 3, 2023

@swift-server-bot test this please

3 similar comments
@tomerd
Copy link
Member

tomerd commented Feb 3, 2023

@swift-server-bot test this please

@tomerd
Copy link
Member

tomerd commented Feb 3, 2023

@swift-server-bot test this please

@tomerd
Copy link
Member

tomerd commented Feb 3, 2023

@swift-server-bot test this please

@tomerd
Copy link
Member

tomerd commented Feb 3, 2023

@swift-server-bot test this please

@glbrntt glbrntt force-pushed the gb-read-buffers branch 4 times, most recently from abeb277 to ab1cece Compare February 6, 2023 17:24
@glbrntt glbrntt requested a review from Lukasa February 8, 2023 09:02
@glbrntt
Copy link
Contributor Author

glbrntt commented Feb 8, 2023

API breakage is the usual false positive of adding to a protocol with a default implementation:

08:18:40 1 breaking change detected in NIOCore:
08:18:40   💔 API breakage: func RecvByteBufferAllocator.nextBufferSize() has been added as a protocol requirement

@glbrntt glbrntt requested a review from Lukasa February 15, 2023 16:53
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Cool, this is getting a lot closer! A few more notes.

var index = startIndex

// Sweep from after the lasted used index through to the end.
while index != self.buffers.endIndex {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely better, but these methods are still large and repetitive. This pair of while loops, for example, can certainly be refactored into an extension method on array which will allow us to reduce the number of lines of code in this method.

if let result = try self.buffer?.modifyIfUniquelyOwned(minimumCapacity: nextBufferSize, body) {
// `result` can only be non-nil if `buffer` is non-nil.
return (self.buffer!, result)
} else if !self.buffers.isEmpty {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this conditional: the code written below has to work correctly regardless so I think we can just remove it.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice one, thanks @glbrntt!

Motivation:

Channels can read `ChannelOptions.maxMessagesPerRead` times from a
socket in each read cycle. They typically re-use the same buffer for
each read and rely on it CoWing if necessary. If we read more than once
in a cycle then we may CoW the buffer. Instead of reusing one buffer we
can reuse a pool of buffers limited by `maxMessagesPerRead` and cycle
through each, reducing the chance of CoWing the buffers.

Modifications:

- Extend `RecvByteBufferAllocator` to provide the size of the next
  buffer with a default implementation returning `nil`.
- Add an recv buffer pool which lazily grows up to a fixed size and
  attempts to reuse buffers where possible if doing so avoids CoWing.

Results:

Fewer allocations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants