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

transport: write buffer size should equal the size specified by the user #5758

Closed
easwars opened this issue Nov 2, 2022 · 3 comments
Closed

Comments

@easwars
Copy link
Contributor

easwars commented Nov 2, 2022

This issues addresses bullet point 1 from #5751.

Write buffer sizes can be configured on the client and the server. The docstring on these two APIs clearly states:

The corresponding memory allocation for this buffer will be twice the size to keep syscalls low.

Looking at the implementation though, it is not obvious what benefit we get out of configuring a buffer which is twice the size.

Once we have addressed #5757, we can see if there is any change in benchmark results as a result of using a buffer which is the same size as configured by the user.

Another possibly important data point is that the buffer is flushed not only when we have buffered more than the user configured size, but also when the loopy writer decides that it should be flushed. It is important to understand why this is being done. See:

l.framer.writer.Flush()

@atollena
Copy link
Collaborator

Another possibly important data point is that the buffer is flushed not only when we have buffered more than the user configured size, but also when the loopy writer decides that it should be flushed. It is important to understand why this is being done.

This call to Flush() is necessary to make sure that we eventually write data on the transport when the buffer is not filled entirely. So each time there is data available and loopy runs, it gets a chance to flush the buffer.

@atollena
Copy link
Collaborator

atollena commented Nov 21, 2022

I ran some benchmark and put the results alongside a proposed change in this pull request: #5807.

@atollena
Copy link
Collaborator

This was fixed in #6983.

@dfawley dfawley closed this as completed Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants