From 4db38b4e0c734b9cdaf0591dd0c5b3f4d4f11ea4 Mon Sep 17 00:00:00 2001 From: Nick Hill Date: Thu, 15 Aug 2019 23:18:09 -0700 Subject: [PATCH] Don't zero non-readable buffer regions when capacity is decreased (#9427) Motivation #1802 fixed ByteBuf implementations to ensure that the whole buffer region is preserved when capacity is increased, not just the readable part. The behaviour is still different however when the capacity is _decreased_ - data outside the currently-readable region is zeroed. Modifications Update ByteBuf capacity(int) implementations to also copy the whole buffer region when the new capacity is less than the current capacity. Result Consistent behaviour of ByteBuf#capacity(int) regardless of whether the new capacity is greater than or less than the current capacity. --- .../java/io/netty/buffer/AbstractByteBuf.java | 7 ++++ .../main/java/io/netty/buffer/PoolArena.java | 30 ++++--------- .../java/io/netty/buffer/PooledByteBuf.java | 2 +- .../netty/buffer/UnpooledDirectByteBuf.java | 42 +++++++------------ .../io/netty/buffer/UnpooledHeapByteBuf.java | 36 +++++++--------- .../UnpooledUnsafeNoCleanerDirectByteBuf.java | 14 +------ 6 files changed, 48 insertions(+), 83 deletions(-) diff --git a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java index 3a19f85c21..254d40480a 100644 --- a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java @@ -268,6 +268,13 @@ public abstract class AbstractByteBuf extends ByteBuf { } } + // Called after a capacity reduction + protected final void trimIndicesToCapacity(int newCapacity) { + if (writerIndex() > newCapacity) { + setIndex0(Math.min(readerIndex(), newCapacity), newCapacity); + } + } + @Override public ByteBuf ensureWritable(int minWritableBytes) { checkPositiveOrZero(minWritableBytes, "minWritableBytes"); diff --git a/buffer/src/main/java/io/netty/buffer/PoolArena.java b/buffer/src/main/java/io/netty/buffer/PoolArena.java index 67955ce6d4..2005e9a0ec 100644 --- a/buffer/src/main/java/io/netty/buffer/PoolArena.java +++ b/buffer/src/main/java/io/netty/buffer/PoolArena.java @@ -379,9 +379,7 @@ abstract class PoolArena implements PoolArenaMetric { } void reallocate(PooledByteBuf buf, int newCapacity, boolean freeOldMemory) { - if (newCapacity < 0 || newCapacity > buf.maxCapacity()) { - throw new IllegalArgumentException("newCapacity: " + newCapacity); - } + assert newCapacity >= 0 && newCapacity <= buf.maxCapacity(); int oldCapacity = buf.length; if (oldCapacity == newCapacity) { @@ -394,29 +392,17 @@ abstract class PoolArena implements PoolArenaMetric { T oldMemory = buf.memory; int oldOffset = buf.offset; int oldMaxLength = buf.maxLength; - int readerIndex = buf.readerIndex(); - int writerIndex = buf.writerIndex(); + // This does not touch buf's reader/writer indices allocate(parent.threadCache(), buf, newCapacity); + int bytesToCopy; if (newCapacity > oldCapacity) { - memoryCopy( - oldMemory, oldOffset, - buf.memory, buf.offset, oldCapacity); - } else if (newCapacity < oldCapacity) { - if (readerIndex < newCapacity) { - if (writerIndex > newCapacity) { - writerIndex = newCapacity; - } - memoryCopy( - oldMemory, oldOffset + readerIndex, - buf.memory, buf.offset + readerIndex, writerIndex - readerIndex); - } else { - readerIndex = writerIndex = newCapacity; - } + bytesToCopy = oldCapacity; + } else { + buf.trimIndicesToCapacity(newCapacity); + bytesToCopy = newCapacity; } - - buf.setIndex(readerIndex, writerIndex); - + memoryCopy(oldMemory, oldOffset, buf.memory, buf.offset, bytesToCopy); if (freeOldMemory) { free(oldChunk, oldNioBuffer, oldHandle, oldMaxLength, buf.cache); } diff --git a/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java b/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java index 8c2d54674d..5b984eb7e7 100644 --- a/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/PooledByteBuf.java @@ -110,7 +110,7 @@ abstract class PooledByteBuf extends AbstractReferenceCountedByteBuf { (maxLength > 512 || newCapacity > maxLength - 16)) { // here newCapacity < length length = newCapacity; - setIndex(Math.min(readerIndex(), newCapacity), Math.min(writerIndex(), newCapacity)); + trimIndicesToCapacity(newCapacity); return this; } } diff --git a/buffer/src/main/java/io/netty/buffer/UnpooledDirectByteBuf.java b/buffer/src/main/java/io/netty/buffer/UnpooledDirectByteBuf.java index af465c2cd9..daad1408e7 100644 --- a/buffer/src/main/java/io/netty/buffer/UnpooledDirectByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/UnpooledDirectByteBuf.java @@ -146,35 +146,23 @@ public class UnpooledDirectByteBuf extends AbstractReferenceCountedByteBuf { @Override public ByteBuf capacity(int newCapacity) { checkNewCapacity(newCapacity); - - int readerIndex = readerIndex(); - int writerIndex = writerIndex(); - int oldCapacity = capacity; - if (newCapacity > oldCapacity) { - ByteBuffer oldBuffer = buffer; - ByteBuffer newBuffer = allocateDirect(newCapacity); - oldBuffer.position(0).limit(oldBuffer.capacity()); - newBuffer.position(0).limit(oldBuffer.capacity()); - newBuffer.put(oldBuffer); - newBuffer.clear(); - setByteBuffer(newBuffer, true); - } else if (newCapacity < oldCapacity) { - ByteBuffer oldBuffer = buffer; - ByteBuffer newBuffer = allocateDirect(newCapacity); - if (readerIndex < newCapacity) { - if (writerIndex > newCapacity) { - writerIndex(writerIndex = newCapacity); - } - oldBuffer.position(readerIndex).limit(writerIndex); - newBuffer.position(readerIndex).limit(writerIndex); - newBuffer.put(oldBuffer); - newBuffer.clear(); - } else { - setIndex(newCapacity, newCapacity); - } - setByteBuffer(newBuffer, true); + if (newCapacity == oldCapacity) { + return this; } + int bytesToCopy; + if (newCapacity > oldCapacity) { + bytesToCopy = oldCapacity; + } else { + trimIndicesToCapacity(newCapacity); + bytesToCopy = newCapacity; + } + ByteBuffer oldBuffer = buffer; + ByteBuffer newBuffer = allocateDirect(newCapacity); + oldBuffer.position(0).limit(bytesToCopy); + newBuffer.position(0).limit(bytesToCopy); + newBuffer.put(oldBuffer).clear(); + setByteBuffer(newBuffer, true); return this; } diff --git a/buffer/src/main/java/io/netty/buffer/UnpooledHeapByteBuf.java b/buffer/src/main/java/io/netty/buffer/UnpooledHeapByteBuf.java index 083583956f..0db078b665 100644 --- a/buffer/src/main/java/io/netty/buffer/UnpooledHeapByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/UnpooledHeapByteBuf.java @@ -120,29 +120,23 @@ public class UnpooledHeapByteBuf extends AbstractReferenceCountedByteBuf { @Override public ByteBuf capacity(int newCapacity) { checkNewCapacity(newCapacity); - - int oldCapacity = array.length; byte[] oldArray = array; - if (newCapacity > oldCapacity) { - byte[] newArray = allocateArray(newCapacity); - System.arraycopy(oldArray, 0, newArray, 0, oldArray.length); - setArray(newArray); - freeArray(oldArray); - } else if (newCapacity < oldCapacity) { - byte[] newArray = allocateArray(newCapacity); - int readerIndex = readerIndex(); - if (readerIndex < newCapacity) { - int writerIndex = writerIndex(); - if (writerIndex > newCapacity) { - writerIndex(writerIndex = newCapacity); - } - System.arraycopy(oldArray, readerIndex, newArray, readerIndex, writerIndex - readerIndex); - } else { - setIndex(newCapacity, newCapacity); - } - setArray(newArray); - freeArray(oldArray); + int oldCapacity = oldArray.length; + if (newCapacity == oldCapacity) { + return this; } + + int bytesToCopy; + if (newCapacity > oldCapacity) { + bytesToCopy = oldCapacity; + } else { + trimIndicesToCapacity(newCapacity); + bytesToCopy = newCapacity; + } + byte[] newArray = allocateArray(newCapacity); + System.arraycopy(oldArray, 0, newArray, 0, bytesToCopy); + setArray(newArray); + freeArray(oldArray); return this; } diff --git a/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeNoCleanerDirectByteBuf.java b/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeNoCleanerDirectByteBuf.java index 3b9c05b83b..cc00e865ad 100644 --- a/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeNoCleanerDirectByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeNoCleanerDirectByteBuf.java @@ -48,18 +48,8 @@ class UnpooledUnsafeNoCleanerDirectByteBuf extends UnpooledUnsafeDirectByteBuf { return this; } - ByteBuffer newBuffer = reallocateDirect(buffer, newCapacity); - - if (newCapacity < oldCapacity) { - if (readerIndex() < newCapacity) { - if (writerIndex() > newCapacity) { - writerIndex(newCapacity); - } - } else { - setIndex(newCapacity, newCapacity); - } - } - setByteBuffer(newBuffer, false); + trimIndicesToCapacity(newCapacity); + setByteBuffer(reallocateDirect(buffer, newCapacity), false); return this; } }