[GitHub] [hbase] Apache9 commented on a diff in pull request #4592: HBASE-27170 ByteBuffAllocator leak when decompressing blocks near minSizeForReservoirUse

[GitBox <gi...@apache.org>]

Apache9 commented on code in PR #4592:
URL: https://github.com/apache/hbase/pull/4592#discussion_r912489386


##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1997,23 +2015,31 @@ static String toStringHeader(ByteBuff buf) throws IOException {
       + onDiskDataSizeWithHeader;
   }
 
-  private static HFileBlockBuilder createBuilder(HFileBlock blk) {
+  /**
+   * Creates a new HFileBlockBuilder from the existing block and a new ByteBuff. The builder will be
+   * loaded with all of the original fields from blk, except now using the newBuff and setting
+   * isSharedMem based on the source of the passed in newBuff. An existing HFileBlock may have been
+   * an {@link ExclusiveMemHFileBlock}, but the new buffer might call for a
+   * {@link SharedMemHFileBlock}. Or vice versa.
+   * @param blk     the block to clone from
+   * @param newBuff the new buffer to use
+   */
+  private static HFileBlockBuilder createBuilder(HFileBlock blk, ByteBuff newBuff) {

Review Comment:
   OK, so here is the trick.
   
   In the old code, we will just reuse the ByteBuff when unpacking, and then call allocateBuffer to copy the content to a new buffer, and then decompress it.
   
   And the problem is that, for the old implementation  of this method, we will always use the same isSharedMem, but actually, we may allocate off heap byte buff for the unpacked HFileBlock, while the packed HFileBlock is on heap.
   
   Good.



##########
hbase-common/src/test/java/org/apache/hadoop/hbase/io/TestByteBuffAllocator.java:
##########
@@ -21,6 +21,7 @@
 import static org.apache.hadoop.hbase.io.ByteBuffAllocator.getHeapAllocationRatio;

Review Comment:
   You can call InternalLoggerFactory.setDefaultFactory to inject a customized InternalLogger which will be used in netty.



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1997,23 +2015,31 @@ static String toStringHeader(ByteBuff buf) throws IOException {
       + onDiskDataSizeWithHeader;
   }
 
-  private static HFileBlockBuilder createBuilder(HFileBlock blk) {
+  /**
+   * Creates a new HFileBlockBuilder from the existing block and a new ByteBuff. The builder will be
+   * loaded with all of the original fields from blk, except now using the newBuff and setting
+   * isSharedMem based on the source of the passed in newBuff. An existing HFileBlock may have been
+   * an {@link ExclusiveMemHFileBlock}, but the new buffer might call for a
+   * {@link SharedMemHFileBlock}. Or vice versa.
+   * @param blk     the block to clone from
+   * @param newBuff the new buffer to use
+   */
+  private static HFileBlockBuilder createBuilder(HFileBlock blk, ByteBuff newBuff) {
     return new HFileBlockBuilder().withBlockType(blk.blockType)
       .withOnDiskSizeWithoutHeader(blk.onDiskSizeWithoutHeader)
       .withUncompressedSizeWithoutHeader(blk.uncompressedSizeWithoutHeader)
-      .withPrevBlockOffset(blk.prevBlockOffset).withByteBuff(blk.buf.duplicate()) // Duplicate the
-                                                                                  // buffer.
-      .withOffset(blk.offset).withOnDiskDataSizeWithHeader(blk.onDiskDataSizeWithHeader)
+      .withPrevBlockOffset(blk.prevBlockOffset).withByteBuff(newBuff).withOffset(blk.offset)
+      .withOnDiskDataSizeWithHeader(blk.onDiskDataSizeWithHeader)
       .withNextBlockOnDiskSize(blk.nextBlockOnDiskSize).withHFileContext(blk.fileContext)
-      .withByteBuffAllocator(blk.allocator).withShared(blk.isSharedMem());
+      .withByteBuffAllocator(blk.allocator).withShared(!newBuff.hasArray());
   }
 
-  static HFileBlock shallowClone(HFileBlock blk) {
-    return createBuilder(blk).build();
+  static HFileBlock shallowClone(HFileBlock blk, ByteBuff newBuf) {
+    return createBuilder(blk, newBuf).build();
   }
 
   static HFileBlock deepCloneOnHeap(HFileBlock blk) {
     ByteBuff deepCloned = ByteBuff.wrap(ByteBuffer.wrap(blk.buf.toBytes(0, blk.buf.limit())));
-    return createBuilder(blk).withByteBuff(deepCloned).withShared(false).build();
+    return createBuilder(blk, deepCloned).withShared(false).build();

Review Comment:
   Looking at the code, SharedMem is always off heap and ExclusiveMem is always on heap? Then I do not think we need to call withShard here? The createBuilder method will set it for us.



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/ByteBuffAllocator.java:
##########
@@ -319,11 +319,20 @@ public ByteBuff allocate(int size) {
       // just allocate the ByteBuffer from on-heap.
       bbs.add(allocateOnHeap(remain));
     }
-    ByteBuff bb = ByteBuff.wrap(bbs, () -> {
-      for (int i = 0; i < lenFromReservoir; i++) {
-        this.putbackBuffer(bbs.get(i));
-      }
-    });
+
+    ByteBuff bb;
+    // we only need a recycler if we successfully pulled from the pool
+    // this matters for determining whether to add leak detection in RefCnt
+    if (lenFromReservoir == 0) {

Review Comment:
   Good. Looking at the code, this could save some CPU cycles when releasing...



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1997,23 +2015,31 @@ static String toStringHeader(ByteBuff buf) throws IOException {
       + onDiskDataSizeWithHeader;
   }
 
-  private static HFileBlockBuilder createBuilder(HFileBlock blk) {
+  /**
+   * Creates a new HFileBlockBuilder from the existing block and a new ByteBuff. The builder will be
+   * loaded with all of the original fields from blk, except now using the newBuff and setting
+   * isSharedMem based on the source of the passed in newBuff. An existing HFileBlock may have been
+   * an {@link ExclusiveMemHFileBlock}, but the new buffer might call for a
+   * {@link SharedMemHFileBlock}. Or vice versa.
+   * @param blk     the block to clone from
+   * @param newBuff the new buffer to use
+   */
+  private static HFileBlockBuilder createBuilder(HFileBlock blk, ByteBuff newBuff) {
     return new HFileBlockBuilder().withBlockType(blk.blockType)
       .withOnDiskSizeWithoutHeader(blk.onDiskSizeWithoutHeader)
       .withUncompressedSizeWithoutHeader(blk.uncompressedSizeWithoutHeader)
-      .withPrevBlockOffset(blk.prevBlockOffset).withByteBuff(blk.buf.duplicate()) // Duplicate the
-                                                                                  // buffer.
-      .withOffset(blk.offset).withOnDiskDataSizeWithHeader(blk.onDiskDataSizeWithHeader)
+      .withPrevBlockOffset(blk.prevBlockOffset).withByteBuff(newBuff).withOffset(blk.offset)
+      .withOnDiskDataSizeWithHeader(blk.onDiskDataSizeWithHeader)
       .withNextBlockOnDiskSize(blk.nextBlockOnDiskSize).withHFileContext(blk.fileContext)
-      .withByteBuffAllocator(blk.allocator).withShared(blk.isSharedMem());
+      .withByteBuffAllocator(blk.allocator).withShared(!newBuff.hasArray());
   }
 
-  static HFileBlock shallowClone(HFileBlock blk) {
-    return createBuilder(blk).build();
+  static HFileBlock shallowClone(HFileBlock blk, ByteBuff newBuf) {

Review Comment:
   Since it is only called in unpack method, let's make it private?



##########
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/ByteBuff.java:
##########
@@ -547,6 +547,27 @@ public static ByteBuff wrap(ByteBuffer buffer) {
     return wrap(buffer, RefCnt.create());
   }
 
+  /**
+   * Calling this method in strategic locations where ByteBuffs are referenced may help diagnose
+   * potential buffer leaks. We pass the buffer itself as a default hint, but one can use
+   * {@link #touch(Object)} to pass their own hint as well.
+   */
+  @Override
+  public ByteBuff touch() {
+    return touch(this);
+  }
+
+  @Override
+  public ByteBuff touch(Object hint) {
+    refCnt.touch(hint);
+    return this;
+  }
+
+  // Visible for testing
+  public RefCnt getRefCnt() {

Review Comment:
   Better add a RestrictedApi to disable usage in code other than tests.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org