Change how TCP connection reads data to improve performance #3178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Port over of the "faster tcp" logic from wallaroo PRs: WallarooLabs/wally#2203 WallarooLabs/wally#2563 That provides a significant performance improvement to the throughputs and latencies for the wallaro performance test.
SeanTAllen
added a commit
that referenced
this pull request
Jun 18, 2019
Prior to this commit, the buffering in TCPConnection was overly complicated. This was a carry over from the old style of buffer handling. However, after Dipin Hora's commits to bring over the "faster tcp" changes from Wallaroo, the buffer handling no longer makes sense. Prior to this commit, a TCPConnection would be provided two variables to control read buffering: init_size, max_size. The read buffer would start at init_size and would grow to max_size. Before "faster tcp", the buffer would only grow in size if we read the entire buffer's worth of data in one recv call. After the data was read, the buffer would be truncated and then handed off to the TCPConnectionNotify. This was very wasteful. If you had a buffer of 128 allocated bytes and only read 4, then the 124 allocated bytes would be truncated away and the buffer would be reallocated. With "faster tcp", we allocate a large buffer and when reading data, we chop it off the buffer as needed. This means, that "grow the buffer" doesn't make much sense anymore. The buffer will continually shrink in size as we slowly chop more and more off. This meant that with the code as it is prior to change, we would always grow to max buffer size. If we are always going to grow to max buffer size, it makes sense to simplify the code and our interfaces and only allow for setting the buffer size. This commit makes that simplication. Now we will: - allocate a buffer of size `read_buffer_size` - reallocate when the buffer is empty - reallocate if the caller is using `expect` and the buffer is too small to hold the expected payload This is a breaking change for TPCConnection and TCPListener as it changes the constructor interfaces. Additional information on the "faster tcp" change can be found in the following PR: #3178
SeanTAllen
added a commit
that referenced
this pull request
Jun 18, 2019
Prior to this commit, the buffering in TCPConnection was overly complicated. This was a carry over from the old style of buffer handling. However, after Dipin Hora's commits to bring over the "faster tcp" changes from Wallaroo, the buffer handling no longer makes sense. Prior to this commit, a TCPConnection would be provided two variables to control read buffering: init_size, max_size. The read buffer would start at init_size and would grow to max_size. Before "faster tcp", the buffer would only grow in size if we read the entire buffer's worth of data in one recv call. After the data was read, the buffer would be truncated and then handed off to the TCPConnectionNotify. This was very wasteful. If you had a buffer of 128 allocated bytes and only read 4, then the 124 allocated bytes would be truncated away and the buffer would be reallocated. With "faster tcp", we allocate a large buffer and when reading data, we chop it off the buffer as needed. This means, that "grow the buffer" doesn't make much sense anymore. The buffer will continually shrink in size as we slowly chop more and more off. This meant that with the code as it is prior to change, we would always grow to max buffer size. If we are always going to grow to max buffer size, it makes sense to simplify the code and our interfaces and only allow for setting the buffer size. This commit makes that simplication. Now we will: - allocate a buffer of size `read_buffer_size` - reallocate when the buffer is empty - reallocate if the caller is using `expect` and the buffer is too small to hold the expected payload This is a breaking change for TPCConnection and TCPListener as it changes the constructor interfaces. Additional information on the "faster tcp" change can be found in the following PR: #3178
patches11
pushed a commit
to patches11/ponyc
that referenced
this pull request
Jun 29, 2019
patches11
pushed a commit
to patches11/ponyc
that referenced
this pull request
Jun 29, 2019
Prior to this commit, the buffering in TCPConnection was overly complicated. This was a carry over from the old style of buffer handling. However, after Dipin Hora's commits to bring over the "faster tcp" changes from Wallaroo, the buffer handling no longer makes sense. Prior to this commit, a TCPConnection would be provided two variables to control read buffering: init_size, max_size. The read buffer would start at init_size and would grow to max_size. Before "faster tcp", the buffer would only grow in size if we read the entire buffer's worth of data in one recv call. After the data was read, the buffer would be truncated and then handed off to the TCPConnectionNotify. This was very wasteful. If you had a buffer of 128 allocated bytes and only read 4, then the 124 allocated bytes would be truncated away and the buffer would be reallocated. With "faster tcp", we allocate a large buffer and when reading data, we chop it off the buffer as needed. This means, that "grow the buffer" doesn't make much sense anymore. The buffer will continually shrink in size as we slowly chop more and more off. This meant that with the code as it is prior to change, we would always grow to max buffer size. If we are always going to grow to max buffer size, it makes sense to simplify the code and our interfaces and only allow for setting the buffer size. This commit makes that simplication. Now we will: - allocate a buffer of size `read_buffer_size` - reallocate when the buffer is empty - reallocate if the caller is using `expect` and the buffer is too small to hold the expected payload This is a breaking change for TPCConnection and TCPListener as it changes the constructor interfaces. Additional information on the "faster tcp" change can be found in the following PR: ponylang#3178
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Port over of the "faster tcp" logic from wallaroo PRs:
WallarooLabs/wally#2203
WallarooLabs/wally#2563
That provides a significant performance improvement to the
throughputs and latencies for the wallaro performance test.
Also fix performance issue with buffer handling for non-expect case