You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by bb...@apache.org on 2023/03/17 19:24:35 UTC
[hbase] branch branch-2 updated: HBASE-27710 ByteBuff ref counting is too expensive for on-heap buffers (#5115)
This is an automated email from the ASF dual-hosted git repository.
bbeaudreault pushed a commit to branch branch-2
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-2 by this push:
new 5de1e4f88e6 HBASE-27710 ByteBuff ref counting is too expensive for on-heap buffers (#5115)
5de1e4f88e6 is described below
commit 5de1e4f88e60bd0ac0861cd96defb441364d5234
Author: Bryan Beaudreault <bb...@apache.org>
AuthorDate: Fri Mar 17 15:24:28 2023 -0400
HBASE-27710 ByteBuff ref counting is too expensive for on-heap buffers (#5115)
Signed-off-by: Duo Zhang <zh...@apache.org>
---
.../java/org/apache/hadoop/hbase/nio/ByteBuff.java | 11 ++-
.../java/org/apache/hadoop/hbase/nio/RefCnt.java | 7 ++
.../hadoop/hbase/io/TestByteBuffAllocator.java | 105 ++++++++++++++++++++-
3 files changed, 120 insertions(+), 3 deletions(-)
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 9e77bfcd04b..ce0fd03761f 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
@@ -65,8 +65,17 @@ public abstract class ByteBuff implements HBaseReferenceCounted {
/*************************** Methods for reference count **********************************/
+ /**
+ * Checks that there are still references to the buffer. This protects against the case where a
+ * ByteBuff method (i.e. slice, get, etc) could be called against a buffer whose backing data may
+ * have been released. We only need to do this check if the refCnt has a recycler. If there's no
+ * recycler, the backing data will be handled by normal java GC and won't get incorrectly
+ * released. So we can avoid the overhead of checking the refCnt on every call. See HBASE-27710.
+ */
protected void checkRefCount() {
- ObjectUtil.checkPositive(refCnt(), REFERENCE_COUNT_NAME);
+ if (refCnt.hasRecycler()) {
+ ObjectUtil.checkPositive(refCnt(), REFERENCE_COUNT_NAME);
+ }
}
@Override
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 7c1f23383d3..59b96674ec3 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
@@ -59,6 +59,13 @@ public class RefCnt extends AbstractReferenceCounted {
this.leak = recycler == ByteBuffAllocator.NONE ? null : detector.track(this);
}
+ /**
+ * Returns true if this refCnt has a recycler.
+ */
+ public boolean hasRecycler() {
+ return recycler != ByteBuffAllocator.NONE;
+ }
+
@Override
public ReferenceCounted retain() {
maybeRecord();
diff --git a/hbase-common/src/test/java/org/apache/hadoop/hbase/io/TestByteBuffAllocator.java b/hbase-common/src/test/java/org/apache/hadoop/hbase/io/TestByteBuffAllocator.java
index d77dc6604be..8dcb1465684 100644
--- a/hbase-common/src/test/java/org/apache/hadoop/hbase/io/TestByteBuffAllocator.java
+++ b/hbase-common/src/test/java/org/apache/hadoop/hbase/io/TestByteBuffAllocator.java
@@ -221,8 +221,10 @@ public class TestByteBuffAllocator {
assertEquals(0, buf2.refCnt());
assertEquals(0, dup2.refCnt());
assertEquals(0, alloc.getFreeBufferCount());
- assertException(dup2::position);
- assertException(buf2::position);
+ // these should not throw an exception because they are heap buffers.
+ // off-heap buffers would throw an exception if trying to call a method once released.
+ dup2.position();
+ buf2.position();
// duplicate the buf1, if the dup1 released, buf1 will also be released (MultipleByteBuffer)
ByteBuff dup1 = buf1.duplicate();
@@ -415,4 +417,103 @@ public class TestByteBuffAllocator {
alloc1.allocate(1024);
Assert.assertEquals(getHeapAllocationRatio(HEAP, HEAP, alloc1), 1024f / (1024f + 2048f), 1e-6);
}
+
+ /**
+ * Tests that we only call the logic in checkRefCount for ByteBuff's that have a non-NONE
+ * recycler.
+ */
+ @Test
+ public void testCheckRefCountOnlyWithRecycler() {
+ TrackingSingleByteBuff singleBuff = new TrackingSingleByteBuff(ByteBuffer.allocate(1));
+ singleBuff.get();
+ assertEquals(1, singleBuff.getCheckRefCountCalls());
+ assertEquals(0, singleBuff.getRefCntCalls());
+ singleBuff = new TrackingSingleByteBuff(() -> {
+ // do nothing, just so we dont use NONE recycler
+ }, ByteBuffer.allocate(1));
+ singleBuff.get();
+ assertEquals(1, singleBuff.getCheckRefCountCalls());
+ assertEquals(1, singleBuff.getRefCntCalls());
+
+ TrackingMultiByteBuff multiBuff = new TrackingMultiByteBuff(ByteBuffer.allocate(1));
+
+ multiBuff.get();
+ assertEquals(1, multiBuff.getCheckRefCountCalls());
+ assertEquals(0, multiBuff.getRefCntCalls());
+ multiBuff = new TrackingMultiByteBuff(() -> {
+ // do nothing, just so we dont use NONE recycler
+ }, ByteBuffer.allocate(1));
+ multiBuff.get();
+ assertEquals(1, multiBuff.getCheckRefCountCalls());
+ assertEquals(1, multiBuff.getRefCntCalls());
+ }
+
+ private static class TrackingSingleByteBuff extends SingleByteBuff {
+
+ int refCntCalls = 0;
+ int checkRefCountCalls = 0;
+
+ public TrackingSingleByteBuff(ByteBuffer buf) {
+ super(buf);
+ }
+
+ public TrackingSingleByteBuff(ByteBuffAllocator.Recycler recycler, ByteBuffer buf) {
+ super(recycler, buf);
+ }
+
+ public int getRefCntCalls() {
+ return refCntCalls;
+ }
+
+ public int getCheckRefCountCalls() {
+ return checkRefCountCalls;
+ }
+
+ @Override
+ protected void checkRefCount() {
+ checkRefCountCalls++;
+ super.checkRefCount();
+ }
+
+ @Override
+ public int refCnt() {
+ refCntCalls++;
+ return super.refCnt();
+ }
+ }
+
+ private static class TrackingMultiByteBuff extends MultiByteBuff {
+
+ int refCntCalls = 0;
+ int checkRefCountCalls = 0;
+
+ public TrackingMultiByteBuff(ByteBuffer... items) {
+ super(items);
+ }
+
+ public TrackingMultiByteBuff(ByteBuffAllocator.Recycler recycler, ByteBuffer... items) {
+ super(recycler, items);
+ }
+
+ public int getRefCntCalls() {
+ return refCntCalls;
+ }
+
+ public int getCheckRefCountCalls() {
+ return checkRefCountCalls;
+ }
+
+ @Override
+ protected void checkRefCount() {
+ checkRefCountCalls++;
+ super.checkRefCount();
+ }
+
+ @Override
+ public int refCnt() {
+ refCntCalls++;
+ return super.refCnt();
+ }
+ }
+
}