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

For unary methods, received messages are put into buffer from the pool, but buffer is never returned back to pool. #6578

Closed
efremovmi opened this issue Aug 24, 2023 · 17 comments · Fixed by #6766
Assignees

Comments

@efremovmi
Copy link

What version of gRPC are you using?

1.57.0

What version of Go are you using (go version)?

1.20.6

What operating system (Linux, Windows, …) and version?

MacOS

What did you do?

added buffer pool support and I expect that the number of allocations in the application will decrease

What did you expect to see?

I expect to see a decrease in allocations to the query (processUnaryRPC -> recvAndDecompress -> recvMsg)

What did you see instead?

after adding the buffer pool, the number of allocations has not changed (objects are not returned to the pool anywhere else, but they are always taken from it 616 line figure 2)

figure 1:

image

figure 2:

image

@easwars easwars self-assigned this Aug 29, 2023
@easwars
Copy link
Contributor

easwars commented Aug 29, 2023

after adding the buffer pool

How did you do that?

The buffer is retrieved from the pool here and is put back to the pool here.

@hueypark : Do you see any issues at all with the implementation? I remember that you wanted this functionality quite urgently. Have you been using this buffer pool with your application? How has your experience been? Thanks.

@efremovmi
Copy link
Author

efremovmi commented Aug 29, 2023

How did you do that?

I added a SharedBufferPool to my server and set breakpoints in the Get and Put methods. When debugging, the application stopped in the Get method, but never stopped in the Put method. Then I implemented my Buffer Pool and made a structure that would satisfy SharedBufferPool and also made two breakpoints in Get and Put. The result was repeated - Put is never called.

Please try to do the same and call any unary methods, which you can write yourself. If there are problems, I can share my repository, which you can debug.

Judging by Figure 1, I am not the only one who has faced such a problem.

@efremovmi
Copy link
Author

@pstibrany have you also encountered such a problem? can you describe it? thank you!

@efremovmi
Copy link
Author

efremovmi commented Aug 30, 2023

@easwars this is where the buffer pool is passed, from which the buffer is taken, and then nothing is returned back to the pool (Put).
image

@efremovmi
Copy link
Author

I can assume that after calling this function, you can return the buffer back to the pool
image

@hueypark
Copy link
Contributor

@easwars

@hueypark : Do you see any issues at all with the implementation? I remember that you wanted this functionality quite urgently. Have you been using this buffer pool with your application? How has your experience been? Thanks.

I had already solved the memory problem by reducing the number of RPC calls, so I hadn't introduced a buffer pool yet.
And when I was developing this PR, I was mainly thinking about stream RPCs, so it might not work well with unary calls.

+1 for #5862 (comment)

@efremovmi
Copy link
Author

@hueypark

...I was mainly thinking about stream RPCs, so it might not work well with unary calls.

you can add your own pool that will permanently hold memory, so it will still work well, unlike sync.Pool. The problem is that now nothing is returned back to the pool with unary calls, regardless of which pool it is: SharedBufferPool (provided by developers) or self-written

@hueypark
Copy link
Contributor

@efremovmi

you can add your own pool that will permanently hold memory, so it will still work well, unlike sync.Pool. The problem is that now nothing is returned back to the pool with unary calls, regardless of which pool it is: SharedBufferPool (provided by developers) or self-written

I agree. Unfortunately, I won't be able to allocate time for this improvement in the immediate future.
Would you consider creating a pull request based on your current findings?

@efremovmi
Copy link
Author

@hueypark

I agree. Unfortunately, I won't be able to allocate time for this improvement in the immediate future.
Would you consider creating a pull request based on your current findings?

Yes, I will try to do it.

@efremovmi
Copy link
Author

efremovmi commented Sep 4, 2023

Added a change:
#6605

@ginayeh
Copy link
Contributor

ginayeh commented Sep 19, 2023

Seems like #6605 might be closed soon due to inactivity. @hueypark Are you interested in taking this up since you made the initial PR if you have some bandwidth?

@hueypark
Copy link
Contributor

Sure, I'll take a look and pick it up.

@vtolstov
Copy link

any news ? i want to minimise memory allocs, but i have unary and stream rpc

@dfawley
Copy link
Member

dfawley commented Feb 16, 2024

I approved #6766; we need a second review before merging it. Sorry for the delays.

@efremovmi
Copy link
Author

efremovmi commented Apr 4, 2024

Hi! Have you noticed any changes in memory consumption after merging with the main branch?

@dfawley
Copy link
Member

dfawley commented Apr 4, 2024

@efremovmi The gRPC benchmarks (https://grafana-dot-grpc-testing.appspot.com/?orgId=1) unfortunately don't collect memory usage statistics. Maybe another user has some data they can share?

@efremovmi
Copy link
Author

In this case, I will try to create a load test for my pet project in my free time and come back with the results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants