Add `setReadbufferSize` API to CronetChannelBuilder

[aymanm-google]

By default, CronetClientStreams would use a 4KB buffer to read data from Cronet. This can be inefficient especially if the amount of data being read is huge (~MBs) as each read callback operation incur overhead from Cronet itself (e.g. Context switch, JNI calls). The alternative would be to immediately bump the default to a bigger number but that would incur an increase in memory usage.

If the API is not called then the default of 4KB is used.

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: aymanm-google (561b86c9b015567d69ecef4c193d43e98e047126)

[ejona86]

I think this is a bit more fundamental and we need to think about it some. I agree there's a problem here to be solved, but I'm not all that confident that setting a read buffer size is what we want. In some ways I wouldn't mind adding this, but we would want to be able to remove it in the future.

I'm assuming the latency here is between the application and network thread. There had been some work on MigratingDeframer. I wonder if it helps this problem. (If you revert https://github.com/grpc/grpc-java/pull/7198 you might be able to see if the performance improves.)

[aymanm-google]

So I understand that adding a new API is a little bit difficult here. However, this API is purely for performance reasons, which means that it can be deprecated at any time and it would not affect the correctness. If we conclude that the difference in performance was not that reasonable then we can just put a @Deprecated above it and say that the default is still 4KB?

I'm assuming the latency here is between the application and network thread. There had been some work on MigratingDeframer. I wonder if it helps this problem. (If you revert https://github.com/grpc/grpc-java/pull/7198 you might be able to see if the performance improves.)

I don't understand how this is related? The current performance overhead is between the application and Cronet's network thread which gRPC does not have control over.

[ejona86]

The current performance overhead is between the application and Cronet's network thread which gRPC does not have control over.

I wasn't convinced that was the full problem. I see now that gRPC is starting the next read() directly from the onReadCompleted() callback.

gRPC does know how many bytes are remaining for the current message, but we avoid using it to avoid penalizing streaming. So something along the lines of this PR does seem necessary.

Should this setting be per-stream instead of per-channel? That can be communicated via CallOptions, like done with CRONET_ANNOTATIONS_KEY. The caller then uses stub.withOption(THE_KEY, 4096) to configure it on the stub. So they can still use the same setting for multiple RPCs.

[groakley]

Should this setting be per-stream instead of per-channel?

I agree that a per-stream setting would make sense. A single service may have one method that expects particularly large message sizes, but others that have more conventional payload sizes where the extra buffer space would be wasteful.

Configuring this with a CallOption doesn't prevent setting this once for a channel if that's the desired behavior. You could wrap the channel with a ClientInterceptor that sets the option for all methods and avoid setting it on each stub.

My concern with the CallOption approach would be if it prevents the Cronet channel from pooling these buffers across streams. But from what I can tell looking through CronetClientStream, there is no buffer pooling today; each new stream allocates a new buffer anyways.