Avoid flushing the stream after writing a message in case the server returns a single response

[amirhadadi]

Resolves #4884

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: amirhadadi (b4658bb)

[ejona86]

Have you confirmed whether this makes a difference with a packet dump?

Digging into the code, it seems this could improve things, but only because we already have this optimization later on in the write path:

grpc-java/core/src/main/java/io/grpc/internal/AbstractServerStream.java

Lines 109 to 111 in 428b541

	// Since endOfStream is triggered by the sending of trailers, avoid flush here and just flush
	// after the trailers.
	abstractServerStreamSink().writeFrame(frame, endOfStream ? false : flush, numMessages);

[ejona86]

Yeah, I think this just may work. This was what I thought was the hard part! But the plumbing in the transport already has a solution!

Delaying the flush for the initial response should be relatively easy and similar to client-side. We just need to use false instead of true for the flush argument here:

grpc-java/netty/src/main/java/io/grpc/netty/NettyServerStream.java

Lines 99 to 103 in 428b541

	writeQueue.enqueue(
	SendResponseHeadersCommand.createHeaders(
	transportState(),
	Utils.convertServerHeaders(headers)),
	true);

(That's fine as a separate PR)

[amirhadadi]

Have you confirmed whether this makes a difference with a packet dump?

It seems to create the same number of packets. Maybe if the message body + trailers were aggregated into a single netty object before flushing it would make it a single packet? I don't have enough Netty know how to tell.
However, it does eliminate one flush and ends up with a single system call for writing out the data instead of 2.

[amirhadadi]

Delaying the flush for the initial response should be relatively easy and similar to client-side. We just need to use false instead of true for the flush argument here:

Would that require the serverSendsOneMessage check?
If it does, I guess we need to change this argument to false and move the decision whether to flush over here?

[ejona86]

Would that require the serverSendsOneMessage check?

Yes, it would.

If it does, I guess we need to change this argument to false and move the decision whether to flush over here?

I thought we'd just do it in io.grpc.netty, not in the ServerCall. But I see now, NettyServerStream doesn't have access to the MethodDescriptor like NettyClientStream does. That's because it is determined later, after the call starts. We could either pass the method descriptor down into the server stream using a new method, or plumb the decision down in writeHeaders(). It makes sense to me to pass the flush decision as an argument to writeHeaders(). (That'd differ from client-side, but really, we could do the same thing on client-side and it wouldn't be bad.)