-
Notifications
You must be signed in to change notification settings - Fork 910
Use Unpooled in StreamedRequest#onNext #1136
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
Conversation
the onNext method can be called from a non SDK owned thread, and if we allocate the ByteBuf using the pooling allocator, then we pollute the customer's thread with a ThreadLocal. Using Unpooled#wrappedBuffer avoids actually copying the arrays so this should be more performant as well. Fixes aws#1133
Codecov Report
@@ Coverage Diff @@
## master #1136 +/- ##
============================================
- Coverage 58.77% 58.75% -0.03%
+ Complexity 4557 4556 -1
============================================
Files 742 742
Lines 22941 22895 -46
Branches 1713 1707 -6
============================================
- Hits 13483 13451 -32
+ Misses 8770 8754 -16
- Partials 688 690 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any testing we can add here?
@spfink Can't think of a good one. This avoids creating the ThreadLocal(s) used by the memory pooling mechanism in Netty so ideally we would check that they aren't created but not sure how besides using reflection to to get the TreadLocal instance and calling |
…62fed70f4 Pull request: release <- staging/cd627411-16db-438d-b2f7-69162fed70f4
Description
the onNext method can be called from a non SDK owned thread, and if we allocate
the ByteBuf using the pooling allocator, then we pollute the customer's thread
with a ThreadLocal.
Using Unpooled#wrappedBuffer avoids actually copying the arrays so this should
be more performant as well.
Motivation and Context
Fixes #1133
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsLicense