Repository navigation
Add configurable Thrift buffer reuse thresholds to Java clients #19148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,28 +28,36 @@ | |
| public class AutoScalingBufferReadTransport extends NonOpenTransport { | ||
|
|
||
| private final AutoResizingBuffer buf; | ||
| private final int maxReusableBufferSize; | ||
| private byte[] temporaryBuffer; | ||
| private int pos = 0; | ||
| private int limit = 0; | ||
|
|
||
| public AutoScalingBufferReadTransport(int initialCapacity) throws IOException { | ||
| this(initialCapacity, Integer.MAX_VALUE); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep the existing constructor's unlimited reuse policy while allowing transports that opt in to provide a per-instance threshold. This avoids changing existing callers or sharing one client's setting with another. |
||
| } | ||
|
|
||
| AutoScalingBufferReadTransport(int initialCapacity, int maxReusableBufferSize) | ||
| throws IOException { | ||
| this.buf = new AutoResizingBuffer(initialCapacity); | ||
| this.maxReusableBufferSize = maxReusableBufferSize; | ||
| } | ||
|
|
||
| public void fill(TTransport inTrans, int length) throws TTransportException { | ||
| try { | ||
| buf.resizeIfNecessary(length); | ||
| resizeIfNecessary(length); | ||
| } catch (IOException e) { | ||
| throw new TTransportException(e); | ||
| } | ||
| inTrans.readAll(buf.array(), 0, length); | ||
| inTrans.readAll(getBuffer(), 0, length); | ||
| pos = 0; | ||
| limit = length; | ||
| } | ||
|
|
||
| @Override | ||
| public final int read(byte[] target, int off, int len) { | ||
| int amtToRead = Math.min(len, getBytesRemainingInBuffer()); | ||
| System.arraycopy(buf.array(), pos, target, off, amtToRead); | ||
| System.arraycopy(getBuffer(), pos, target, off, amtToRead); | ||
| consumeBuffer(amtToRead); | ||
| return amtToRead; | ||
| } | ||
|
|
@@ -62,6 +70,12 @@ public void write(byte[] buf, int off, int len) { | |
| @Override | ||
| public final void consumeBuffer(int len) { | ||
| pos += len; | ||
| if (pos == limit && temporaryBuffer != null) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop the transport's reference only after the temporary frame is fully consumed, and clear it on close as well. Protocol-returned ByteBuffers can retain their own array reference; freeing the transport reference must not overwrite those values. |
||
| // Protocols may keep their own reference to a consumed binary value. Drop only our reference. | ||
| temporaryBuffer = null; | ||
| pos = 0; | ||
| limit = 0; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -82,7 +96,7 @@ public void checkReadBytesAvailable(long numBytes) throws TTransportException { | |
|
|
||
| @Override | ||
| public final byte[] getBuffer() { | ||
| return buf.array(); | ||
| return temporaryBuffer == null ? buf.array() : temporaryBuffer; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -96,12 +110,22 @@ public final int getBytesRemainingInBuffer() { | |
| } | ||
|
|
||
| public void resizeIfNecessary(int size) throws IOException { | ||
| buf.resizeIfNecessary(size); | ||
| if (size > maxReusableBufferSize) { | ||
| byte[] newBuffer = new byte[size]; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Requests above the threshold allocate temporary read storage and reset the accounted reusable array to its initial capacity. fill(), read(), and getBuffer() all select the same active array, so partial reads keep using the frame that was actually received. Exact-boundary requests still reuse storage. |
||
| buf.resetToInitialCapacity(); | ||
| temporaryBuffer = newBuffer; | ||
| } else { | ||
| buf.resizeIfNecessary(size); | ||
| temporaryBuffer = null; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| super.close(); | ||
| temporaryBuffer = null; | ||
| pos = 0; | ||
| limit = 0; | ||
| buf.close(); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| import org.apache.thrift.transport.TTransportException; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Arrays; | ||
|
|
||
| /** | ||
| * Note that this class is mainly copied from class {@link | ||
|
|
@@ -32,10 +33,18 @@ | |
| public class AutoScalingBufferWriteTransport extends NonOpenTransport { | ||
|
|
||
| private final AutoResizingBuffer buf; | ||
| private final int maxReusableBufferSize; | ||
| private byte[] temporaryBuffer; | ||
| private int pos; | ||
|
|
||
| public AutoScalingBufferWriteTransport(int initialCapacity) throws IOException { | ||
| this(initialCapacity, Integer.MAX_VALUE); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preserve the public constructor's reuse behavior and store an opt-in threshold per write buffer. Both ordinary and compression output buffers can use the same policy without introducing shared mutable configuration. |
||
| } | ||
|
|
||
| AutoScalingBufferWriteTransport(int initialCapacity, int maxReusableBufferSize) | ||
| throws IOException { | ||
| this.buf = new AutoResizingBuffer(initialCapacity); | ||
| this.maxReusableBufferSize = maxReusableBufferSize; | ||
| this.pos = 0; | ||
| } | ||
|
|
||
|
|
@@ -49,11 +58,11 @@ public void write(byte[] toWrite, int off, int len) throws TTransportException { | |
| try { | ||
| // A small prefix during serialization does not indicate a small frame. Only the complete | ||
| // frame size, supplied after flushing, may trigger shrinking. | ||
| buf.ensureCapacity(pos + len); | ||
| ensureCapacity(pos + len); | ||
| } catch (IOException e) { | ||
| throw new TTransportException(e); | ||
| } | ||
| System.arraycopy(toWrite, off, buf.array(), pos, len); | ||
| System.arraycopy(toWrite, off, getBuffer(), pos, len); | ||
| pos += len; | ||
| } | ||
|
|
||
|
|
@@ -63,21 +72,55 @@ public int getPos() { | |
|
|
||
| public void reset() { | ||
| pos = 0; | ||
| temporaryBuffer = null; | ||
| } | ||
|
|
||
| void resetAndResize() throws IOException { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After a flush, release temporary storage without immediately allocating another temporary array for the completed large frame. Frames within the threshold retain the existing shrink policy; reset()/close() also drop any pending temporary write. |
||
| int length = pos; | ||
| reset(); | ||
| // A completed large frame must not recreate the temporary array just released by reset(). | ||
| if (length <= maxReusableBufferSize) { | ||
| buf.resizeIfNecessary(length); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| super.close(); | ||
| reset(); | ||
| buf.close(); | ||
| } | ||
|
|
||
| public void resizeIfNecessary(int size) throws IOException { | ||
| buf.resizeIfNecessary(size); | ||
| if (size > maxReusableBufferSize) { | ||
| ensureCapacity(size); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Compression writes directly into a capacity-reserved output array, so resizeIfNecessary() must apply the temporary-storage policy in addition to the incremental write() path. Returning to a small request selects reusable storage again. |
||
| } else { | ||
| buf.resizeIfNecessary(size); | ||
| temporaryBuffer = null; | ||
| } | ||
| } | ||
|
|
||
| private void ensureCapacity(int size) throws IOException { | ||
| if (size > maxReusableBufferSize) { | ||
| if (temporaryBuffer == null || temporaryBuffer.length < size) { | ||
| // Preserve an unfinished frame when incremental serialization crosses the threshold. | ||
| // Keep amortized growth for subsequent small writes within the same temporary frame. | ||
| int capacity = | ||
| temporaryBuffer == null | ||
| ? size | ||
| : Math.max(size, temporaryBuffer.length + (temporaryBuffer.length >> 1)); | ||
| byte[] newBuffer = Arrays.copyOf(getBuffer(), capacity); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preserve the already serialized prefix when incremental writes cross the threshold. Later temporary-array growth remains amortized, and the reusable allocation is reset without discarding the unfinished frame. The byte-by-byte regression test verifies the transition and subsequent growth. |
||
| buf.resetToInitialCapacity(); | ||
| temporaryBuffer = newBuffer; | ||
| } | ||
| } else { | ||
| buf.ensureCapacity(size); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public byte[] getBuffer() { | ||
| return buf.array(); | ||
| return temporaryBuffer == null ? buf.array() : temporaryBuffer; | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,14 +42,28 @@ public static DeepCopyRpcTransportFactory getInstance( | |
| return create(USE_SNAPPY, thriftDefaultBufferSize, thriftMaxFrameSize); | ||
| } | ||
|
|
||
| /** Creates a client factory whose reusable-buffer threshold is independent of other clients. */ | ||
| public static DeepCopyRpcTransportFactory getInstance( | ||
| int thriftDefaultBufferSize, int thriftMaxFrameSize, int maxReusableBufferSize) { | ||
| return create(USE_SNAPPY, thriftDefaultBufferSize, thriftMaxFrameSize, maxReusableBufferSize); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Construct a separate client factory carrying the requested threshold into both plain and Snappy transports. The existing overload delegates with Integer.MAX_VALUE, preserving old callers. The independent-factories regression verifies both policies in one JVM. |
||
| } | ||
|
|
||
| private static DeepCopyRpcTransportFactory create( | ||
| boolean useSnappy, int thriftDefaultBufferSize, int thriftMaxFrameSize) { | ||
| return create(useSnappy, thriftDefaultBufferSize, thriftMaxFrameSize, Integer.MAX_VALUE); | ||
| } | ||
|
|
||
| private static DeepCopyRpcTransportFactory create( | ||
| boolean useSnappy, | ||
| int thriftDefaultBufferSize, | ||
| int thriftMaxFrameSize, | ||
| int maxReusableBufferSize) { | ||
| return useSnappy | ||
| ? new DeepCopyRpcTransportFactory( | ||
| new TimeoutChangeableTSnappyFramedTransport.Factory( | ||
| thriftDefaultBufferSize, thriftMaxFrameSize, true)) | ||
| thriftDefaultBufferSize, thriftMaxFrameSize, true, maxReusableBufferSize)) | ||
| : new DeepCopyRpcTransportFactory( | ||
| new TimeoutChangeableTFastFramedTransport.Factory( | ||
| thriftDefaultBufferSize, thriftMaxFrameSize, true)); | ||
| thriftDefaultBufferSize, thriftMaxFrameSize, true, maxReusableBufferSize)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,10 +36,28 @@ protected TCompressedElasticFramedTransport( | |
| int thriftMaxFrameSize, | ||
| boolean copyBinary) | ||
| throws TTransportException { | ||
| super(underlying, thriftDefaultBufferSize, thriftMaxFrameSize, copyBinary); | ||
| this(underlying, thriftDefaultBufferSize, thriftMaxFrameSize, copyBinary, Integer.MAX_VALUE); | ||
| } | ||
|
|
||
| /** | ||
| * @param maxReusableBufferSize requests larger than this number of bytes use temporary arrays in | ||
| * all four buffers and reset their reusable storage to {@code thriftDefaultBufferSize}; | ||
| * {@link Integer#MAX_VALUE} preserves the original resizing policy | ||
| */ | ||
| protected TCompressedElasticFramedTransport( | ||
| TTransport underlying, | ||
| int thriftDefaultBufferSize, | ||
| int thriftMaxFrameSize, | ||
| boolean copyBinary, | ||
| int maxReusableBufferSize) | ||
| throws TTransportException { | ||
| super( | ||
| underlying, thriftDefaultBufferSize, thriftMaxFrameSize, copyBinary, maxReusableBufferSize); | ||
| try { | ||
| writeCompressBuffer = new AutoScalingBufferWriteTransport(thriftDefaultBufferSize); | ||
| readCompressBuffer = new AutoScalingBufferReadTransport(thriftDefaultBufferSize); | ||
| writeCompressBuffer = | ||
| new AutoScalingBufferWriteTransport(thriftDefaultBufferSize, maxReusableBufferSize); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apply the same threshold to compression/decompression scratch arrays as to the inherited raw buffers. Delegating the old constructor with Integer.MAX_VALUE keeps the existing four-buffer reuse policy for unchanged callers. |
||
| readCompressBuffer = | ||
| new AutoScalingBufferReadTransport(thriftDefaultBufferSize, maxReusableBufferSize); | ||
| } catch (IOException e) { | ||
| closeAllocatedBuffers(); | ||
| throw new TTransportException(e); | ||
|
|
@@ -97,11 +115,13 @@ public void flush() throws TTransportException { | |
| underlying.write(writeCompressBuffer.getBuffer(), 0, compressedLength); | ||
| } catch (IOException e) { | ||
| throw new TTransportException(e); | ||
| } finally { | ||
| // The compressed output has been handed to the underlying transport, so do not retain it. | ||
| writeCompressBuffer.reset(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Release temporary compressed output after compression/writing, including exceptional paths, and reset the raw write buffer without recreating a large temporary array. Existing compressed wire-frame limits and underlying flush behavior remain in place. |
||
| } | ||
|
|
||
| writeBuffer.reset(); | ||
| try { | ||
| writeBuffer.resizeIfNecessary(length); | ||
| writeBuffer.resetAndResize(); | ||
| } catch (IOException e) { | ||
| throw new TTransportException(e); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,14 +87,26 @@ public TElasticFramedTransport( | |
| int thriftMaxFrameSize, | ||
| boolean copyBinary) | ||
| throws TTransportException { | ||
| this(underlying, thriftDefaultBufferSize, thriftMaxFrameSize, copyBinary, Integer.MAX_VALUE); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep the original framed-transport constructor compatible and add an overload that applies the threshold to both raw read and write buffers. Socket and in-memory factory paths use the same underlying buffer implementation. |
||
| } | ||
|
|
||
| protected TElasticFramedTransport( | ||
| TTransport underlying, | ||
| int thriftDefaultBufferSize, | ||
| int thriftMaxFrameSize, | ||
| boolean copyBinary, | ||
| int maxReusableBufferSize) | ||
| throws TTransportException { | ||
| this.underlying = underlying; | ||
| this.thriftDefaultBufferSize = thriftDefaultBufferSize; | ||
| this.thriftMaxFrameSize = thriftMaxFrameSize; | ||
| this.copyBinary = copyBinary; | ||
| alignUnderlyingConfiguration(); | ||
| try { | ||
| readBuffer = new AutoScalingBufferReadTransport(thriftDefaultBufferSize); | ||
| writeBuffer = new AutoScalingBufferWriteTransport(thriftDefaultBufferSize); | ||
| readBuffer = | ||
| new AutoScalingBufferReadTransport(thriftDefaultBufferSize, maxReusableBufferSize); | ||
| writeBuffer = | ||
| new AutoScalingBufferWriteTransport(thriftDefaultBufferSize, maxReusableBufferSize); | ||
| } catch (IOException e) { | ||
| closeAllocatedBuffers(); | ||
| throw new TTransportException(e); | ||
|
|
@@ -316,9 +328,8 @@ public void flush() throws TTransportException { | |
| TFramedTransport.encodeFrameSize(length, i32buf); | ||
| underlying.write(i32buf, 0, 4); | ||
| underlying.write(writeBuffer.getBuffer(), 0, length); | ||
| writeBuffer.reset(); | ||
| try { | ||
| writeBuffer.resizeIfNecessary(length); | ||
| writeBuffer.resetAndResize(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the completed-frame reset operation after sending a plain frame so its temporary array is released while small frames continue to follow the original reuse/shrink policy. Frame-size validation still occurs before writing the frame. |
||
| } catch (IOException e) { | ||
| throw new TTransportException(e); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,8 @@ public class TSnappyElasticFramedTransport extends TCompressedElasticFramedTrans | |
|
|
||
| public static class Factory extends TElasticFramedTransport.Factory { | ||
|
|
||
| private final int maxReusableBufferSize; | ||
|
|
||
| public Factory() { | ||
| this(RpcUtils.THRIFT_DEFAULT_BUF_CAPACITY, RpcUtils.THRIFT_FRAME_MAX_SIZE, true); | ||
| } | ||
|
|
@@ -38,13 +40,25 @@ public Factory(int thriftDefaultBufferSize, boolean copyBinary) { | |
| } | ||
|
|
||
| public Factory(int thriftDefaultBufferSize, int thriftMaxFrameSize, boolean copyBinary) { | ||
| this(thriftDefaultBufferSize, thriftMaxFrameSize, copyBinary, Integer.MAX_VALUE); | ||
| } | ||
|
|
||
| /** | ||
| * @param maxReusableBufferSize buffer requests above this size in bytes use temporary arrays | ||
| */ | ||
| public Factory( | ||
| int thriftDefaultBufferSize, | ||
| int thriftMaxFrameSize, | ||
| boolean copyBinary, | ||
| int maxReusableBufferSize) { | ||
| super(thriftDefaultBufferSize, thriftMaxFrameSize, copyBinary); | ||
| this.maxReusableBufferSize = maxReusableBufferSize; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Carry the opt-in threshold through the Snappy factory into each constructed transport, keeping the old factory constructors on the original reuse policy. This also covers callers that wrap a non-socket transport. |
||
| } | ||
|
|
||
| @Override | ||
| public TTransport getTransport(TTransport trans) throws TTransportException { | ||
| return new TSnappyElasticFramedTransport( | ||
| trans, thriftDefaultBufferSize, thriftMaxFrameSize, copyBinary); | ||
| trans, thriftDefaultBufferSize, thriftMaxFrameSize, copyBinary, maxReusableBufferSize); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -61,6 +75,20 @@ public TSnappyElasticFramedTransport( | |
| super(underlying, thriftDefaultBufferSize, thriftMaxFrameSize, copyBinary); | ||
| } | ||
|
|
||
| /** | ||
| * @param maxReusableBufferSize buffer requests above this size in bytes use temporary arrays | ||
| */ | ||
| public TSnappyElasticFramedTransport( | ||
| TTransport underlying, | ||
| int thriftDefaultBufferSize, | ||
| int thriftMaxFrameSize, | ||
| boolean copyBinary, | ||
| int maxReusableBufferSize) | ||
| throws TTransportException { | ||
| super( | ||
| underlying, thriftDefaultBufferSize, thriftMaxFrameSize, copyBinary, maxReusableBufferSize); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Expose a Snappy transport constructor accepting the reuse threshold and forward it to the four-buffer implementation. Existing constructors continue to work unchanged. |
||
| } | ||
|
|
||
| @Override | ||
| protected int uncompressedLength(byte[] buf, int off, int len) throws IOException { | ||
| return Snappy.uncompressedLength(buf, off, len); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reset the reusable array through the existing accounting-aware resize path, so releasing a large retained allocation also releases its memory reservation. Resetting the shrink counters keeps later small-frame reuse on the existing policy.