Proposal to set max threads on a sync server

[Sree Kuchibhotla]

https://github.com/grpc/proposal/pull/84

[Christopher Warrington - MSFT]

> In the initial implementation, I plan to divide the max_threads equally
> among multiple `ThreadManager` objects (there are as many `ThreadManager`
> objects as the number of server completion queues). It is not clear on
> which strategy is better at this point
> - (1) Have all thread managers create threads from a common pool (but
>       potentially starving some thread managers and also making the
>       quota-check a potential global contention point)

> * In the initial implementation, I plan to divide the max_threads equally
>   among multiple `ThreadManager` objects (there are as many
>   `ThreadManager` objects as the number of server completion queues). It
>   is not clear on which strategy is better at this point
>   - (1) Have all thread managers create threads from a common pool (but
>         potentially starving some thread managers and also making the
>         quota-check a potential global contention point)
>   - (2) Divide the max_threads equally among thread managers (with the
>         downside that some thread managers are "over provisioned" while
>         some might be "under provisioned").

Thread creation/destruction seems like it is a rare enough occurrence that having one
shared atomically accessed value will work just fine. There shouldn't be any
contention, and when there is contention the delay of just creating a thread
is going to dwarf any waiting time to be negligible.

Thus, proposal 1 seems both simpler and more robust.

Continuing from a [GitHub discussion][1]:

CW>> Is 0 a valid value for max threads? Is that how I say "I will never use
CW>> this server synchronously. Do not create any threads. I'll be polling
CW>> completion queues myself"?

SK> 0 is a valid value but that will effectively make the server not respond
SK> to any incoming requests. So it is not a good idea to initialize
SK> max_threads to 0. However, if for whatever reason, the application
SK> suddenly decides to stop accepting incoming requests for sometime, it
SK> can set the max_threads to 0.
SK> <snip>
SK> No, actually the server completion queues used by the sync server are
SK> not exposed to the application. So there is no question of the
SK> application trying to poll the queues itself. If application wants to
SK> poll itself, it has to create its own completion queues an poll them ..

Makes sense.

To come at this from a different perspective: in our framework, we *never*
create sync servers--only async onces--and we want to manage the threads
ourselves (so we can share them among multiple, related components). We
would like a way to tell gRPC "DO NOT CREATE ANY THREADS. I will make sure
you have some you can borrow. If I don't my throughput will tank, and that
will be my fault."

I'm reading your response as max_threads = 0 won't help with this and it
won't hurt either.

[1]: https://github.com/grpc/proposal/pull/84#discussion_r193767559

--
Christopher Warrington
Microsoft Corp.

[Sree Kuchibhotla]

Thanks for the suggestions..

>>Thread creation/destruction seems like it is a rare enough occurrence that having one

shared atomically accessed value will work just fine. There shouldn't be any
contention, and when there is contention the delay of just creating a thread
is going to dwarf any waiting time to be negligible.

You make a good point. I'll go with option (1) then.  I changed the proposal to reflect this.

>>> To come at this from a different perspective: in our framework, we *never*
create sync servers--only async onces--and we want to manage the threads
ourselves (so we can share them among multiple, related components). We
would like a way to tell gRPC "DO NOT CREATE ANY THREADS. I will make sure
you have some you can borrow. If I don't my throughput will tank, and that
will be my fault."

I'm reading your response as max_threads = 0 won't help with this and it
won't hurt either.

Yes you are right. Since you are using async servers, the max_threads won't have any effect on this. 

thanks,

Sree

[Vijay Pai]

Right, with async-only servers, this can't have any impact since there are no threads created for such servers in the C++ API implementation. That said, there are still threads created in core for timer management, and some other off-critical-path execution events.

[Sree Kuchibhotla]

I made small change to the RFC. I am changing the default value of max_threads to 1500 instead of INT_MAX.

I feel its a good idea to enforce some limit by default instead of leaving this unbounded. The number 1500 came from some earlier experiment I did when debugging thread-avalanche issue https://github.com/grpc/grpc/pull/12139 in the past and found that on my machine with 32G RAM and 12 cores, things started getting worse at ~1500 threads.

I am happy to take other recommendations for the default value :)

-Sree