HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read #479
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
openinx
reviewed
Aug 13, 2019
| */ | ||
| @FunctionalInterface | ||
| interface ChannelReader { | ||
| int read(ReadableByteChannel channel, ByteBuffer buf, long offset) throws IOException; |
There was a problem hiding this comment.
The ReadableByteChannel channel cannot accept the argument with offset position ? Looks strange here we provide a offset argument...
There was a problem hiding this comment.
any good idea to abstract this？because FileChannel need this, so provide it as an arg
There was a problem hiding this comment.
Have no better idea now, maybe we can keep the current way :-)
| // Read max possible into the current BB | ||
| int len = channelRead(channel, this.curItem); | ||
| if (len > 0) | ||
| int len = read(channel, this.curItem, offset, reader); |
There was a problem hiding this comment.
Seems we can also create a iterator to read & fill the ByteBuff ? Similar with the BufferIterator in ByteBufferArray, I think that would be more clearer.... Can be a following issue ?
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/MultiByteBuff.java
Show resolved
Hide resolved
| @@ -304,23 +303,23 @@ void refreshFileConnection(int accessFileNum, IOException ioe) throws IOExceptio | |||
| } | |||
|
|
|||
| private interface FileAccessor { | |||
| int access(FileChannel fileChannel, ByteBuffer byteBuffer, long accessOffset) | |||
| int access(FileChannel fileChannel, ByteBuff byteBuffer, long accessOffset) | |||
There was a problem hiding this comment.
byteBuffer -> byteBuff or buff ?
| throws IOException; | ||
| } | ||
|
|
||
| private static class FileReadAccessor implements FileAccessor { | ||
| @Override | ||
| public int access(FileChannel fileChannel, ByteBuffer byteBuffer, | ||
| public int access(FileChannel fileChannel, ByteBuff byteBuffer, |
| } | ||
| } | ||
|
|
||
| private static class FileWriteAccessor implements FileAccessor { | ||
| @Override | ||
| public int access(FileChannel fileChannel, ByteBuffer byteBuffer, | ||
| public int access(FileChannel fileChannel, ByteBuff byteBuffer, |
...erver/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileScannerImplReferenceCount.java
Show resolved
Hide resolved
openinx
reviewed
Aug 13, 2019
| @@ -689,7 +691,7 @@ private void freeEntireBuckets(int completelyFreeBucketsNeeded) { | |||
| // this set is small around O(Handler Count) unless something else is wrong | |||
| Set<Integer> inUseBuckets = new HashSet<>(); | |||
| backingMap.forEach((k, be) -> { | |||
| if (be.isRpcRef()) { | |||
| if (ioEngine.usesSharedMemory() && be.isRpcRef()) { | |||
There was a problem hiding this comment.
The javadoc says:
/**
* This method will find the buckets that are minimally occupied
* and are not reference counted and will free them completely
* without any constraint on the access times of the elements,
* and as a process will completely free at most the number of buckets
* passed, sometimes it might not due to changing refCounts
*
* @param completelyFreeBucketsNeeded number of buckets to free
**/
Means the free processing only consider the usage percentage regardless of the accessCounter, so we will free the entire bucket even if we still have some RPC reading for Exclusive ioengine ? That should be inconrrect ?
openinx
reviewed
Aug 13, 2019
| @@ -502,8 +502,10 @@ public Cacheable getBlock(BlockCacheKey key, boolean caching, boolean repeat, | |||
| // block will use the refCnt of bucketEntry, which means if two HFileBlock mapping to | |||
| // the same BucketEntry, then all of the three will share the same refCnt. | |||
| Cacheable cachedBlock = ioEngine.read(bucketEntry); | |||
| // RPC start to reference, so retain here. | |||
| cachedBlock.retain(); | |||
| if (ioEngine.usesSharedMemory()) { | |||
There was a problem hiding this comment.
Could we consider the patch in another side ? Say the BucketEntry#refCnt still means how many reference path from the RPC handler & BucketCache ... But the Cacheable#refCnt will always be 1 when the RPC is still handling.. Once the cells shipped to client, then both the Cacheable#refCnt & BucketEntry#refCnt will decrease, actually the Cacheable will de-allocate the memory from ByteBuffAllocator.
In this way, the BucketEntry#refCnt still have the same meaning as the SharedFileIOEngine in BucketCache. I mean in BucketCache we don't need to care the IOEngine is shared or exclusive.
There was a problem hiding this comment.
let me think how to achieve this
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
openinx
reviewed
Aug 15, 2019
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/MultiByteBuff.java
Show resolved
Hide resolved
| private Iterator<ByteBuffer> buffsIterator = new Iterator<ByteBuffer>() { | ||
| @Override | ||
| public boolean hasNext() { | ||
| return curItemIndex <= limitedItemIndex && items[curItemIndex].hasRemaining(); |
There was a problem hiding this comment.
One concern here, assume the following steps:
- currentByteBuffer = it#next();
- it#hasNext() must return true;
- we write the current ByteBuffer & fullfill it, then its hasRemaining will be false ....
- it#hasNext() will return false because of the currBuff.hasRemaining is false.
Say for the same ByteBuffer, the it#hasNext() will return different value. In a general sense, if we don't have any Next(), then the it#hasNext() should always have the same return value, I mean ?
Maybe we can remove the && items[curItemIndex].hasRemaining() here, can let the outside logic hande it ?
There was a problem hiding this comment.
how about change it like this
curItemIndex < limitedItemIndex || (curItemIndex == limitedItemIndex && items[curItemIndex].hasRemaining())
| */ | ||
| @FunctionalInterface | ||
| interface ChannelReader { | ||
| int read(ReadableByteChannel channel, ByteBuffer buf, long offset) throws IOException; |
There was a problem hiding this comment.
Have no better idea now, maybe we can keep the current way :-)
| @@ -60,4 +64,30 @@ protected final void deallocate() { | |||
| public final ReferenceCounted touch(Object hint) { | |||
| throw new UnsupportedOperationException(); | |||
| } | |||
|
|
|||
| public void setInnerRefCnt(Optional<RefCnt> refCnt) { | |||
There was a problem hiding this comment.
The RefCnt is a general reference count class here... The refCnt with interRefCnt is mainly used for BucketEntry ? Could we create a subclass for BuckentEntry only ? I mean the general ByteBuff use the customized will be resource-consuming ...
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
openinx
reviewed
Aug 18, 2019
| return this.refCnt; | ||
| } | ||
|
|
||
| public void setRefCnt(RefCnt refCnt) { |
There was a problem hiding this comment.
I think setRefCnt for a given ByteBuff is dangerous ? because its previous recycler will be lost, so better not to expose this method as public ? We have one similar issue before, see:
/**
* In theory, the upstream should never construct an ByteBuff by passing an given refCnt, so
* please don't use this public method in other place. Make the method public here because the
* BucketEntry#wrapAsCacheable in hbase-server module will use its own refCnt and ByteBuffers from
* IOEngine to composite an HFileBlock's ByteBuff, we didn't find a better way so keep the public
* way here.
*/
public static ByteBuff wrap(ByteBuffer[] buffers, RefCnt refCnt) {
There was a problem hiding this comment.
have not find a better way yet, any suggestions?
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/CompositeRefCnt.java
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public boolean release() { | ||
| boolean innerRes = true; |
There was a problem hiding this comment.
Just the following (not compile, should be similar):
return super.release() && innerRefCnt.map(innerRefCnt::release).orElse(true);
|
|
||
| @Override | ||
| public ReferenceCounted retain() { | ||
| if (innerRefCnt.isPresent()) { |
There was a problem hiding this comment.
innerRefCnt.map(innerRefCnt::retain).orElse(super.retain);
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/MultiByteBuff.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| @edu.umd.cs.findbugs.annotations.SuppressWarnings(value="IT_NO_SUCH_ELEMENT", |
There was a problem hiding this comment.
I think here can throw the NoSuchElementException if curItemIndex exceed ?
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@chenxu14 seems there're still some checkstyle issues ? please see: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-479/55/artifact/out/diff-javadoc-javadoc-hbase-common.txt ... |
Sorry for the delay, very glad if you can help improve it. |
anoopsjohn
reviewed
Aug 26, 2019
| * the upstream should not use this public method in other place, or the previous recycler | ||
| * will be lost. | ||
| */ | ||
| public void shareRefCnt(RefCnt refCnt, boolean replace) { |
There was a problem hiding this comment.
In case of File based BC, now we make it to read the cached data into the pooled BBs. Every read RPC will acquire own BBs and read into. There is ideally no sharing of BBs across the readers happen at all.. But seems here we try share the ref count of the BC (File based here) with the RPCs. Little strange to digest.
There was a problem hiding this comment.
Sorry, there missing some conversations with @openinx
We do this in order not to violate the LRU, when BucketCache#freeEntireBuckets is executed
anoopsjohn
reviewed
Aug 26, 2019
| dstBuffer.rewind(); | ||
| return be.wrapAsCacheable(new ByteBuffer[] { dstBuffer }); | ||
| dstBuff.rewind(); | ||
| return be.wrapAsCacheable(dstBuff); |
There was a problem hiding this comment.
Reading this wrapAsCacheable method. Seems we have only one BucketEntry type now for shared or exclusive types of entries and that uses the refCount associated with it (within BC scope) and this makes it required to have this kind of Composite count mechanism and all now?
There was a problem hiding this comment.
yep, exclusive HFileBlock will use CompositeRefCnt#innerRefCnt to share with BucketEntry
There was a problem hiding this comment.
IMHO this is an unwanted sharing and ref count check.
anoopsjohn
reviewed
Aug 26, 2019
| * BucketCache#freeEntireBuckets is called, will not violate the LRU policy. | ||
| * <p> | ||
| * And it has its own refCnt & Recycler, Once the cells shipped to client, then both the | ||
| * Cacheable#refCnt & BucketEntry#refCnt will be decreased. when Cacheable's refCnt decrease |
There was a problem hiding this comment.
This means when say 2 read RPCs access a block from File based BC, that entry can NOT get evicted unless both these RPCs are over?
There was a problem hiding this comment.
yep, as mentioned above
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
I think it should good enough to commit now, let me commit this patch if no objection. |
|
💔 -1 overall
This message was automatically generated. |
|
Raised a concern regarding the usage of RPC ref counts even for the evictions when it is FileIOE and it is Exclusive cache. IMHO we should really see how to avoid this. |
|
Can we ignore LRU in this case(BucketCache#freeEntireBuckets), I think HBASE-16630 has already done so. In this way, there is no refCnt sharing between exclusive HFileBlock and BucketEntry, and many temporary object creation(CompositeRefCnt) can be omitted. |
|
I think @chenxu14 have tried to accomplish the patch in some eailier patch version ( says just ignore the RPC reference when freeEntireBuckets for exclusive io engine case). What I concern before is: we're freeing some referenced block in exclusive IO engine, seems it's volidating the LRU policy. Reconsider the case, for exclusive IO engine, free the memory area in bucket allocator should be OK. Compared to the buggy CompositeRefCnt (Not say the current version have bug but say it's an more complex implementation and easy to introduce new bugs), sacrificing some LRU attribution in rare cases should also be acceptable. I mean we need to balance between them, maybe we can still try the patch without CompositeRefCnt. @anoopsjohn FYI. |
|
will add another PR to replace this |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.