Repository: netty/netty
Issue: ByteBuf implementation should only invert bytes if ByteOrder differ from native ByteOrder
Labels: improvement

@pavanka made me aware of an issue with how we handle native ByteOrder and endianess. Basically we always assume that native ByteOrder is BIG_ENDIAN and so we end up invert bytes when ByteOrder.LITTLE_ENDIAN is set on the ByteBuf and the native ByteOrder is LITTLE_ENDIAN.

We should change our implementation to act more like what ByteBuffer does which is only invert bytes if it the ByteOrder of the ByteBuf differs from the native ByteOrder.

``` java
  //java/nio/ByteBuffer.java
  boolean bigEndian   = true;
  boolean nativeByteOrder  = (Bits.byteOrder() == ByteOrder.BIG_ENDIAN);        
  // default is set as bigEndian, however if we are running on a littleEndian
  // machines then Bits.byteOrder() == ByteOrder.LITTLE_ENDIAN so
  // nativeOrder = false, till here behavior is the same in netty as well

  // now say I set order(LITTLE_ENDIAN) then per this code from
  // ByteBuffer.java,   nativeByteOrder = true so no swapping in code underneath
  public final ByteBuffer order(ByteOrder bo) {
      bigEndian = (bo == ByteOrder.BIG_ENDIAN);
      nativeByteOrder =
          (bigEndian == (Bits.byteOrder() == ByteOrder.BIG_ENDIAN));
      return this;
  }

  //java/nio/DirectByteBuffer.java
  //even on a little endian machine nativeOrder = true so no bits swap
  private ByteBuffer putInt(long a, int x) {
      if (unaligned) {
          int y = (x);
          unsafe.putInt(a, (nativeByteOrder ? y : Bits.swap(y)));
      } else {
          Bits.putInt(a, x, bigEndian);
      }
      return this;
  }
```

Compare this with Netty:

``` java
  /io/netty/buffer/PooledUnsafeDirectByteBuf.java
  private static final boolean NATIVE_ORDER = ByteOrder.nativeOrder() ==   ByteOrder.BIG_ENDIAN;
  // in netty however NATIVE_ORDER is always false on little endian
  // machines so underneath

  // since NATIVE_ORDER is always false so bytes are always reversed.
  @Override
  protected void _setInt(int index, int value) {
      PlatformDependent.putInt(addr(index), NATIVE_ORDER ? value : Integer.reverseBytes(value));
  }

  // even setting buf.order() is not of much help because default is always
  // assumed to be big-endian so the buffer is wrapped by a swapped
  //bytebuf, so any writes are always swapped
  @Override
  public ByteBuf order(ByteOrder endianness) {
      if (endianness == null) {
          throw new NullPointerException("endianness");
      }
      if (endianness == order()) {
          return this;
      }

      SwappedByteBuf swappedBuf = this.swappedBuf;
      if (swappedBuf == null) {
          this.swappedBuf = swappedBuf = new SwappedByteBuf(this);
      }
      return swappedBuf;
  }

  @Override
  public ByteOrder order() {
      return ByteOrder.BIG_ENDIAN;
  }
```


Issue comments:
Yeah, I was quite lazy when writing this.  Another food for thought is: do we really need a swapped buffer? Why don't we just add a little endian getters and setters into `ByteBuf`?  It will make it a little bit crowded but it actually should simplify things.

@trustin lol :) What you mean with getter and setter here ? Just allow to change the endianess on the fly (like what ByteBuffer does) ?

Nope. Just having additional methods:

```
ByteBuf buf = ...;
int bigEndianValue = buf.readInt();
int littleEndialValue = buf.readIntL();
```

sounds legit... This would also reduce the GC pressure as we can eliminate the extra object creation for the SwappedByteBuf. But I think we can only do this for 4.1 and master. I would like to come up with some "fix/workaround" for 4.0 too. 

Go ahead.

@trustin lol yeah... any preferences how to fix this for 4.0.x ? (I can also come up with some PR based on your idea for 4.1 and master).

I prefer to leave 4.0 as is unless it's a critical problem (you can define 'critical' by yourself :-))

All the power for me :) I will think about it buddy and come up with something for 4.1 and master in the meantime.

@pavanka I just pushed a commit into 4.0, 4.1 and master branches. This should be part of the next 4.0.20.Final release. Thanks for reporting !


Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java
index 1ec4afa71fa..92e4a471bdf 100644
--- a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java
@@ -38,7 +38,7 @@ public abstract class AbstractByteBuf extends ByteBuf {
     static final ResourceLeakDetector<ByteBuf> leakDetector = new ResourceLeakDetector<ByteBuf>(ByteBuf.class);
 
     int readerIndex;
-    private int writerIndex;
+    int writerIndex;
     private int markedReaderIndex;
     private int markedWriterIndex;
 
@@ -321,11 +321,18 @@ public ByteBuf order(ByteOrder endianness) {
 
         SwappedByteBuf swappedBuf = this.swappedBuf;
         if (swappedBuf == null) {
-            this.swappedBuf = swappedBuf = new SwappedByteBuf(this);
+            this.swappedBuf = swappedBuf = newSwappedByteBuf();
         }
         return swappedBuf;
     }
 
+    /**
+     * Creates a new {@link SwappedByteBuf} for this {@link ByteBuf} instance.
+     */
+    protected SwappedByteBuf newSwappedByteBuf() {
+        return new SwappedByteBuf(this);
+    }
+
     @Override
     public byte getByte(int index) {
         checkIndex(index);
diff --git a/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java b/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java
index 4d15509021c..795335e4c35 100644
--- a/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java
@@ -381,4 +381,9 @@ public long memoryAddress() {
     private long addr(int index) {
         return memoryAddress + index;
     }
+
+    @Override
+    protected SwappedByteBuf newSwappedByteBuf() {
+        return new UnsafeDirectSwappedByteBuf(this, memoryAddress);
+    }
 }
diff --git a/buffer/src/main/java/io/netty/buffer/SwappedByteBuf.java b/buffer/src/main/java/io/netty/buffer/SwappedByteBuf.java
index a11538117eb..3c917c4f849 100644
--- a/buffer/src/main/java/io/netty/buffer/SwappedByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/SwappedByteBuf.java
@@ -27,7 +27,7 @@
 /**
  * Wrapper which swap the {@link ByteOrder} of a {@link ByteBuf}.
  */
-public final class SwappedByteBuf extends ByteBuf {
+public class SwappedByteBuf extends ByteBuf {
 
     private final ByteBuf buf;
     private final ByteOrder order;
diff --git a/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java b/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java
index 89df31acde7..9b6cb76f78c 100644
--- a/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java
@@ -514,4 +514,9 @@ public ByteBuf unwrap() {
     long addr(int index) {
         return memoryAddress + index;
     }
+
+    @Override
+    protected SwappedByteBuf newSwappedByteBuf() {
+        return new UnsafeDirectSwappedByteBuf(this, memoryAddress);
+    }
 }
diff --git a/buffer/src/main/java/io/netty/buffer/UnsafeDirectSwappedByteBuf.java b/buffer/src/main/java/io/netty/buffer/UnsafeDirectSwappedByteBuf.java
new file mode 100644
index 00000000000..5c346f38943
--- /dev/null
+++ b/buffer/src/main/java/io/netty/buffer/UnsafeDirectSwappedByteBuf.java
@@ -0,0 +1,181 @@
+/*
+* Copyright 2014 The Netty Project
+*
+* The Netty Project licenses this file to you under the Apache License,
+* version 2.0 (the "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at:
+*
+*   http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+* License for the specific language governing permissions and limitations
+* under the License.
+*/
+
+package io.netty.buffer;
+
+import io.netty.util.internal.PlatformDependent;
+
+import java.nio.ByteOrder;
+
+/**
+ * Special {@link SwappedByteBuf} for {@link ByteBuf}s that are backed by a {@code memoryAddress}.
+ */
+final class UnsafeDirectSwappedByteBuf extends SwappedByteBuf {
+    private static final boolean NATIVE_ORDER = ByteOrder.nativeOrder() == ByteOrder.BIG_ENDIAN;
+    private final boolean nativeByteOrder;
+    private final AbstractByteBuf wrapped;
+    private final long memoryAddress;
+
+    UnsafeDirectSwappedByteBuf(AbstractByteBuf buf, long memoryAddress) {
+        super(buf);
+        wrapped = buf;
+        this.memoryAddress = memoryAddress;
+        nativeByteOrder = NATIVE_ORDER == (order() == ByteOrder.BIG_ENDIAN);
+    }
+
+    private long addr(int index) {
+        return memoryAddress + index;
+    }
+
+    @Override
+    public long getLong(int index) {
+        wrapped.checkIndex(index, 8);
+        long v = PlatformDependent.getLong(addr(index));
+        return nativeByteOrder? v : Long.reverseBytes(v);
+    }
+
+    @Override
+    public float getFloat(int index) {
+        return Float.intBitsToFloat(getInt(index));
+    }
+
+    @Override
+    public double getDouble(int index) {
+        return Double.longBitsToDouble(getLong(index));
+    }
+
+    @Override
+    public char getChar(int index) {
+        return (char) getShort(index);
+    }
+
+    @Override
+    public long getUnsignedInt(int index) {
+        return getInt(index) & 0xFFFFFFFFL;
+    }
+
+    @Override
+    public int getInt(int index) {
+        wrapped.checkIndex(index, 4);
+        int v = PlatformDependent.getInt(addr(index));
+        return nativeByteOrder? v : Integer.reverseBytes(v);
+    }
+
+    @Override
+    public int getUnsignedShort(int index) {
+        return getShort(index) & 0xFFFF;
+    }
+
+    @Override
+    public short getShort(int index) {
+        wrapped.checkIndex(index, 2);
+        short v = PlatformDependent.getShort(addr(index));
+        return nativeByteOrder? v : Short.reverseBytes(v);
+    }
+
+    @Override
+    public ByteBuf setShort(int index, int value) {
+        wrapped.checkIndex(index, 2);
+        _setShort(index, value);
+        return this;
+    }
+
+    @Override
+    public ByteBuf setInt(int index, int value) {
+        wrapped.checkIndex(index, 4);
+        _setInt(index, value);
+        return this;
+    }
+
+    @Override
+    public ByteBuf setLong(int index, long value) {
+        wrapped.checkIndex(index, 8);
+        _setLong(index, value);
+        return this;
+    }
+
+    @Override
+    public ByteBuf setChar(int index, int value) {
+        setShort(index, value);
+        return this;
+    }
+
+    @Override
+    public ByteBuf setFloat(int index, float value) {
+        setInt(index, Float.floatToRawIntBits(value));
+        return this;
+    }
+
+    @Override
+    public ByteBuf setDouble(int index, double value) {
+        setLong(index, Double.doubleToRawLongBits(value));
+        return this;
+    }
+
+    @Override
+    public ByteBuf writeShort(int value) {
+        wrapped.ensureWritable(2);
+        _setShort(wrapped.writerIndex, value);
+        wrapped.writerIndex += 2;
+        return this;
+    }
+
+    @Override
+    public ByteBuf writeInt(int value) {
+        wrapped.ensureWritable(4);
+        _setInt(wrapped.writerIndex, value);
+        wrapped.writerIndex += 4;
+        return this;
+    }
+
+    @Override
+    public ByteBuf writeLong(long value) {
+        wrapped.ensureWritable(8);
+        _setLong(wrapped.writerIndex, value);
+        wrapped.writerIndex += 8;
+        return this;
+    }
+
+    @Override
+    public ByteBuf writeChar(int value) {
+        writeShort(value);
+        return this;
+    }
+
+    @Override
+    public ByteBuf writeFloat(float value) {
+        writeInt(Float.floatToRawIntBits(value));
+        return this;
+    }
+
+    @Override
+    public ByteBuf writeDouble(double value) {
+        writeLong(Double.doubleToRawLongBits(value));
+        return this;
+    }
+
+    private void _setShort(int index, int value) {
+        PlatformDependent.putShort(addr(index), nativeByteOrder ? (short) value : Short.reverseBytes((short) value));
+    }
+
+    private void _setInt(int index, int value) {
+        PlatformDependent.putInt(addr(index), nativeByteOrder ? value : Integer.reverseBytes(value));
+    }
+
+    private void _setLong(int index, long value) {
+        PlatformDependent.putLong(addr(index), nativeByteOrder ? value : Long.reverseBytes(value));
+    }
+}
diff --git a/microbench/pom.xml b/microbench/pom.xml
index cdf7a0e9bee..be9b2e27825 100644
--- a/microbench/pom.xml
+++ b/microbench/pom.xml
@@ -47,7 +47,13 @@
     <dependency>
       <groupId>org.openjdk.jmh</groupId>
       <artifactId>jmh-core</artifactId>
-      <version>0.4.1</version>
+      <version>0.8</version>
+    </dependency>
+    <dependency>
+      <groupId>org.openjdk.jmh</groupId>
+      <artifactId>jmh-generator-annprocess</artifactId>
+      <version>0.8</version>
+      <scope>provided</scope>
     </dependency>
   </dependencies>
 


Test patch:
diff --git a/microbench/src/test/java/io/netty/microbench/buffer/SwappedByteBufBenchmark.java b/microbench/src/test/java/io/netty/microbench/buffer/SwappedByteBufBenchmark.java
new file mode 100644
index 00000000000..e359a6c23cf
--- /dev/null
+++ b/microbench/src/test/java/io/netty/microbench/buffer/SwappedByteBufBenchmark.java
@@ -0,0 +1,80 @@
+/*
+* Copyright 2014 The Netty Project
+*
+* The Netty Project licenses this file to you under the Apache License,
+* version 2.0 (the "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at:
+*
+*   http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+* License for the specific language governing permissions and limitations
+* under the License.
+*/
+package io.netty.microbench.buffer;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.SwappedByteBuf;
+import io.netty.buffer.Unpooled;
+import io.netty.microbench.util.AbstractMicrobenchmark;
+import org.openjdk.jmh.annotations.GenerateMicroBenchmark;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.Warmup;
+
+import java.nio.ByteOrder;
+
+@State(Scope.Benchmark)
+@Warmup(iterations = 10)
+@Measurement(iterations = 25)
+public class SwappedByteBufBenchmark extends AbstractMicrobenchmark {
+    private ByteBuf swappedByteBuf;
+    private ByteBuf unsafeSwappedByteBuf;
+
+    @Setup
+    public void setup() {
+        swappedByteBuf = new SwappedByteBuf(Unpooled.directBuffer(8));
+        unsafeSwappedByteBuf = Unpooled.directBuffer(8).order(ByteOrder.LITTLE_ENDIAN);
+        if (unsafeSwappedByteBuf.getClass().equals(SwappedByteBuf.class)) {
+            throw new IllegalStateException("Should not use " + SwappedByteBuf.class.getSimpleName());
+        }
+    }
+
+    @Param("16384")
+    public int size;
+
+    @GenerateMicroBenchmark
+    public void swappedByteBufSetInt() {
+        swappedByteBuf.setLong(0, size);
+    }
+
+    @GenerateMicroBenchmark
+    public void swappedByteBufSetShort() {
+        swappedByteBuf.setShort(0, size);
+    }
+
+    @GenerateMicroBenchmark
+    public void swappedByteBufSetLong() {
+        swappedByteBuf.setLong(0, size);
+    }
+
+    @GenerateMicroBenchmark
+    public void unsafeSwappedByteBufSetInt() {
+        unsafeSwappedByteBuf.setInt(0, size);
+    }
+
+    @GenerateMicroBenchmark
+    public void unsafeSwappedByteBufSetShort() {
+        unsafeSwappedByteBuf.setShort(0, size);
+    }
+
+    @GenerateMicroBenchmark
+    public void unsafeSwappedByteBufSetLong() {
+        unsafeSwappedByteBuf.setLong(0, size);
+    }
+}