Repository: netty/netty
Issue: ByteBufUtil.writeUtf8 and WrappedByteBuf
Labels: improvement

There is interesting comment:

```
// Maybe we could also check if we can unwrap() to access the wrapped buffer which
// may be an AbstractByteBuf. But this may be overkill so let us keep it simple for now.
```

But - simple HTTP server under load (like 5k req/sec) uses 38% time on String.getBytes. I use ByteBufUtil.writeUtf8 to write HTTP response (about 10Kb).

Please consider adding that check. Additional instanceof is cheap those days and do matter in some cases.

Well, I can try ByteBuf.unwrap() to get to AbstractByteBuf but is that safe?

Netty 4.0.32.Final, ResourceLeakDetector is on SIMPLE.


Issue comments:
Will check

> Am 03.10.2015 um 13:26 schrieb scf37 notifications@github.com:
> 
> There is interesting comment:
> 
> // Maybe we could also check if we can unwrap() to access the wrapped buffer which
> // may be an AbstractByteBuf. But this may be overkill so let us keep it simple for now.
> But - simple HTTP server under load (like 5k req/sec) uses 38% time on String.getBytes. I use ByteBufUtil.writeUtf8 to write HTTP response (about 10Kb).
> 
> Please consider adding that check. Additional instanceof is cheap those days and do matter in some cases.
> 
> Netty 4.0.32.Final, ResourceLeakDetector is on SIMPLE.
> 
> —
> Reply to this email directly or view it on GitHub.

@scf37 can you show me your code and how this change would help you ?

Code is qute complex, but profiling via jmc states that `writeUtf8` is the bottleneck.

How 20-30% improved performance would help me? Good question. Not even sure what to answer. May be I should stick with unwrap() hack, at least it is fast.

```
override def read(): Future[HttpContent] = {
  //reading by blocks of default socket buffer size
  val len = str.length

  if (readDone) {
    throw new IllegalStateException("Response.read() after EOF")
  }

  val size = Math.min(len - off, 65536)

  //netty internals use size * 3 to ensure buffer for utf-8 strings.
  //Set it as expected to avoid later resizing
  val buf = PooledByteBufAllocator.DEFAULT.ioBuffer(size * 3)

  //ByteBufUtil.writeUtf8 has fast path for WrappedByteBuf
  //unfortunately resource leak detector wraps it in WrappedByteBuf which is not detected
  //by writeUtf8
  //so, trying to unwraaaaap
  //see https://github.com/netty/netty/issues/4313
  val fastBuf = buf match {
    case buf: AbstractByteBuf => buf
    case buf if buf.unwrap().isInstanceOf[AbstractByteBuf] => buf.unwrap()
    case buf => buf
  }

  if (off == 0 && size == len) {
    ByteBufUtil.writeUtf8(fastBuf, str)
  } else {
    ByteBufUtil.writeUtf8(fastBuf, new CharSequenceView(str, off, size))
  }

  off += size

  if (off == len) {
    readDone = true
    Future successful new DefaultLastHttpContent(buf)
  } else {
    Future successful new DefaultHttpContent(buf)
  }
}
```

@scf37 ah got it! May I ask what  level for the leak detector you use ?

Default level. ResourceLeakDetector is on SIMPLE

@scf37 thanks... working on a patch.

@scf37 see #4345

@normanmaurer Assuming you are asking me to review. Done.

@scf37 replied to your comments


Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java b/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
index 7dc375fbb47..08c747dfcf3 100644
--- a/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
+++ b/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
@@ -395,39 +395,46 @@ public static int writeUtf8(ByteBuf buf, CharSequence seq) {
         final int len = seq.length();
         final int maxSize = len * 3;
         buf.ensureWritable(maxSize);
-        if (buf instanceof AbstractByteBuf) {
-            // Fast-Path
-            AbstractByteBuf buffer = (AbstractByteBuf) buf;
-            int oldWriterIndex = buffer.writerIndex;
-            int writerIndex = oldWriterIndex;
-
-            // We can use the _set methods as these not need to do any index checks and reference checks.
-            // This is possible as we called ensureWritable(...) before.
-            for (int i = 0; i < len; i++) {
-                char c = seq.charAt(i);
-                if (c < 0x80) {
-                    buffer._setByte(writerIndex++, (byte) c);
-                } else if (c < 0x800) {
-                    buffer._setByte(writerIndex++, (byte) (0xc0 | (c >> 6)));
-                    buffer._setByte(writerIndex++, (byte) (0x80 | (c & 0x3f)));
-                } else {
-                    buffer._setByte(writerIndex++, (byte) (0xe0 | (c >> 12)));
-                    buffer._setByte(writerIndex++, (byte) (0x80 | ((c >> 6) & 0x3f)));
-                    buffer._setByte(writerIndex++, (byte) (0x80 | (c & 0x3f)));
-                }
+
+        for (;;) {
+            if (buf instanceof AbstractByteBuf) {
+                return writeUtf8((AbstractByteBuf) buf, seq, len);
+            } else if (buf instanceof WrappedByteBuf) {
+                // Unwrap as the wrapped buffer may be an AbstractByteBuf and so we can use fast-path.
+                buf = buf.unwrap();
+            } else {
+                byte[] bytes = seq.toString().getBytes(CharsetUtil.UTF_8);
+                buf.writeBytes(bytes);
+                return bytes.length;
             }
-            // update the writerIndex without any extra checks for performance reasons
-            buffer.writerIndex = writerIndex;
-            return writerIndex - oldWriterIndex;
-        } else {
-            // Maybe we could also check if we can unwrap() to access the wrapped buffer which
-            // may be an AbstractByteBuf. But this may be overkill so let us keep it simple for now.
-            byte[] bytes = seq.toString().getBytes(CharsetUtil.UTF_8);
-            buf.writeBytes(bytes);
-            return bytes.length;
         }
     }
 
+    // Fast-Path implementation
+    private static int writeUtf8(AbstractByteBuf buffer, CharSequence seq, int len) {
+        int oldWriterIndex = buffer.writerIndex;
+        int writerIndex = oldWriterIndex;
+
+        // We can use the _set methods as these not need to do any index checks and reference checks.
+        // This is possible as we called ensureWritable(...) before.
+        for (int i = 0; i < len; i++) {
+            char c = seq.charAt(i);
+            if (c < 0x80) {
+                buffer._setByte(writerIndex++, (byte) c);
+            } else if (c < 0x800) {
+                buffer._setByte(writerIndex++, (byte) (0xc0 | (c >> 6)));
+                buffer._setByte(writerIndex++, (byte) (0x80 | (c & 0x3f)));
+            } else {
+                buffer._setByte(writerIndex++, (byte) (0xe0 | (c >> 12)));
+                buffer._setByte(writerIndex++, (byte) (0x80 | ((c >> 6) & 0x3f)));
+                buffer._setByte(writerIndex++, (byte) (0x80 | (c & 0x3f)));
+            }
+        }
+        // update the writerIndex without any extra checks for performance reasons
+        buffer.writerIndex = writerIndex;
+        return writerIndex - oldWriterIndex;
+    }
+
     /**
      * Encode a {@link CharSequence} in <a href="http://en.wikipedia.org/wiki/ASCII">ASCII</a> and write it
      * to a {@link ByteBuf}.
@@ -444,26 +451,33 @@ public static int writeAscii(ByteBuf buf, CharSequence seq) {
         // ASCII uses 1 byte per char
         final int len = seq.length();
         buf.ensureWritable(len);
-        if (buf instanceof AbstractByteBuf) {
-            // Fast-Path
-            AbstractByteBuf buffer = (AbstractByteBuf) buf;
-            int writerIndex = buffer.writerIndex;
-
-            // We can use the _set methods as these not need to do any index checks and reference checks.
-            // This is possible as we called ensureWritable(...) before.
-            for (int i = 0; i < len; i++) {
-                buffer._setByte(writerIndex++, (byte) seq.charAt(i));
+        for (;;) {
+            if (buf instanceof AbstractByteBuf) {
+                writeAscii((AbstractByteBuf) buf, seq, len);
+                break;
+            } else if (buf instanceof WrappedByteBuf) {
+                // Unwrap as the wrapped buffer may be an AbstractByteBuf and so we can use fast-path.
+                buf = buf.unwrap();
+            } else {
+                buf.writeBytes(seq.toString().getBytes(CharsetUtil.US_ASCII));
             }
-            // update the writerIndex without any extra checks for performance reasons
-            buffer.writerIndex = writerIndex;
-        } else {
-            // Maybe we could also check if we can unwrap() to access the wrapped buffer which
-            // may be an AbstractByteBuf. But this may be overkill so let us keep it simple for now.
-            buf.writeBytes(seq.toString().getBytes(CharsetUtil.US_ASCII));
         }
         return len;
     }
 
+    // Fast-Path implementation
+    private static void writeAscii(AbstractByteBuf buffer, CharSequence seq, int len) {
+        int writerIndex = buffer.writerIndex;
+
+        // We can use the _set methods as these not need to do any index checks and reference checks.
+        // This is possible as we called ensureWritable(...) before.
+        for (int i = 0; i < len; i++) {
+            buffer._setByte(writerIndex++, (byte) seq.charAt(i));
+        }
+        // update the writerIndex without any extra checks for performance reasons
+        buffer.writerIndex = writerIndex;
+    }
+
     /**
      * Encode the given {@link CharBuffer} using the given {@link Charset} into a new {@link ByteBuf} which
      * is allocated via the {@link ByteBufAllocator}.
diff --git a/buffer/src/main/java/io/netty/buffer/WrappedByteBuf.java b/buffer/src/main/java/io/netty/buffer/WrappedByteBuf.java
index e8eb093aa2e..de9f92cb195 100644
--- a/buffer/src/main/java/io/netty/buffer/WrappedByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/WrappedByteBuf.java
@@ -27,6 +27,13 @@
 import java.nio.channels.ScatteringByteChannel;
 import java.nio.charset.Charset;
 
+/**
+ * Wraps another {@link ByteBuf}.
+ *
+ * It's important that the {@link #readerIndex()} and {@link #writerIndex()} will not do any adjustments on the
+ * indices on the fly because of internal optimizations made by {@link ByteBufUtil#writeAscii(ByteBuf, CharSequence)}
+ * and {@link ByteBufUtil#writeUtf8(ByteBuf, CharSequence)}.
+ */
 class WrappedByteBuf extends ByteBuf {
 
     protected final ByteBuf buf;
@@ -39,17 +46,17 @@ protected WrappedByteBuf(ByteBuf buf) {
     }
 
     @Override
-    public boolean hasMemoryAddress() {
+    public final boolean hasMemoryAddress() {
         return buf.hasMemoryAddress();
     }
 
     @Override
-    public long memoryAddress() {
+    public final long memoryAddress() {
         return buf.memoryAddress();
     }
 
     @Override
-    public int capacity() {
+    public final int capacity() {
         return buf.capacity();
     }
 
@@ -60,17 +67,17 @@ public ByteBuf capacity(int newCapacity) {
     }
 
     @Override
-    public int maxCapacity() {
+    public final int maxCapacity() {
         return buf.maxCapacity();
     }
 
     @Override
-    public ByteBufAllocator alloc() {
+    public final ByteBufAllocator alloc() {
         return buf.alloc();
     }
 
     @Override
-    public ByteOrder order() {
+    public final ByteOrder order() {
         return buf.order();
     }
 
@@ -80,33 +87,33 @@ public ByteBuf order(ByteOrder endianness) {
     }
 
     @Override
-    public ByteBuf unwrap() {
+    public final ByteBuf unwrap() {
         return buf;
     }
 
     @Override
-    public boolean isDirect() {
+    public final boolean isDirect() {
         return buf.isDirect();
     }
 
     @Override
-    public int readerIndex() {
+    public final int readerIndex() {
         return buf.readerIndex();
     }
 
     @Override
-    public ByteBuf readerIndex(int readerIndex) {
+    public final ByteBuf readerIndex(int readerIndex) {
         buf.readerIndex(readerIndex);
         return this;
     }
 
     @Override
-    public int writerIndex() {
+    public final int writerIndex() {
         return buf.writerIndex();
     }
 
     @Override
-    public ByteBuf writerIndex(int writerIndex) {
+    public final ByteBuf writerIndex(int writerIndex) {
         buf.writerIndex(writerIndex);
         return this;
     }
@@ -118,56 +125,56 @@ public ByteBuf setIndex(int readerIndex, int writerIndex) {
     }
 
     @Override
-    public int readableBytes() {
+    public final int readableBytes() {
         return buf.readableBytes();
     }
 
     @Override
-    public int writableBytes() {
+    public final int writableBytes() {
         return buf.writableBytes();
     }
 
     @Override
-    public int maxWritableBytes() {
+    public final int maxWritableBytes() {
         return buf.maxWritableBytes();
     }
 
     @Override
-    public boolean isReadable() {
+    public final boolean isReadable() {
         return buf.isReadable();
     }
 
     @Override
-    public boolean isWritable() {
+    public final boolean isWritable() {
         return buf.isWritable();
     }
 
     @Override
-    public ByteBuf clear() {
+    public final ByteBuf clear() {
         buf.clear();
         return this;
     }
 
     @Override
-    public ByteBuf markReaderIndex() {
+    public final ByteBuf markReaderIndex() {
         buf.markReaderIndex();
         return this;
     }
 
     @Override
-    public ByteBuf resetReaderIndex() {
+    public final ByteBuf resetReaderIndex() {
         buf.resetReaderIndex();
         return this;
     }
 
     @Override
-    public ByteBuf markWriterIndex() {
+    public final ByteBuf markWriterIndex() {
         buf.markWriterIndex();
         return this;
     }
 
     @Override
-    public ByteBuf resetWriterIndex() {
+    public final ByteBuf resetWriterIndex() {
         buf.resetWriterIndex();
         return this;
     }
@@ -801,17 +808,17 @@ public ByteBuf retain() {
     }
 
     @Override
-    public boolean isReadable(int size) {
+    public final boolean isReadable(int size) {
         return buf.isReadable(size);
     }
 
     @Override
-    public boolean isWritable(int size) {
+    public final boolean isWritable(int size) {
         return buf.isWritable(size);
     }
 
     @Override
-    public int refCnt() {
+    public final int refCnt() {
         return buf.refCnt();
     }
 


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java b/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java
index fb04c78ce71..1725fb525ac 100644
--- a/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java
+++ b/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java
@@ -33,6 +33,19 @@ public void testWriteUsAscii() {
         Assert.assertEquals(buf, buf2);
     }
 
+    @Test
+    public void testWriteUsAsciiWrapped() {
+        String usAscii = "NettyRocks";
+        ByteBuf buf = Unpooled.unreleasableBuffer(ReferenceCountUtil.releaseLater(Unpooled.buffer(16)));
+        assertWrapped(buf);
+        buf.writeBytes(usAscii.getBytes(CharsetUtil.US_ASCII));
+        ByteBuf buf2 = Unpooled.unreleasableBuffer(ReferenceCountUtil.releaseLater(Unpooled.buffer(16)));
+        assertWrapped(buf2);
+        ByteBufUtil.writeAscii(buf2, usAscii);
+
+        Assert.assertEquals(buf, buf2);
+    }
+
     @Test
     public void testWriteUtf8() {
         String usAscii = "Some UTF-8 like äÄ∏ŒŒ";
@@ -43,4 +56,21 @@ public void testWriteUtf8() {
 
         Assert.assertEquals(buf, buf2);
     }
+
+    @Test
+    public void testWriteUtf8Wrapped() {
+        String usAscii = "Some UTF-8 like äÄ∏ŒŒ";
+        ByteBuf buf = Unpooled.unreleasableBuffer(ReferenceCountUtil.releaseLater(Unpooled.buffer(16)));
+        assertWrapped(buf);
+        buf.writeBytes(usAscii.getBytes(CharsetUtil.UTF_8));
+        ByteBuf buf2 = Unpooled.unreleasableBuffer(ReferenceCountUtil.releaseLater(Unpooled.buffer(16)));
+        assertWrapped(buf2);
+        ByteBufUtil.writeUtf8(buf2, usAscii);
+
+        Assert.assertEquals(buf, buf2);
+    }
+
+    private static void assertWrapped(ByteBuf buf) {
+        Assert.assertTrue(buf instanceof WrappedByteBuf);
+    }
 }