Repository: netty/netty
Issue: MessageToByteEncoder always starts with ByteBuf that use initalCapacity == 0
Labels: improvement

The problem is that we use ByteBufAllocator.ioBuffer() which returns a ByteBuf with intialCapacity of 0 when preferDirect is specified in the constructor. 


Issue comments:
@trustin so either we let ioBuffer() not use size of 0 as default or allow to configure it. I would prefer the first. WDYT ?

It's due to a historical reason - long time ago in the early days of 4.0.AlphaX, we allocated a buffer per handler.  Having a non-zero capacity buffer from the beginning of the connection was a huge waste.

We are not affected by that problem anymore, so I think we are fine to use the same default initial capacity for `ioBuffer()`.

Actually, an encoder implementation should know better about the initial capacity of the buffer.  How about adding a protected method?

``` java
    protected ByteBuf allocateBuffer(ChannelHandlerContext ctx, I msg) throws Exception {
        // default impl:
        if (preferDirect) {
            return ctx.alloc().ioBuffer();
        } else {
            return ctx.alloc().heapBuffer();
        }
    }
```

The overriding methods need to know if the handler prefers a direct buffer or not, so maybe we should make `preferDirect` protected, add a new protected method `isDirectBufferPreferred()`, or just pass it as a parameter to `allocateBuffer()`.

We could also remove the constructor that accepts `preferDirect` and let `allocateBuffer()` use `ioBuffer()` by default.  If a user prefers a heap buffer, he or she can still override it.

For 4.0/4.1, we could simply deprecate that constructor and let the default `allocateBuffer()` impl respect `preferDirect`.

What about adding another constructor that take preferDirect and initialCapacity as parameter and deprecate the old constructors that accept preferDirect? The initialCapacity will be used for direct and heap buffers.

WDYT?

> Am 21.06.2014 um 12:35 schrieb Trustin Lee notifications@github.com:
> 
> We could also remove the constructor that accepts preferDirect and let allocateBuffer() use ioBuffer() by default. If a user prefers a heap buffer, he or she can still override it.
> 
> For 4.0/4.1, we could simply deprecate that constructor and let the default allocateBuffer() impl respect preferDirect.
> 
> —
> Reply to this email directly or view it on GitHub.

I think we should add a protected method that allows a handler to allocate a buffer in a contextual way, because the initial capacity of a buffer really depends on a message being encoded.

Let me come up with a PR

> Am 22.06.2014 um 05:04 schrieb Trustin Lee notifications@github.com:
> 
> I think we should add a protected method that allows a handler to allocate a buffer in a contextual way, because the initial capacity of a buffer really depends on a message being encoded.
> 
> —
> Reply to this email directly or view it on GitHub.

Fixed by #2591 


Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/AbstractByteBufAllocator.java b/buffer/src/main/java/io/netty/buffer/AbstractByteBufAllocator.java
index 4d39bc29eed..34e5d2c6dd4 100644
--- a/buffer/src/main/java/io/netty/buffer/AbstractByteBufAllocator.java
+++ b/buffer/src/main/java/io/netty/buffer/AbstractByteBufAllocator.java
@@ -96,9 +96,9 @@ public ByteBuf buffer(int initialCapacity, int maxCapacity) {
     @Override
     public ByteBuf ioBuffer() {
         if (PlatformDependent.hasUnsafe()) {
-            return directBuffer(0);
+            return directBuffer(DEFAULT_INITIAL_CAPACITY);
         }
-        return heapBuffer(0);
+        return heapBuffer(DEFAULT_INITIAL_CAPACITY);
     }
 
     @Override
diff --git a/buffer/src/main/java/io/netty/buffer/ByteBufAllocator.java b/buffer/src/main/java/io/netty/buffer/ByteBufAllocator.java
index e1778020b24..ca7df32ce0b 100644
--- a/buffer/src/main/java/io/netty/buffer/ByteBufAllocator.java
+++ b/buffer/src/main/java/io/netty/buffer/ByteBufAllocator.java
@@ -43,7 +43,7 @@ public interface ByteBufAllocator {
     ByteBuf buffer(int initialCapacity, int maxCapacity);
 
     /**
-     * Allocate a {@link ByteBuf} whose initial capacity is 0, preferably a direct buffer which is suitable for I/O.
+     * Allocate a {@link ByteBuf}, preferably a direct buffer which is suitable for I/O.
      */
     ByteBuf ioBuffer();
 
diff --git a/codec/src/main/java/io/netty/handler/codec/MessageToByteEncoder.java b/codec/src/main/java/io/netty/handler/codec/MessageToByteEncoder.java
index f6c506b2375..0aab4247c21 100644
--- a/codec/src/main/java/io/netty/handler/codec/MessageToByteEncoder.java
+++ b/codec/src/main/java/io/netty/handler/codec/MessageToByteEncoder.java
@@ -102,11 +102,7 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise)
             if (acceptOutboundMessage(msg)) {
                 @SuppressWarnings("unchecked")
                 I cast = (I) msg;
-                if (preferDirect) {
-                    buf = ctx.alloc().ioBuffer();
-                } else {
-                    buf = ctx.alloc().heapBuffer();
-                }
+                buf = allocateBuffer(ctx, cast, preferDirect);
                 try {
                     encode(ctx, cast, buf);
                 } finally {
@@ -134,6 +130,19 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise)
         }
     }
 
+    /**
+     * Allocate a {@link ByteBuf} which will be used as argument of {@link #encode(ChannelHandlerContext, I, ByteBuf)}.
+     * Sub-classes may override this method to returna {@link ByteBuf} with a perfect matching {@code initialCapacity}.
+     */
+    protected ByteBuf allocateBuffer(ChannelHandlerContext ctx, @SuppressWarnings("unused") I msg,
+                               boolean preferDirect) throws Exception {
+        if (preferDirect) {
+            return ctx.alloc().ioBuffer();
+        } else {
+            return ctx.alloc().heapBuffer();
+        }
+    }
+
     /**
      * Encode a message into a {@link ByteBuf}. This method will be called for each written message that can be handled
      * by this encoder.


Test patch: