Skip to content
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

core: reduce CompositeReadableBuffer allocation #3279

Conversation

Gordiychuk
Copy link
Contributor

Add ability for CompositeReadableBuffer read specified length bytes
to another CompositeReadableBuffer instead of create temp
CompositeReadableBuffer.

Allocation before changes:
screenshot from 2017-07-26 19 30 32

Allocation after changes:
screenshot from 2017-07-26 19 30 42

Close: #3278

Add ability for CompositeReadableBuffer read specified length bytes
to another CompositeReadableBuffer instead of create temp
CompositeReadableBuffer.

Close: grpc#3278
@grpc-kokoro
Copy link

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.

@Gordiychuk
Copy link
Contributor Author

Now in allocation top netty netty/netty#7026

@ejona86
Copy link
Member

ejona86 commented Jul 26, 2017

I'm fine with these changes, but I'd like @carl-mastrangelo to review.

Copy link
Contributor

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ejona86
Copy link
Member

ejona86 commented Aug 24, 2018

@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 new ArrayDeque<ReadableBuffer>(2)? I do agree that the default of 16 is probably a bit much, maybe you'd be happy with 4? Worst-case, maybe we revert that part of the change but apply the other changes?

@thelinuxfoundation
Copy link

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,
CLA GitHub bot

Copy link
Contributor

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, (assuming it can merge)

@zhangkun83
Copy link
Contributor

@ejona86 @carl-mastrangelo ping

Copy link
Contributor

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dapengzhang0
Copy link
Member

@Gordiychuk Would you please sign the CLA (otherwise we can't merge) and then resolve the merge conflicts?

@Gordiychuk
Copy link
Contributor Author

I signed it

@sanjaypujare
Copy link
Contributor

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.

@georgelee1
Copy link

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.

@larry-safran larry-safran added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Feb 13, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Feb 13, 2023
@ejona86
Copy link
Member

ejona86 commented Feb 22, 2023

Note: #7375 might have partly addressed the same allocation. But the solution here is simpler and more predictable, so we could have both.

@larry-safran larry-safran merged commit 0f21574 into grpc:master Dec 28, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CompositeReadableBuffer and GC overhead
10 participants