diff --git a/hbase-benchmarks/src/main/java/org/apache/hadoop/hbase/util/ByteBuffBenchmark.java b/hbase-benchmarks/src/main/java/org/apache/hadoop/hbase/util/ByteBuffBenchmark.java index d0c8aab3e8..50ad5ec4ab 100644 --- a/hbase-benchmarks/src/main/java/org/apache/hadoop/hbase/util/ByteBuffBenchmark.java +++ b/hbase-benchmarks/src/main/java/org/apache/hadoop/hbase/util/ByteBuffBenchmark.java @@ -5,7 +5,6 @@ import java.util.concurrent.TimeUnit; import org.apache.hadoop.hbase.io.ByteBuffAllocator; import org.apache.hadoop.hbase.nio.ByteBuff; import org.apache.hadoop.hbase.nio.SingleByteBuff; -import org.apache.hadoop.io.WritableUtils; import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.BenchmarkMode; import org.openjdk.jmh.annotations.Fork; diff --git a/hbase-benchmarks/src/main/java/org/apache/hadoop/hbase/util/ReadVLongBenchmark.java b/hbase-benchmarks/src/main/java/org/apache/hadoop/hbase/util/ReadVLongBenchmark.java index 5f09dce2c3..307ada2497 100644 --- a/hbase-benchmarks/src/main/java/org/apache/hadoop/hbase/util/ReadVLongBenchmark.java +++ b/hbase-benchmarks/src/main/java/org/apache/hadoop/hbase/util/ReadVLongBenchmark.java @@ -25,7 +25,6 @@ import org.openjdk.jmh.runner.options.OptionsBuilder; @State(Scope.Thread) public class ReadVLongBenchmark { - //@Param({"9", "512", "80000", "2146483640", "548755813887", "1700104028981", "35028797018963967", "9123372036854775807"}) @Param({"9", "512", "80000", "548755813887", "1700104028981", "9123372036854775807"}) public long vlong; @@ -79,62 +78,6 @@ public class ReadVLongBenchmark { OFF_HEAP_PADDED.mark(); } - /*@Benchmark - public void readVLong_OnHeapBB(Blackhole blackhole) { - blackhole.consume(readVLong(ON_HEAP_BB)); - - ON_HEAP.reset(); - } - - @Benchmark - public void readVLong_OffHeapBB(Blackhole blackhole) { - blackhole.consume(readVLong(OFF_HEAP_BB)); - - OFF_HEAP.reset(); - } - - @Benchmark - public void readVLong_OnHeapBB_Padded(Blackhole blackhole) { - blackhole.consume(readVLong(ON_HEAP_PADDED_BB)); - - ON_HEAP_PADDED.reset(); - } - - @Benchmark - public void readVLong_OffHeapBB_Padded(Blackhole blackhole) { - blackhole.consume(readVLong(OFF_HEAP_PADDED_BB)); - - OFF_HEAP_PADDED.reset(); - } - - @Benchmark - public void readVLongTimestamp_OnHeapBB(Blackhole blackhole) { - blackhole.consume(readVLongTimestamp(ON_HEAP_BB)); - - ON_HEAP.reset(); - } - - @Benchmark - public void readVLongTimestamp_OffHeapBB(Blackhole blackhole) { - blackhole.consume(readVLongTimestamp(OFF_HEAP_BB)); - - OFF_HEAP.reset(); - } - - @Benchmark - public void readVLongTimestamp_OnHeapBB_Padded(Blackhole blackhole) { - blackhole.consume(readVLongTimestamp(ON_HEAP_PADDED_BB)); - - ON_HEAP_PADDED.reset(); - } - - @Benchmark - public void readVLongTimestamp_OffHeapBB_Padded(Blackhole blackhole) { - blackhole.consume(readVLongTimestamp(OFF_HEAP_PADDED_BB)); - - OFF_HEAP_PADDED.reset(); - }*/ - @Benchmark public void readVLongHBase14186_OnHeapBB(Blackhole blackhole) { blackhole.consume(readVLongHBase14186(ON_HEAP_BB)); @@ -149,20 +92,6 @@ public class ReadVLongBenchmark { OFF_HEAP.reset(); } - /*@Benchmark - public void readVLongHBase14186_OnHeapBB_Padded(Blackhole blackhole) { - blackhole.consume(readVLongHBase14186(ON_HEAP_PADDED_BB)); - - ON_HEAP_PADDED.reset(); - } - - @Benchmark - public void readVLongHBase14186_OffHeapBB_Padded(Blackhole blackhole) { - blackhole.consume(readVLongHBase14186(OFF_HEAP_PADDED_BB)); - - OFF_HEAP_PADDED.reset(); - }*/ - @TearDown public void tearDown() { ON_HEAP_PADDED_BB.release(); @@ -178,34 +107,6 @@ public class ReadVLongBenchmark { new Runner(opt).run(); } - private interface ByteVisitor { - byte get(); - } - - private static long readVLong(ByteVisitor visitor) { - byte firstByte = visitor.get(); - int len = WritableUtils.decodeVIntSize(firstByte); - if (len == 1) { - return firstByte; - } - long i = 0; - for (int idx = 0; idx < len - 1; idx++) { - byte b = visitor.get(); - i = i << 8; - i = i | (b & 0xFF); - } - return (WritableUtils.isNegativeVInt(firstByte) ? (i ^ -1L) : i); - } - - - /** - * Similar to {@link WritableUtils#readVLong(java.io.DataInput)} but reads from a - * {@link ByteBuff}. - */ - public static long readVLong(ByteBuff in) { - return readVLong(in::get); - } - public static long readVLongHBase14186(ByteBuff buf) { byte firstByte = buf.get(); int len = WritableUtils.decodeVIntSize(firstByte); @@ -237,33 +138,4 @@ public class ReadVLongBenchmark { return WritableUtils.isNegativeVInt(firstByte) ? ~i : i; } } - - /** - * Similar to {@link WritableUtils#readVLong(java.io.DataInput)} but reads from a - * {@link ByteBuff}. This method is optimized to read memstoreTs fields in data block encodings. - * Since a memstoreTs is 7 bytes when encoded as a vLong (1 size byte + 6 data bytes), this method - * is optimized to read that. Small regressions are present when reading smaller vLongs (2-3 - * bytes). - */ - public static long readVLongTimestamp(ByteBuff buf) { - byte firstByte = buf.get(); - int len = WritableUtils.decodeVIntSize(firstByte); - if (len == 1) { - return firstByte; - } - long i = 0; - if (buf.remaining() >= Bytes.SIZEOF_LONG) { - long k = buf.getLongAfterPosition(0); - int shift = 72 - (len << 3); - i = k >>> shift; - buf.skip(len - 1); - } else { - for (int idx = 0; idx < len - 1; idx++) { - byte b = buf.get(); - i = i << 8; - i = i | (b & 0xFF); - } - } - return WritableUtils.isNegativeVInt(firstByte) ? ~i : i; - } } diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/nio/ByteBuff.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/nio/ByteBuff.java index ce0fd03761..1cea2a9017 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/nio/ByteBuff.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/nio/ByteBuff.java @@ -58,7 +58,6 @@ import org.apache.hbase.thirdparty.io.netty.util.internal.ObjectUtil; */ @InterfaceAudience.Private public abstract class ByteBuff implements HBaseReferenceCounted { - private static final String REFERENCE_COUNT_NAME = "ReferenceCount"; private static final int NIO_BUFFER_LIMIT = 64 * 1024; // should not be more than 64KB. protected RefCnt refCnt; @@ -73,8 +72,8 @@ public abstract class ByteBuff implements HBaseReferenceCounted { * released. So we can avoid the overhead of checking the refCnt on every call. See HBASE-27710. */ protected void checkRefCount() { - if (refCnt.hasRecycler()) { - ObjectUtil.checkPositive(refCnt(), REFERENCE_COUNT_NAME); + if (refCnt.isRecycled()) { + throw new IllegalArgumentException("Reference count must be positive! Has this buffer already been released?"); } } diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/nio/RefCnt.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/nio/RefCnt.java index 59b96674ec..4437e1f8b3 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/nio/RefCnt.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/nio/RefCnt.java @@ -20,6 +20,7 @@ package org.apache.hadoop.hbase.nio; import com.google.errorprone.annotations.RestrictedApi; import org.apache.hadoop.hbase.io.ByteBuffAllocator; import org.apache.hadoop.hbase.io.ByteBuffAllocator.Recycler; +import org.apache.hadoop.util.Preconditions; import org.apache.yetus.audience.InterfaceAudience; import org.apache.hbase.thirdparty.io.netty.util.AbstractReferenceCounted; @@ -37,9 +38,32 @@ public class RefCnt extends AbstractReferenceCounted { private static final ResourceLeakDetector detector = ResourceLeakDetectorFactory.instance().newResourceLeakDetector(RefCnt.class); - private final Recycler recycler; + private volatile Recycler recycler; private final ResourceLeakTracker leak; + /** + * This class exists such that we can keep the invariant that "this.leak == null" is only true for + * the NONE recycler as detector#track can return null + */ + enum NoLeakTracker implements ResourceLeakTracker { + INSTANCE; + + @Override + public void record() { + + } + + @Override + public void record(Object hint) { + + } + + @Override + public boolean close(RefCnt trackedObject) { + return true; + } + } + /** * Create an {@link RefCnt} with an initial reference count = 1. If the reference count become * zero, the recycler will do nothing. Usually, an Heap {@link ByteBuff} will use this kind of @@ -55,15 +79,34 @@ public class RefCnt extends AbstractReferenceCounted { } public RefCnt(Recycler recycler) { - this.recycler = recycler; - this.leak = recycler == ByteBuffAllocator.NONE ? null : detector.track(this); + this.recycler = Preconditions.checkNotNull(recycler, "recycler cannot be null, pass NONE instead"); + this.leak = buildLeakTracker(recycler); } - /** - * Returns true if this refCnt has a recycler. - */ - public boolean hasRecycler() { - return recycler != ByteBuffAllocator.NONE; + private ResourceLeakTracker buildLeakTracker(Recycler recycler) { + if (recycler == ByteBuffAllocator.NONE) { + return null; + } + + ResourceLeakTracker leakTracker = detector.track(this); + if (leakTracker == null) { + return NoLeakTracker.INSTANCE; + } else { + return leakTracker; + } + } + + public boolean isRecycled() { + // TODO: variant 1 is commented below. It performs a check only on the volatile recycler field but shows a pretty large regression for NONE recyclers + /* + * Recycler r = recycler; + * return r != ByteBuffAllocator.NONE && r == null; + */ + // TODO: variant 2 is below which fixes the regression present in variant 1, but shows no real performance improvement + // We use leak != null as a replacement for checking this.recycler != ByteBuffAllocator.NONE as it's a + // non-volatile field, therefore, we avoid eating a memory barrier when checking recycle status for + // ByteBuffAllocator.NONE buffers (micro benchmarks showed the regression being significant) + return leak != null && recycler == null; } @Override @@ -92,7 +135,14 @@ public class RefCnt extends AbstractReferenceCounted { @Override protected final void deallocate() { - this.recycler.free(); + Recycler r = this.recycler; + if (r == null) { + return; + } + // set to null before actually releasing to minimize the time window that we could use a recycled instance + this.recycler = null; + r.free(); + if (leak != null) { this.leak.close(this); } diff --git a/pom.xml b/pom.xml index 20efab111c..a0b403f03e 100644 --- a/pom.xml +++ b/pom.xml @@ -758,6 +758,7 @@ hbase-asyncfs hbase-logging hbase-compression + hbase-benchmarks scm:git:git://gitbox.apache.org/repos/asf/hbase.git