THRIFT-4714: optimize java TFramedTransports to call write only once per flush #1671
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
|
Going to leave this here for a day or two for visibility and review. |
allengeorge
reviewed
Jan 3, 2019
lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java
Outdated
Show resolved
Hide resolved
lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java
Outdated
Show resolved
Hide resolved
lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java
Outdated
Show resolved
Hide resolved
|
@allengeorge check this out again, I changed it. |
allengeorge
reviewed
Jan 3, 2019
| * This allows framed transport to reserve space | ||
| * for the frame buffer length. | ||
| */ | ||
| public AutoExpandingBufferWriteTransport(int initialCapacity, int frontReserve) { |
There was a problem hiding this comment.
Probably ensure that initialCapacity >= frontReserve? Throw an IllegalArgumentException if it doesn't?
Other than that, I like the fact that there's no conditional check on each write()
allengeorge
approved these changes
Jan 3, 2019
|
@allengeorge IAE added with tests. |
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.
Code calls two writes to the underlying transport per flush: one for the frame size and one for the payload. The result is that folks use TBufferedTransport on top of TFramedTransport to avoid getting two ethernet frames for small packets. This means everything is double-buffered!
Based on THRIFT-1121 and THRIFT-959 this may have been an issue for a long time - the fix was easy however. We make room for the frame size before writing to the buffer, then we only have to pull out the buffer, drop the frame size into the 4 bytes that we pre-allocated, and then write to the underlying transport once.