core: reduce CompositeReadableBuffer allocation#3279
Conversation
Add ability for CompositeReadableBuffer read specified length bytes to another CompositeReadableBuffer instead of create temp CompositeReadableBuffer. Close: grpc#3278
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure. |
|
Now in allocation top netty netty/netty#7026 |
|
I'm fine with these changes, but I'd like @carl-mastrangelo to review. |
carl-mastrangelo
left a comment
There was a problem hiding this comment.
I suspect this may hurt streaming RPCs, though unary seems to pass.
@Gordiychuk Would you mind looking at a previous PR that modifies this file in a different way: #2092
|
@carl-mastrangelo, what do we want to do here? The change seemed fair to me and seems to be a easy way to reduce garbage. Was your concern just the |
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
carl-mastrangelo
left a comment
There was a problem hiding this comment.
LGTM, (assuming it can merge)
|
@Gordiychuk Would you please sign the CLA (otherwise we can't merge) and then resolve the merge conflicts? |
|
I signed it |
|
This PR was approved but now needs to be rebased after resolving conflicts and also needs the new CLA. Otherwise it should be closed I think. |
|
Hey @Gordiychuk, any plans to rebase soon to get this in, keen to see this get merged. If not I'm happy to create a new PR with these changes based on master. Let me know, thanks. |
|
Note: #7375 might have partly addressed the same allocation. But the solution here is simpler and more predictable, so we could have both. |
Add ability for CompositeReadableBuffer read specified length bytes
to another CompositeReadableBuffer instead of create temp
CompositeReadableBuffer.
Allocation before changes:
Allocation after changes:
Close: #3278