server: use least-requests loadbalancer for workers

[SaveTheRbtz]

This is a proposal to change load-balancing mechanism from round robin to least-requests by switching an array of channels to a single channel. The downside of the existing code is that round-robin may pick a worker that is already busy while there are plenty of workers free.

In the future we could also add a customizable queue discipline for the cases where all workers are busy so that it can act as a cheap circuit breaker / load-shedding mechanism.

While here, I'm also switching wg.Done() to be consistently ran in a defer.

Ref: #3204

RELEASE NOTES:

• server: improve stream handler goroutine worker allocation when NumStreamWorkers is used

[SaveTheRbtz]

cc: @adtac @menghanl @dfawley

[zasweq]

This is marked as draft. I'll wait until it gets unmarked as draft until I take a look.

[dfawley]

Thank you for the PR. This seems like the obvious way to do this, which makes me wonder why it was done the way it was before.

Is it possible for you to run the benchmarks and see what happens to performance when this is enabled before/after this change?

[github-actions]

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.

[SaveTheRbtz]

Sorry for the slow reply!

None of the existing benchmarks show any difference, mostly because they do not exercise the affected codepath. I've created a very dirty benchmark specifically for this[1] but even it does not show much difference in go1.20: the only difference i was able to spot is that now requests are more likely to be handled by workers instead of one-off goroutines.

This is likely do to the fact that go right now automatically adjust stack size based on historic data, which means even if request is not processed in worker, it won't be pathologically slow.

[1] https://gist.github.com/SaveTheRbtz/cae6e8abb9311e2016acbab6bc63752a

[github-actions]

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.

[easwars]

Does it at all make sense to create this channel with a buffer of size numServerWorkers?

[SaveTheRbtz]

This would increase latencies, since then up to numServerWorkers requests can be indefinitely queued until we free up a worker. Without buffering such a request would instead be handled by a spawned goroutine immediately.