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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
