From e542a2cf26774885a87014f8b47ea22aa8157da0 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 7 Sep 2018 07:47:02 +0200 Subject: [PATCH] Use a non-volatile read for ensureAccessible() whenever possible to reduce overhead and allow better inlining. (#8266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Motiviation: At the moment whenever ensureAccessible() is called in our ByteBuf implementations (which is basically on each operation) we will do a volatile read. That per-se is not such a bad thing but the problem here is that it will also reduce the the optimizations that the compiler / jit can do. For example as these are volatile it can not eliminate multiple loads of it when inline the methods of ByteBuf which happens quite frequently because most of them a quite small and very hot. That is especially true for all the methods that act on primitives. It gets even worse as people often call a lot of these after each other in the same method or even use method chaining here. The idea of the change is basically just ue a non-volatile read for the ensureAccessible() check as its a best-effort implementation to detect acting on already released buffers anyway as even with a volatile read it could happen that the user will release it in another thread before we actual access the buffer after the reference check. Modifications: - Try to do a non-volatile read using sun.misc.Unsafe if we can use it. - Add a benchmark Result: Big performance win when multiple ByteBuf methods are called from a method. With the change: UnsafeByteBufBenchmark.setGetLongUnsafeByteBuf thrpt 20 281395842,128 ± 5050792,296 ops/s Before the change: UnsafeByteBufBenchmark.setGetLongUnsafeByteBuf thrpt 20 217419832,801 ± 5080579,030 ops/s --- .../java/io/netty/buffer/AbstractByteBuf.java | 10 ++- .../AbstractReferenceCountedByteBuf.java | 29 ++++++++- .../netty/buffer/WrappedCompositeByteBuf.java | 5 ++ .../buffer/UnsafeByteBufBenchmark.java | 64 +++++++++++++++++++ 4 files changed, 105 insertions(+), 3 deletions(-) create mode 100644 microbench/src/main/java/io/netty/microbench/buffer/UnsafeByteBufBenchmark.java diff --git a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java index c8c40a5d24..1fe1dbe272 100644 --- a/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java @@ -1442,11 +1442,19 @@ public abstract class AbstractByteBuf extends ByteBuf { * if the buffer was released before. */ protected final void ensureAccessible() { - if (checkAccessible && refCnt() == 0) { + if (checkAccessible && internalRefCnt() == 0) { throw new IllegalReferenceCountException(0); } } + /** + * Returns the reference count that is used internally by {@link #ensureAccessible()} to try to guard + * against using the buffer after it was released (best-effort). + */ + int internalRefCnt() { + return refCnt(); + } + final void setIndex0(int readerIndex, int writerIndex) { this.readerIndex = readerIndex; this.writerIndex = writerIndex; diff --git a/buffer/src/main/java/io/netty/buffer/AbstractReferenceCountedByteBuf.java b/buffer/src/main/java/io/netty/buffer/AbstractReferenceCountedByteBuf.java index d624d855f4..84de93fab6 100644 --- a/buffer/src/main/java/io/netty/buffer/AbstractReferenceCountedByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/AbstractReferenceCountedByteBuf.java @@ -17,6 +17,7 @@ package io.netty.buffer; import io.netty.util.IllegalReferenceCountException; +import io.netty.util.internal.PlatformDependent; import java.util.concurrent.atomic.AtomicIntegerFieldUpdater; @@ -26,17 +27,40 @@ import static io.netty.util.internal.ObjectUtil.checkPositive; * Abstract base class for {@link ByteBuf} implementations that count references. */ public abstract class AbstractReferenceCountedByteBuf extends AbstractByteBuf { - + private static final long REFCNT_FIELD_OFFSET; private static final AtomicIntegerFieldUpdater refCntUpdater = AtomicIntegerFieldUpdater.newUpdater(AbstractReferenceCountedByteBuf.class, "refCnt"); private volatile int refCnt; + static { + long refCntFieldOffset = -1; + try { + if (PlatformDependent.hasUnsafe()) { + refCntFieldOffset = PlatformDependent.objectFieldOffset( + AbstractReferenceCountedByteBuf.class.getDeclaredField("refCnt")); + } + } catch (Throwable ignore) { + refCntFieldOffset = -1; + } + + REFCNT_FIELD_OFFSET = refCntFieldOffset; + } + protected AbstractReferenceCountedByteBuf(int maxCapacity) { super(maxCapacity); refCntUpdater.set(this, 1); } + @Override + int internalRefCnt() { + // Try to do non-volatile read for performance as the ensureAccessible() is racy anyway and only provide + // a best-effort guard. + // + // TODO: Once we compile against later versions of Java we can replace the Unsafe usage here by varhandles. + return REFCNT_FIELD_OFFSET != -1 ? PlatformDependent.getInt(this, REFCNT_FIELD_OFFSET) : refCnt(); + } + @Override public int refCnt() { return refCnt; @@ -94,7 +118,8 @@ public abstract class AbstractReferenceCountedByteBuf extends AbstractByteBuf { if (oldRef == decrement) { deallocate(); return true; - } else if (oldRef < decrement || oldRef - decrement > oldRef) { + } + if (oldRef < decrement || oldRef - decrement > oldRef) { // Ensure we don't over-release, and avoid underflow. refCntUpdater.getAndAdd(this, decrement); throw new IllegalReferenceCountException(oldRef, -decrement); diff --git a/buffer/src/main/java/io/netty/buffer/WrappedCompositeByteBuf.java b/buffer/src/main/java/io/netty/buffer/WrappedCompositeByteBuf.java index 8f5161620f..b124eb2d2b 100644 --- a/buffer/src/main/java/io/netty/buffer/WrappedCompositeByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/WrappedCompositeByteBuf.java @@ -423,6 +423,11 @@ class WrappedCompositeByteBuf extends CompositeByteBuf { return wrapped.refCnt(); } + @Override + int internalRefCnt() { + return wrapped.internalRefCnt(); + } + @Override public ByteBuf duplicate() { return wrapped.duplicate(); diff --git a/microbench/src/main/java/io/netty/microbench/buffer/UnsafeByteBufBenchmark.java b/microbench/src/main/java/io/netty/microbench/buffer/UnsafeByteBufBenchmark.java new file mode 100644 index 0000000000..435feb61d8 --- /dev/null +++ b/microbench/src/main/java/io/netty/microbench/buffer/UnsafeByteBufBenchmark.java @@ -0,0 +1,64 @@ +/* +* Copyright 2018 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.UnpooledByteBufAllocator; +import io.netty.buffer.UnpooledUnsafeDirectByteBuf; +import io.netty.microbench.util.AbstractMicrobenchmark; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.TearDown; + +import java.nio.ByteBuffer; + + +public class UnsafeByteBufBenchmark extends AbstractMicrobenchmark { + + private ByteBuf unsafeBuffer; + private ByteBuffer byteBuffer; + + @Setup + public void setup() { + unsafeBuffer = new UnpooledUnsafeDirectByteBuf(UnpooledByteBufAllocator.DEFAULT, 64, 64); + byteBuffer = ByteBuffer.allocateDirect(64); + } + + @TearDown + public void tearDown() { + unsafeBuffer.release(); + } + + @Benchmark + public long setGetLongUnsafeByteBuf() { + return unsafeBuffer.setLong(0, 1).getLong(0); + } + + @Benchmark + public long setGetLongByteBuffer() { + return byteBuffer.putLong(0, 1).getLong(0); + } + + @Benchmark + public ByteBuf setLongUnsafeByteBuf() { + return unsafeBuffer.setLong(0, 1); + } + + @Benchmark + public ByteBuffer setLongByteBuffer() { + return byteBuffer.putLong(0, 1); + } +}