optimize number of buffer allocations #11879
Conversation
| @@ -27,9 +27,6 @@ | |||
| */ | |||
| class OkHttpWritableBufferAllocator implements WritableBufferAllocator { | |||
|
|
|||
| // Use 4k as our minimum buffer size. | |||
| private static final int MIN_BUFFER = 4096; | |||
There was a problem hiding this comment.
Okio Buffer works quite differently than Netty. Okio uses Segments that are all the same size and then combines adjacent buffers to avoid excessive internal fragmentation. We should leave this in-place and actually set it to 8192.
There was a problem hiding this comment.
(If we were more serious, we'd round up: (capacityHint + 8191) / 8192 * 8192.)
panchenko
changed the title
|
I think I see what you're trying to do, but I'm not sure this is how we want to do it. You've basically realized that we needed the minimum allocation size for unknown message sizes/compression. It seems we should quickly revert the Netty/Servlet changes, since those are known to cause regressions. And I need to stare at this more. CC @shivaspeaks |
ejona86
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
|
CC @AgraVator |
| @@ -222,13 +226,25 @@ private int writeKnownLengthUncompressed(InputStream message, int messageLength) | |||
| headerScratch.put(UNCOMPRESSED).putInt(messageLength); | |||
| // Allocate the initial buffer chunk based on frame header + payload length. | |||
| // Note that the allocator may allocate a buffer larger or smaller than this length | |||
| knownLengthPendingAllocation = HEADER_LENGTH + messageLength; | |||
| if (buffer == null) { | |||
There was a problem hiding this comment.
I think you're right that we need to track the ongoing pending allocation. But because of this condition we can get that allocation mis-aligned if the caller doesn't flush after each message. Instead of subtracting from knownLengthPendingAllocation in allocateKnownLength(), I think doing it in writeRaw() would be better, as most writes pass through writeRaw() and that'd keep the remaining knownLengthPendingAllocation in-sync.
We can also remove this condition and let writeRaw() do the allocation.
| @@ -27,4 +27,12 @@ public interface WritableBufferAllocator { | |||
| * free to return a buffer with a greater or lesser capacity. | |||
| */ | |||
| WritableBuffer allocate(int capacityHint); | |||
|
|
|||
| /** | |||
| * Request a new {@link WritableBuffer} with the given {@code capacityHint}. This method is | |||
There was a problem hiding this comment.
The normal allocate() doesn't say it needs to allocate a greater capacity, so this seems to have identical semantics. I think the need for this method is just showing a caller needs to be fixed. After we have knownLengthPendingAllocation and plumb it into writeRaw(), then I think the only problem would be BufferChainOutputStream's first allocation, which is based just on the byte[] length (which may be of size 1 from write(int)). I think if we do a max(64, len) or similar for that allocation, that'd we be pretty okay, as it already has code that doubles each allocation.
Some allocators should have a minimum size, but that should be based on the minimum size it will actually allocate. We know okio buffers won't ever be less than Segment.SIZE. From a casual reading, it seems Netty won't use a size class less than 16 bytes, so a smaller allocation is probably wasting space and we could probably round up to a power-of-two. The point of that is not for MessageFramer's sake, but because the allocation algorithm would just waste the space otherwise.
Currently this improves 2 flows
Known length message which length is greater than 1Mb
previously the first buffer was 1Mb, and then many buffers of 4096 bytes (from CodedOutputStream), now subsequent buffers are also up to 1Mb
in case of compression, the first write is always 10 bytes buffer (gzip header), but worth allocating more space