Only create composite bytebufs if there is more than one incoming gRPC frame. #1167
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
Codecov Report
@@ Coverage Diff @@
## master #1167 +/- ##
==========================================
- Coverage 72.72% 72.68% -0.05%
==========================================
Files 517 517
Lines 23192 23203 +11
Branches 2898 2900 +2
==========================================
- Hits 16866 16864 -2
- Misses 4789 4797 +8
- Partials 1537 1542 +5
Continue to review full report at Codecov.
|
trustin
approved these changes
Apr 27, 2018
minwoox
approved these changes
Apr 30, 2018
hyangtack
approved these changes
Apr 30, 2018
|
Thanks, @anuraaga! |
trustin
pushed a commit
that referenced
this pull request
Oct 18, 2018
Actually, `firstFrame` was never set because I forgot to remove line 197 >< I guess the optimization previously in #1167 was just from the removal of `nextFrame`. I tried finishing the `firstFrame` optimization but there was no difference so just getting rid of it.
fmguerreiro
pushed a commit
to fmguerreiro/armeria
that referenced
this pull request
Sep 19, 2020
…me. (line#1167) Currently, we use a `CompositeByteBuf` to collect incoming frames, and we read required bytes into another `CompositeByteBuf` as data is available. This causes a lot of overhead for the common case where the message fits in one frame. There also doesn't seem to be a good reason to read into a `nextFrame` as we get bytes, instead of when we have enough bytes (I had copied the pattern in this code pretty much verbatim from upstream without thinking too much about it). Now, a `CompositeByteBuf` for incoming frames is only created on the second one. If the stream is finished with a single frame, no composites will ever be allocated. Also, incoming bytes are only read when we have enough bytes to deliver a message - this means we don't need to allocate a composite to keep track of read bytes pre-delivery. After ``` # Run complete. Total time: 00:01:13 Benchmark (clientType) Mode Cnt Score Error Units DownstreamSimpleBenchmark.empty NORMAL thrpt 20 13314.558 ± 223.515 ops/s Benchmark result is saved to /home/anuraag/git/armeria/benchmarks/build/reports/jmh/results.txt ``` Before ``` # Run complete. Total time: 00:01:13 Benchmark (clientType) Mode Cnt Score Error Units DownstreamSimpleBenchmark.empty NORMAL thrpt 20 12508.147 ± 180.084 ops/s Benchmark result is saved to /home/anuraag/git/armeria/benchmarks/build/reports/jmh/results.txt ```
fmguerreiro
pushed a commit
to fmguerreiro/armeria
that referenced
this pull request
Sep 19, 2020
Actually, `firstFrame` was never set because I forgot to remove line 197 >< I guess the optimization previously in line#1167 was just from the removal of `nextFrame`. I tried finishing the `firstFrame` optimization but there was no difference so just getting rid of it.
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.
Currently, we use a
CompositeByteBufto collect incoming frames, and we read required bytes into anotherCompositeByteBufas data is available. This causes a lot of overhead for the common case where the message fits in one frame. There also doesn't seem to be a good reason to read into anextFrameas we get bytes, instead of when we have enough bytes (I had copied the pattern in this code pretty much verbatim from upstream without thinking too much about it).Now, a
CompositeByteBuffor incoming frames is only created on the second one. If the stream is finished with a single frame, no composites will ever be allocated. Also, incoming bytes are only read when we have enough bytes to deliver a message - this means we don't need to allocate a composite to keep track of read bytes pre-delivery.After
Before