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]

The committers listed above are authorized under a signed CLA.

• ✅ login: aymanm-google / name: Mohannad Farrag (b79ffbe)

[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 #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 #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.

[aymanm-google]

So I'm reviving this thread again. I've applied the recommended approach here: #12199 (comment) where there's an optionKey that the user can apply to change the per-stream buffer size.

Let me know if this is okay or there's more changes needed.

[aymanm-google]

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.

Buffering across streams will be really complicated and it is not really clear whether it would have a significant improvement or not. One possible improvement here is instead of creating a new buffer every single time to read, we could potentially reuse the same buffer passed to Cronet. This is fine since this is a behaviour that we've maintained in Cronet where we pass the same buffer which the user passed to us after filling it with data.

[aymanm-google]

Friendly ping for review!

[ejona86]

Whoops, sorry. I missed the "Add CRONET_READ_BUFFER_SIZE_KEY API to CronetClientStream" addition.

The change right now looks fine, except there's no way for it to actually be used. Should be good after that is resolved.

[ejona86]

Note that this is not accessible to others, because it is in a package-private class. We want this class to be package-private. If we want this API for internal-only (at least for now), then you can add accessor methods to InternalCronetCallOptions.