Use a non-volatile read for ensureAccessible() whenever possible to reduce overhead and allow better inlining. (#8266)

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
This commit is contained in:
Norman Maurer 2018-09-07 07:47:02 +02:00 committed by GitHub
parent afe0767e9c
commit e542a2cf26
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 105 additions and 3 deletions

View File

@ -1442,11 +1442,19 @@ public abstract class AbstractByteBuf extends ByteBuf {
* if the buffer was released before. * if the buffer was released before.
*/ */
protected final void ensureAccessible() { protected final void ensureAccessible() {
if (checkAccessible && refCnt() == 0) { if (checkAccessible && internalRefCnt() == 0) {
throw new IllegalReferenceCountException(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) { final void setIndex0(int readerIndex, int writerIndex) {
this.readerIndex = readerIndex; this.readerIndex = readerIndex;
this.writerIndex = writerIndex; this.writerIndex = writerIndex;

View File

@ -17,6 +17,7 @@
package io.netty.buffer; package io.netty.buffer;
import io.netty.util.IllegalReferenceCountException; import io.netty.util.IllegalReferenceCountException;
import io.netty.util.internal.PlatformDependent;
import java.util.concurrent.atomic.AtomicIntegerFieldUpdater; 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. * Abstract base class for {@link ByteBuf} implementations that count references.
*/ */
public abstract class AbstractReferenceCountedByteBuf extends AbstractByteBuf { public abstract class AbstractReferenceCountedByteBuf extends AbstractByteBuf {
private static final long REFCNT_FIELD_OFFSET;
private static final AtomicIntegerFieldUpdater<AbstractReferenceCountedByteBuf> refCntUpdater = private static final AtomicIntegerFieldUpdater<AbstractReferenceCountedByteBuf> refCntUpdater =
AtomicIntegerFieldUpdater.newUpdater(AbstractReferenceCountedByteBuf.class, "refCnt"); AtomicIntegerFieldUpdater.newUpdater(AbstractReferenceCountedByteBuf.class, "refCnt");
private volatile int 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) { protected AbstractReferenceCountedByteBuf(int maxCapacity) {
super(maxCapacity); super(maxCapacity);
refCntUpdater.set(this, 1); 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 @Override
public int refCnt() { public int refCnt() {
return refCnt; return refCnt;
@ -94,7 +118,8 @@ public abstract class AbstractReferenceCountedByteBuf extends AbstractByteBuf {
if (oldRef == decrement) { if (oldRef == decrement) {
deallocate(); deallocate();
return true; return true;
} else if (oldRef < decrement || oldRef - decrement > oldRef) { }
if (oldRef < decrement || oldRef - decrement > oldRef) {
// Ensure we don't over-release, and avoid underflow. // Ensure we don't over-release, and avoid underflow.
refCntUpdater.getAndAdd(this, decrement); refCntUpdater.getAndAdd(this, decrement);
throw new IllegalReferenceCountException(oldRef, -decrement); throw new IllegalReferenceCountException(oldRef, -decrement);

View File

@ -423,6 +423,11 @@ class WrappedCompositeByteBuf extends CompositeByteBuf {
return wrapped.refCnt(); return wrapped.refCnt();
} }
@Override
int internalRefCnt() {
return wrapped.internalRefCnt();
}
@Override @Override
public ByteBuf duplicate() { public ByteBuf duplicate() {
return wrapped.duplicate(); return wrapped.duplicate();

View File

@ -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);
}
}