Optimize buffer usage in MessageFramer

[ejona86]

We currently allocate a large non-direct buffer per stream due to MessageFramer, only to copy immediately out of it. We should instead write directly to the transport-native buffer, which will cause us to have a WriteableBuffer or some such like our current Buffer class that is ready-only.

[ejona86]

Rough design: introduce WritableBuffer (likely renaming current Buffer to ReadableBuffer) that only allows writes. MessageFramer writes to that buffer, and uses a provided buffer allocator to get instances. The provided allocator would generally use the transport's "native" buffer pool.

Currently the MessageFramer re-uses the buffer, but this shouldn't happen in the future. It only works today because the various transports make a copy of the buffer, which is what this feature would be trying to prevent. Instead, it should use the allocator provided and use a new buffer each time. In the case that a transport does want to reuse, then it can have a simple allocator that "pools" a single buffer instance.

Using a new buffer each time means that MessageFramer won't need to be closed when the Call is closed, which turns out to be quite helpful from a threading perspective since MessaegFramer is currently run within the application thread and so we can't clean it explicitly without making it thread-safe.

[buchgr]

Using a new buffer each time means that MessageFramer won't need to be closed when the Call is closed, which turns out to be quite helpful from a threading perspective since MessaegFramer is currently run within the application thread and so we can't clean it explicitly without making it thread-safe.

@ejona86 I am currently working on this and I was wondering how you thought piggy-backing the END_STREAM flag would work without close/dispose and a new buffer on every write?

@louiscryan

[ejona86]

I think it is reasonable to assume a flush/close/dispose will always be coming.

[buchgr]

so we still need the flush/close/dispose on the MessageFramer to behave the same as they did until know, correct?

I don't understand the part of "that MessageFramer won't need to be closed when the Call is closed". Could you please clarify this? Thanks @ejona86

[ejona86]

Basic flow (but, of course, with framing mixed in):

void write(blah) {
  while (blah.moreToWrite()) {
    if (buffer != null && buffer.full()) flush();
    if (buffer == null) buffer = alloc();
    writeAsMuchAsPossible(buffer, blah);
  }
}

void flush() {
  if (buffer != null) {
    send(buffer);
    buffer = null;
  }
}

In that code if the message is precisely the size of the buffer then it isn't flushed. That is more "optimal" but doesn't really matter. I wrote it like that here since I think that is what the current code is doing.

[buchgr]

@ejona86 ahh got ya. yeah that's what I thought. thank you! :-).

[ejona86]

On "idle" there won't be a buffer. When a message is sent, a flush/close must be coming basically immediately after. So dispose() would only be necessary in case of exception or something. We wouldn't need to call dispose() to clean up when the stream is torn down (which I can't remember if we are doing now, since the current buffer is on the heap)

[buchgr]

@ejona86 I was just thinking. What's the point of having a WritableBuffer and ReadableBuffer, why not simply extend the existing buffer with a write method? Basically all we want to do is abstract from Netty's ByteBuf and OkHttp's Buffer.

[ejona86]

We could, and considered it. The main problem is that the API becomes a lot more interesting when you can read and write at the same time. Consider what be necessary to use JDK's ByteBuffer in such a circumstance.

All very doable, but it seemed keeping them separate was no more work, simpler, and had no real deficiencies.