From cc069722a2d3ecdd6ec1dc9ae8245eca3e8d734f Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 9 Nov 2017 08:48:19 -0800 Subject: [PATCH] CompositeBytebuf.copy() and copy(...) should respect the allocator Motivation: When calling CompositeBytebuf.copy() and copy(...) we currently use Unpooled to allocate the buffer. This is not really correct and may produce more GC then needed. We should use the allocator that was used when creating the CompositeByteBuf to allocate the new buffer which may be for example the PooledByteBufAllocator. Modifications: - Use alloc() to allocate the new buffer. - Add tests - Fix tests that depend on the copy to be backed by an byte-array without checking hasArray() first. Result: Fixes [#7393]. --- .../io/netty/buffer/CompositeByteBuf.java | 2 +- .../buffer/AbstractCompositeByteBufTest.java | 20 +++++++++ .../io/netty/buffer/ConsolidationTest.java | 41 ++++++++++++------- 3 files changed, 48 insertions(+), 15 deletions(-) diff --git a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java index da4876d837..8a3681b530 100644 --- a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java @@ -1333,7 +1333,7 @@ public class CompositeByteBuf extends AbstractReferenceCountedByteBuf implements @Override public ByteBuf copy(int index, int length) { checkIndex(index, length); - ByteBuf dst = Unpooled.buffer(length); + ByteBuf dst = allocBuffer(length); if (length != 0) { copyTo(index, length, toComponentIndex(index), dst); } diff --git a/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java index 0d3b5db3c9..f51475bf9a 100644 --- a/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/AbstractCompositeByteBufTest.java @@ -1116,4 +1116,24 @@ public abstract class AbstractCompositeByteBufTest extends AbstractByteBufTest { assertEquals(0, buffer.refCnt()); } + @Test + public void testAllocatorIsSameWhenCopy() { + testAllocatorIsSameWhenCopy(false); + } + + @Test + public void testAllocatorIsSameWhenCopyUsingIndexAndLength() { + testAllocatorIsSameWhenCopy(true); + } + + private void testAllocatorIsSameWhenCopy(boolean withIndexAndLength) { + ByteBuf buffer = newBuffer(8); + buffer.writeZero(4); + ByteBuf copy = withIndexAndLength ? buffer.copy(0, 4) : buffer.copy(); + assertEquals(buffer, copy); + assertEquals(buffer.isDirect(), copy.isDirect()); + assertSame(buffer.alloc(), copy.alloc()); + buffer.release(); + copy.release(); + } } diff --git a/buffer/src/test/java/io/netty/buffer/ConsolidationTest.java b/buffer/src/test/java/io/netty/buffer/ConsolidationTest.java index dcbccacb66..e8dcec2102 100644 --- a/buffer/src/test/java/io/netty/buffer/ConsolidationTest.java +++ b/buffer/src/test/java/io/netty/buffer/ConsolidationTest.java @@ -15,6 +15,7 @@ */ package io.netty.buffer; +import io.netty.util.CharsetUtil; import org.junit.Test; import static io.netty.buffer.Unpooled.*; @@ -26,11 +27,13 @@ import static org.junit.Assert.*; public class ConsolidationTest { @Test public void shouldWrapInSequence() { - ByteBuf currentBuffer = wrappedBuffer(wrappedBuffer("a".getBytes()), wrappedBuffer("=".getBytes())); - currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("1".getBytes()), wrappedBuffer("&".getBytes())); + ByteBuf currentBuffer = wrappedBuffer(wrappedBuffer("a".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII))); + currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("1".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII))); ByteBuf copy = currentBuffer.copy(); - String s = new String(copy.array()); + String s = copy.toString(CharsetUtil.US_ASCII); assertEquals("a=1&", s); currentBuffer.release(); @@ -39,23 +42,33 @@ public class ConsolidationTest { @Test public void shouldConsolidationInSequence() { - ByteBuf currentBuffer = wrappedBuffer(wrappedBuffer("a".getBytes()), wrappedBuffer("=".getBytes())); - currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("1".getBytes()), wrappedBuffer("&".getBytes())); + ByteBuf currentBuffer = wrappedBuffer(wrappedBuffer("a".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII))); + currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("1".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII))); - currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("b".getBytes()), wrappedBuffer("=".getBytes())); - currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("2".getBytes()), wrappedBuffer("&".getBytes())); + currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("b".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII))); + currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("2".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII))); - currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("c".getBytes()), wrappedBuffer("=".getBytes())); - currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("3".getBytes()), wrappedBuffer("&".getBytes())); + currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("c".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII))); + currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("3".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII))); - currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("d".getBytes()), wrappedBuffer("=".getBytes())); - currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("4".getBytes()), wrappedBuffer("&".getBytes())); + currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("d".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII))); + currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("4".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII))); - currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("e".getBytes()), wrappedBuffer("=".getBytes())); - currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("5".getBytes()), wrappedBuffer("&".getBytes())); + currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("e".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("=".getBytes(CharsetUtil.US_ASCII))); + currentBuffer = wrappedBuffer(currentBuffer, wrappedBuffer("5".getBytes(CharsetUtil.US_ASCII)), + wrappedBuffer("&".getBytes(CharsetUtil.US_ASCII))); ByteBuf copy = currentBuffer.copy(); - String s = new String(copy.array()); + String s = copy.toString(CharsetUtil.US_ASCII); assertEquals("a=1&b=2&c=3&d=4&e=5&", s); currentBuffer.release();