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

Add new ChannelOption to get the amount of buffered outbound data in the Channel #2849

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

johnnzhou
Copy link

@johnnzhou johnnzhou commented Aug 20, 2024

Add ability to get the amount of buffered outbound data from Channel

Motivation:

Right now, SwiftNIO does not have the API to answer the question "how much data is buffered in the Channel". Applications focusing on performance may need to fine-tune the amount of outbound data that will be sent to optimize data throughput, adjust sending rate to avoid overflow, and potentially reduce latency.

SwiftNIO currently provides some backpressure mechanism. This new API will be a good addition. By knowing how much data is buffered directly, applications can make informed decision to adjust for optimal buffer sizes and send rates.

Modifications:

  • Expose current buffer size through ChannelOptions so that users can read the value out. StreamSocketChannel, DatagramSocketChannel, EmbeddedChannel, and AsyncTestingChannel have the same API interface.
  • Various modifications to the existing tests to make sure the new API is working correctly.
  • Add a new so_sndbuf socket option so that users can easily adjust the send buffer size.

Result:

Users can get the amount of outbound bytes currently buffered in the Channel through the new BufferedWritableBytesOption channel option.

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.

Thanks for this, it generally looks good! Can you add support for the new channel option in EmbeddedChannel and AsyncTestingChannel as well? It should only behave sensibly when the buffered data is ByteBuffer, so runtime type assertions there are fine.

While we're here, it's worth calling out that this is an incomplete solution, as there may be ChannelHandlers that have also buffered some bytes. This is a very useful building block, but we may want to revisit this space to add more fully-featured functionality for this.

Sources/NIOCore/ChannelOption.swift Outdated Show resolved Hide resolved
@johnnzhou
Copy link
Author

johnnzhou commented Aug 21, 2024

Hi @Lukasa,

While we're here, it's worth calling out that this is an incomplete solution, as there may be ChannelHandlers that have also buffered some bytes. This is a very useful building block, but we may want to revisit this space to add more fully-featured functionality for this.

thanks for bringing it up. I'm planning to follow our discussion here to add a new protocol that audits the outbound buffered bytes. Anyone who's interested in is also welcome to contribute :). I will fix the comments ASAP.

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, one quick note, can we get tests for the EmbeddedChannel and AsyncTestingChannel implementations?

@johnnzhou
Copy link
Author

Sure. Actually I'm working on the tests - should be ready soon. Thanks for the reminder!

@weissi
Copy link
Member

weissi commented Aug 23, 2024

Super cool, thanks @johnnzhou !!

@johnnzhou johnnzhou requested a review from Lukasa August 27, 2024 06:30
@johnnzhou
Copy link
Author

Hi @Lukasa, whenever you are available, could you take a look at the code change. Appreciate your time and help in advance!

@johnnzhou
Copy link
Author

johnnzhou commented Sep 3, 2024

Hi @Lukasa @weissi, thanks for the help refining my PR. I think it is ready now. Whenever you have a moment, could you please review the change? Your feedback is greatly appreciated! Thanks for your time!

@Lukasa Lukasa added the semver/minor Adds new public API. label Sep 16, 2024
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.

Very nice patch @johnnzhou, thank you so much and sorry for the delay.

@Lukasa Lukasa enabled auto-merge (squash) September 16, 2024 13:24
@johnnzhou
Copy link
Author

johnnzhou commented Sep 16, 2024

Thanks @Lukasa for the review! I synced the PR with the main. It requires maintainer approval to kick off the pipeline. If you are available, can you help rerun the pipeline? Thanks!

part 2 of this feature is also on its way.

auto-merge was automatically disabled September 17, 2024 18:18

Head branch was pushed to by a user without write access

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