core: don't wrap sequential executors in ClientCallImpl and ServerImpl

[njhill]

If a blocking ThreadlessExecutor is being used, or the user-provided executor is a sequential executor created via guava's MoreExecutors.newSequentialExecutor(), it doesn't need to be wrapped in a SerializingExecutor in ClientCallImpl and ServerImpl.

In-process TransportBenchmark shows speedup of 5-10% (this is with stats/tracing disabled). Note that both the direct and sequential parameters only actually affect the server-side since ThreadlessExecutor is used for blocking calls on client side regardless.

Benchmark                         (direct)  (sequential)  (transport)  (wrapSerialized)  Mode  Cnt      Score     Error  Units
TransportBenchmark.unaryCall1024      true         false    INPROCESS              true  avgt   40   1805.940 ±  40.523  ns/op
TransportBenchmark.unaryCall1024      true         false    INPROCESS             false  avgt   40   1619.765 ±  33.452  ns/op
TransportBenchmark.unaryCall1024     false          true    INPROCESS              true  avgt   40  12783.736 ± 297.523  ns/op
TransportBenchmark.unaryCall1024     false          true    INPROCESS             false  avgt   40  12174.038 ± 342.385  ns/op
TransportBenchmark.unaryCall1024     false         false    INPROCESS              true  avgt   40  12458.629 ± 220.206  ns/op
TransportBenchmark.unaryCall1024     false         false    INPROCESS             false  avgt   40  11709.750 ± 554.520  ns/op

It would be awesome if we could also include appropriate netty executor classes in the list though I guess that might be considered more "dangerous" since those generally aren't final.

This is kind of a second swing at #3914 :)

[njhill]

This is here to make the benchmark comparisons easier, can be tidied up if this is accepted.

[ejona86]

Using a sequential executor on server-side seems a bit bizarre, as that limits you to one thread for all RPCs. And if anything I'd see it as being more likely to be a single-thread thread pool. Sequential executor on client-side is not harmful, but doesn't seem to do much and thus would be rare.

Avoiding the extra wrapping for Threadless would be a win. We had realized that earlier, but hadn't wanted to special-case it. The wins though are nice, so it is probably worth it.

SynchronizationContext would be a rarer case, although it can show up in OobChannel for Load Balancers.

[ejona86]

drive-by nit: wrapSerialized is guaranteed to be false here

[ejona86]

drive-by nit: Instead of the cast, use asSubclass(Executor.class)

[ejona86]

For Netty, any EventLoop is probably safe. There is some risk that Netty code somewhere does if (inEventLoop()) r.run() "optimization" to run the Runnable immediately. That would break us. While that sort of logic is common in Netty, it seems EventLoops are still likely safe.

Unfortunately, Netty would also need to be handled by reflection. On the plus side, it seems there would be no benefit to checking for the shaded Netty in grpc-netty-shaded.

I guess the only time Netty EventLoops will be provided is for clients. I guess we could also detect EventLoopGroup and call next() on it to get an EventLoop. That would work for servers, although it's a bit unclear if directExecutor() would be a better approach.

I'm not sure I want to go down the rabbit hole optimizing for Netty event loops at this point.

[njhill]

Thanks @ejona86, have addressed your code comments.

Using a sequential executor on server-side seems a bit bizarre, as that limits you to one thread for all RPCs.

Agree on server side it would be kind of unusual/niche - possibly when some kind of up-front processing has to be done in a serialized manner based strictly on arrival order.

And if anything I'd see it as being more likely to be a single-thread thread pool.

Also agree ... it's maybe unfortunate that we can't reliably detect single-thread thread pools, for example those returned from Executors.newSingleThreadExecutor().

Sequential executor on client-side is not harmful, but doesn't seem to do much and thus would be rare.

I don't quite follow - do you mean specifically guava's or executors with sequential properties in general? I don't think it would be that uncommon for requests to be made from "event loops" which have the required properties. These wouldn't necessarily be based on guava's sequential executor but supporting this would at least give consuming libraries/apps the option of changing to use that to avoid the additional intermediate executor overhead - my own case matches this which is what motivated this PR.

SynchronizationContext would be a rarer case

The thought was to cover known executor types which fit the requirements, even if it would be rare for them to show up.

For Netty, any EventLoop is probably safe. There is some risk that Netty code somewhere does if (inEventLoop()) r.run() "optimization" to run the Runnable immediately. That would break us. While that sort of logic is common in Netty, it seems EventLoops are still likely safe.

The characteristics of EventLoops which makes them safe exactly match the OrderedEventExecutor superinterface, right? Which asserts that all submitted tasks will be processed in "an ordered / serial fashion". The "ordered" part of that should rule out the kind of "optimization" you're referring to from being used within the executor itself. I know netty does this in a bunch of other places, but that's code above the executor which presumably understands the implications and knows that it doesn't matter if the task to be submitted at that point is instead "subsumed" into the task already being run, even if there are others already outstanding.

I'm not sure I want to go down the rabbit hole optimizing for Netty event loops at this point.

Fair enough, as implied in the original description this was more kind of wishful thinking! Impl-wise though it would be just adding another class to the list of executor types (using reflection like you say, as is done for others).

[dapengzhang0]

@ejona86 Can you clarify?

I don't quite follow - do you mean specifically guava's or executors with sequential properties in general?

[sanjaypujare]

@njhill do you still want to pursue this PR?