Propagate explicit flushes through MessageFramer #323
Conversation
| if (frame.readableBytes() > 0) { | ||
| sendFrame(frame, false); | ||
| sendFrame(frame, false, endOfStream ? false : flush); |
There was a problem hiding this comment.
why in the case of endOfStream do you not flush?
There was a problem hiding this comment.
Yes, trailers. We know on line 149 here that we will send trailers.
|
Based on discussion with @louiscryan, I think there is room for both of these changes, since with #313 we likely will cap out the maximum buffer size we use in order to ensure we always use the buffer pool. The main thing I am hesitant on is whether we want MessageFramer to do write a WritableBuffer[] instead of just a single WritableBuffer. For performance, I see both Netty and OkHttp wanting to buffer frames until they receive the flush (for less synchronization). It would make sense to do that in one place (MessageFramer), however both Netty and OkHttp may want to do that buffering for headers as well, and so buffering in MessageFramer wouldn't be enough. |
|
Sounds good. LGTM as-is. The change to using |
MessageFramer allows queing of data and explicit flushing. Sinks generally can benefit from knowing when they are required to flush, so we now tell them when MessageFramer received a flush so they only have to flush when required.
ejona86
force-pushed
the
flush-out-of-framer
branch
from
cc9ea96
to
fc3e416
Compare
MessageFramer allows queing of data and explicit flushing. Sinks
generally can benefit from knowing when they are required to flush, so
we now tell them when MessageFramer received a flush so they only have
to flush when required.
This is an alternative to #313 which when combined with netty/netty#3670
would greatly decrease the number of TCP segments we send. The same
approach of this CL could be taken with headers to delay them if the request
is unary (which I think is already being done in OkHttp).
Note that this CL doesn't get all the gains we could. We currently do thread
synchronization in both OkHttp and Netty per-write. To optimize that we could
buffer in the sync until the flush arrives. This would allow combining header
writing into the same buffer. Alternatively, instead of the approach of this PR,
we could write batches of frames to the MessageFramer.Sink.
Note that all approaches discussed here aren't impacted by number of
messages being sent.