server.go: use worker goroutines for fewer stack allocations

[adtac]

Currently (go1.13.4), the default stack size for newly spawned
goroutines is 2048 bytes. This is insufficient when processing gRPC
requests as the we often require more than 4 KiB stacks. This causes the
Go runtime to call runtime.morestack at least twice per RPC, which
causes performance to suffer needlessly as stack reallocations require
all sorts of internal work such as changing pointers to point to new
addresses.

Since this stack growth is guaranteed to happen at least twice per RPC,
reusing goroutines gives us two wins:

1. The stack is already grown to 8 KiB after the first RPC, so
   subsequent RPCs do not call runtime.morestack.
2. We eliminate the need to spawn a new goroutine for each request
   (even though they're relatively inexpensive).

Performance improves across the board. The improvement is especially
visible in small, unary requests as the overhead of stack reallocation
is higher, percentage-wise. QPS is up anywhere between 3% and 5%
depending on the number of concurrent RPC requests in flight. Latency is
down ~3%. There is even a 1% decrease in memory footprint in some cases,
though that is an unintended, but happy coincidence.

unary-networkMode_none-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_8-reqSize_1B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps      2613512      2701705     3.37%
             SendOps            0            0      NaN%
             RecvOps            0            0      NaN%
            Bytes/op      8657.00      8654.17    -0.03%
           Allocs/op       173.37       173.28     0.00%
             ReqT/op    348468.27    360227.33     3.37%
            RespT/op    348468.27    360227.33     3.37%
            50th-Lat    174.601µs    167.378µs    -4.14%
            90th-Lat    233.132µs    229.087µs    -1.74%
            99th-Lat     438.98µs    441.857µs     0.66%
             Avg-Lat    183.263µs     177.26µs    -3.28%

[adtac]

Unary RPCs (1-byte req/resp). Similar, but a less positive impact with larger payloads / streaming RPCs.

[dfawley]

Per offline discussions:

• Please add a ServerOption to configure this value, disabled by default
• Benchmark the difference between using % instead of &.
• Use streamID >> 1 instead of streamID since the client always uses odd stream IDs.
• Run benchmarks to compare this approach with the more obvious one that uses a single channel with multiple listeners and report the results.

[adtac]

Please add a ServerOption to configure this value, disabled by default

Done.

Benchmark the difference between using % instead of &.

Turns out to be insignificant. Also, since we're now allowing the user to set the number of stream workers, enforcing it to be an exponent of two is probably not a good idea. Switched to modulo.

Use streamID >> 1 instead of streamID since the client always uses odd stream IDs.

I just realised that there's a slight issue with using stream ID -- it's biased towards lower numbers. If a server receives one unary request from several clients (a common workload), they'll all go to the same channel because the first stream ID is always 0.

Switched to a round-robin method that's actually fairer and 1% faster (~compared to master, round robin is a 5.33% improvement while stream ID is a ~4.54% improvement).

Run benchmarks to compare this approach with the more obvious one that uses a single channel with multiple listeners and report the results.

See previous comment.

Pushed V2.

[dfawley]

After these changes, can you re-run some benchmarks and provide the results here? Thanks!

[dfawley]

Brainstorming: should this be time-based instead of request-based? Or some combination of both?

If a server goes idle, we may want to restart the threads.

[adtac]

Possibly yes, but I expect the overhead introduced by that to be non-negligible. Also heavily depends on how Go's runtime manages shrinking stacks during GC pauses, I think.

[dfawley]

I think we could do it with a Timer checked in the same select as the workload without being too expensive. But this is fine for now - we can work on this more in the future.

[CAFxX]

a late comment just to point out that the runtime shrinks the stacks of goroutines during GC (or at the next safepoint), so this may be not needed at all (and potentially is counter-productive): https://github.com/golang/go/blob/9d812cfa5cbb1f573d61c452c864072270526753/src/runtime/mgcmark.go#L781-L783

Dropping all this part would have the benefit of making it much easier to implement adaptive workers (discussed below)

[dfawley]

(Still interested in seeing some new results after the latest changes, otherwise LGTM.)

[adtac]

ok this is weird, I'm now getting a 15% perf improvement -- why's there a sudden jump at 16? I think it's because I was using 16 worker goroutines.

[CAFxX]

this should have been

defer wg.Done()
s.handleStream(st, stream, s.traceInfo(st, stream))

as it is on line 809 and as it was before the change

[CAFxX]

this is potentially pretty suboptimal in case some workers are busy on requests that take significant time

[CAFxX]

I think we should add a worker in this case, not a one-off goroutine.

Extra workers created over numServerWorkers should linger for a short period of time waiting for new work once they're done, and then shut down if no new work arrives. This would in turn enable to remove the numServerWorkers knob, as new workers will be created as needed.