-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add discardResponseMessage
option for gRPC client
#3820
Add discardResponseMessage
option for gRPC client
#3820
Conversation
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.
Hi @lzakharov,
sorry for the late review. Thanks for your contribution 🙇
Did you run the profiler to make sure it reduces the memory as expected?
After checking a bit the gRPC library, I assume that we are not discarding the response body as we do in HTTP.
With the suggested code, we are mostly only skipping the JSON parsing. That might lead to not being very useful in reducing memory usage. It might be more impactful on the CPU.
Hi, thanks for your review comment! By reducing memory consumption, I meant the absence of the need to allocate objects when parsing into JSON. I don't yet know how to fully ignore a message at the gRPC library side, need to do some research. My main focus was on CPU consumption, since this is the problem we encountered in our tests. |
After some research, I discovered that emptypb.Empty can be used as a response message type to reduce memory consumption. It still requires reading all response data into the bytes buffer here, but I haven't found how to optimize this. Anyway, the current solution shows a noticeable improvement in performance. To compare performance, I modified the gRPC server from the testutils package to include an RPC method that returns a list of features for a specified rectangle (with the same logic as in Without the
With the
|
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.
Hey @lzakharov,
the results are quite interesting! Thanks for taking care of it.
If we can find an alternative solution to the current global variable then we are fine with merging your proposal. 👍
It would be helpful if you could do something similar also for gRPC streaming. You can split it in a second pull request.
I'll try to add it in the next PR. |
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.
LGTM
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.
LGTM!
Thanks for the contribution @lzakharov 🙇
What?
Adds a new gRPC client option
discardResponseMessage
, which is equivalent todiscardResponseBodies
.Why?
Parsing heavy responses can result in high CPU and memory usage.
Checklist
make lint
) and all checks pass.make tests
) and all tests pass.Related PR(s)/Issue(s)
#2497