grpc-kotlin icon indicating copy to clipboard operation
grpc-kotlin copied to clipboard

Provide guidance on Channel executors

Open bubenheimer opened this issue 3 years ago • 7 comments

There does not seem much guidance available regarding Channel executors on client & server.

The Android sample project uses executor(Dispatchers.Default.asExecutor()) which makes some sense, but there is no code comment why this may be a good choice. Personally I would have thought that directExecutor() makes a little more sense: executor(...) explicitly changes the executor prior to launching coroutines machinery that ends up running on the same set of threads as the executor in this case. directExecutor() skips the explicit executor change and relies on coroutines machinery to do the right thing, implicitly ending up on the same set of threads as the first approach. The second approach looks slightly lighter to me.

There is no executor set explicitly in other client samples or server samples. I would have expected directExecutor() as the desired default in all these cases, to avoid briefly switching to a barely used thread. Or does grpc-kotlin set an explicit executor behind the scenes?

bubenheimer avatar May 25 '21 21:05 bubenheimer

Coroutines aren't the only thing running on the executor, but also the underlying gRPC network infrastructure written in Java. I don't believe directExecutor is generally appropriate for that -- blocking the network thread seems highly undesirable.

lowasser avatar May 25 '21 22:05 lowasser

Do you mean the grpc-java network infrastructure or grpc-kotlin infrastructure written in Java? Does grpc-kotlin request/response handling block the threads it runs on? I assumed it would generally launch/async off its threads, ending up on Dispatchers.Default by default for its internal Channel-based/Flow-based operations, and suspending application code.

bubenheimer avatar May 25 '21 22:05 bubenheimer

I mean the grpc-java network infrastructure. grpc-kotlin doesn't define its own network infrastructure.

Hmm. It does look like it's only the listener methods which get called under that executor (https://stackoverflow.com/a/42422045/869736), and those methods should be generally very cheap for gRPC-Kotlin. I'm not sure if I'd go so far as to say Dispatchers.Default is a bad idea, but directExecutor seems like it ought to work all right.

lowasser avatar May 25 '21 22:05 lowasser

After looking at the grpc-kotlin code again I'm pretty sure that directExecutor() is essentially the one and only valid executor pattern (for pure grpc-kotlin projects), everything else is just less efficient with no benefit. Both client and server listeners do nothing but transfer messages to the internal Channel buffer in a non-blocking way.

bubenheimer avatar May 26 '21 03:05 bubenheimer

I suppose a blocking or slow Interceptor could throw things off with directExecutor(). Not the greatest Interceptor I'd think.

bubenheimer avatar May 26 '21 05:05 bubenheimer

@bubenheimer have you had a chance to compare the performance of various executors in any of your projects? Curious to see if directExecutor() worked for you

jeffzoch avatar Aug 11 '21 21:08 jeffzoch

It does work, but I have not done extensive testing. I do think it's the one to use, due to better performance and no real disadvantages with grpc-kotlin that I can see. You can find more info about directExecutor in the grpc-java project

Sent from my phone

On Wed, Aug 11, 2021, 16:31 Jeff Zoch @.***> wrote:

@bubenheimer https://github.com/bubenheimer have you had a chance to compare the performance of various executors in any of your projects? Curious to see if directExecutor() worked for you

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/grpc/grpc-kotlin/issues/263#issuecomment-897168582, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFIE532D2EIULFVEBAGUU3T4LT3JANCNFSM45QHLJEQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

bubenheimer avatar Aug 12 '21 01:08 bubenheimer