grpc-go
grpc-go copied to clipboard
Add sleepBetweenRPCs and connections parameters to benchmarks
I am trying to fix https://github.com/grpc/grpc-go/issues/5759 and have a branch with some promising results. However, the problem is that our current benchmarks don't really test the case I am trying to address. The case is the following: we have some grpc servers with a lot of mostly idle connections (in our case those connections mostly come from healtchecks) In this case grpc-go wastes a lot of memory on transport buffers, that aren't shared between connections.
This PR modifies benchmarks so that we can simulate this case. It adds a random delay between subsequent RPCs and a configuration parameter which controls the maximum value of this delay. It also adds a parameter that allows configuring the number of connections.
RELEASE NOTES: none
@dfawley I fixed your comment, but I also discovered 2 more issues:
- Right now benchmarks create a single connection between client and server, which makes it impossible to reproduce the case I described in the PR description. I added one more parameter which allows to use a separate connection per goroutine. Without this change my original PR is completely useless, but let me know if I should split those 2 changes into separate PRs.
- benchmarks memory reporting is actually not very useful. We compare total alocations before and after benchmark run and divide them by the number of RPC. This includes allocation that are actually garbage collected and this doesn't give you an idea how much memory your benchamrk is actually using. IMO in order to measure the actual memory usage we need to sample MemStats.Alloc during program execution with some relatively high frequency and then take the average (which is something that pprof does) For now I am just using pprof to analyze memory usage, but let me know if we should add some useful memory-usage indicator to the benchmark output.
This includes allocation that are actually garbage collected
I believe this is by design. More allocations = slower and more frequent GC cycles = worse expected performance.
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.
I think I answered all questions and implemented all suggestions, what other updates are expected?
We need a second review before merging PRs from external contributors.
Awesome, thanks for getting to my comments. Feel free to merge (which I think unblocks your other issue) :).
Awesome, thanks for getting to my comments. Feel free to merge (which I think unblocks your other issue) :).
I don't have permissions to merge - please do it for me.
Done :).