benchmark: Add sleepBetweenRPCs and connections parameters #6299
Conversation
s-matyukevich
changed the title
benchmark/benchmain/main.go
Outdated
| if maxSleep > 0 { | ||
| time.Sleep(time.Duration(math_rand.Intn(maxSleep))) | ||
| } |
There was a problem hiding this comment.
Is this client code or server code, or is this used for both sides?
I think we might want the sender in both directions to be sleeping, and not sleeping after receiving. If the sender isn't also sleeping, it could fill up buffers and have more impact than you intended.
There was a problem hiding this comment.
Yes, you are right. I'll remove the delay from the client receive goroutine and instead will add it here
In order to do that I'll need to pass the sleepBetweenRPCs parameter to the server (probably I'll add ti to SimpleRequest rpc)
There was a problem hiding this comment.
Fixed. I end up using metadata to pass the sleep duration to the server. SimpleRequest protobuf is used in too many places across languages, so I am not sure if adding fields to it is a good idea.
|
@dfawley I fixed your comment, but I also discovered 2 more issues:
|
s-matyukevich
changed the title
s-matyukevich
changed the title
I believe this is by design. More allocations = slower and more frequent GC cycles = worse expected performance. |
benchmark/benchmain/main.go
Outdated
| @@ -111,6 +111,7 @@ var ( | |||
| serverReadBufferSize = flags.IntSlice("serverReadBufferSize", []int{-1}, "Configures the server read buffer size in bytes. If negative, use the default - may be a a comma-separated list") | |||
| serverWriteBufferSize = flags.IntSlice("serverWriteBufferSize", []int{-1}, "Configures the server write buffer size in bytes. If negative, use the default - may be a a comma-separated list") | |||
| sleepBetweenRPCs = flags.DurationSlice("sleepBetweenRPCs", []time.Duration{0}, "Configures the maximum amount of time the client should sleep between consecutive RPCs - may be a a comma-separated list") | |||
| shareConnection = flag.Bool("shareConnection", true, "Controls whether a single connection or connection per `maxConcurrentCalls` should be used.") | |||
There was a problem hiding this comment.
Instead of this, how about a flag for the number of connections, where each connection does maxConcurrentCalls at a time? Hopefully that wouldn't be too different implementation-wise, and would allow for some more interesting testing options.
|
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) :). |
I don't have permissions to merge - please do it for me. |
zasweq
changed the title
|
Done :). |
I am trying to fix #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