[GitHub] [hbase] chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp
ByteBuffer allocation in FileIOEngine#read
URL: https://github.com/apache/hbase/pull/479#discussion_r317656286
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/FileIOEngine.java
##
@@ -129,20 +129,20 @@ public Cacheable read(BucketEntry be) throws IOException
{
long offset = be.offset();
int length = be.getLength();
Preconditions.checkArgument(length >= 0, "Length of read can not be less
than 0.");
-ByteBuffer dstBuffer = ByteBuffer.allocate(length);
+ByteBuff dstBuff = be.allocator.allocate(length);
if (length != 0) {
- accessFile(readAccessor, dstBuffer, offset);
+ accessFile(readAccessor, dstBuff, offset);
// The buffer created out of the fileChannel is formed by copying the
data from the file
// Hence in this case there is no shared memory that we point to. Even
if the BucketCache
// evicts this buffer from the file the data is already copied and there
is no need to
// ensure that the results are not corrupted before consuming them.
- if (dstBuffer.limit() != length) {
+ if (dstBuff.limit() != length) {
throw new IllegalArgumentIOException(
-"Only " + dstBuffer.limit() + " bytes read, " + length + "
expected");
+"Only " + dstBuff.limit() + " bytes read, " + length + "
expected");
}
}
-dstBuffer.rewind();
-return be.wrapAsCacheable(new ByteBuffer[] { dstBuffer });
+dstBuff.rewind();
+return be.wrapAsCacheable(dstBuff);
Review comment:
yep, exclusive HFileBlock will use CompositeRefCnt#innerRefCnt to share with
BucketEntry
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read URL: https://github.com/apache/hbase/pull/479#discussion_r317647782 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/nio/CompositeRefCnt.java ## @@ -0,0 +1,59 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.nio; + +import java.util.Optional; +import org.apache.yetus.audience.InterfaceAudience; +import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting; +import org.apache.hbase.thirdparty.io.netty.util.ReferenceCounted; + +/** + * The CompositeRefCnt is mainly used by exclusive memory HFileBlock, it has a innerRefCnt + * to share with BucketEntry, in order to summarize the number of RPC requests. So when + * BucketCache#freeEntireBuckets is called, will not violate the LRU policy. + * + * 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 Review comment: yep, as mentioned above 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp
ByteBuffer allocation in FileIOEngine#read
URL: https://github.com/apache/hbase/pull/479#discussion_r317646441
##
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/nio/ByteBuff.java
##
@@ -78,6 +79,23 @@ public boolean release() {
return refCnt.release();
}
+ public RefCnt getRefCnt() {
+return this.refCnt;
+ }
+
+ /**
+ * BucketEntry use this to share refCnt with ByteBuff, so make the method
public here,
+ * 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) {
Review comment:
Sorry, there missing some conversations with @openinx
We do this in order not to violate the LRU, when
BucketCache#freeEntireBuckets is executed
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp
ByteBuffer allocation in FileIOEngine#read
URL: https://github.com/apache/hbase/pull/479#discussion_r315502192
##
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/nio/ByteBuff.java
##
@@ -78,6 +79,14 @@ public boolean release() {
return refCnt.release();
}
+ public RefCnt getRefCnt() {
+return this.refCnt;
+ }
+
+ public void setRefCnt(RefCnt refCnt) {
Review comment:
have not find a better way yet, any suggestions?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp
ByteBuffer allocation in FileIOEngine#read
URL: https://github.com/apache/hbase/pull/479#discussion_r314206957
##
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/MultiByteBuff.java
##
@@ -53,6 +55,19 @@
private int markedItemIndex = -1;
private final int[] itemBeginPos;
+ private Iterator buffsIterator = new Iterator() {
+@Override
+public boolean hasNext() {
+ return curItemIndex <= limitedItemIndex &&
items[curItemIndex].hasRemaining();
Review comment:
how about change it like this
`curItemIndex < limitedItemIndex || (curItemIndex == limitedItemIndex &&
items[curItemIndex].hasRemaining())`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp
ByteBuffer allocation in FileIOEngine#read
URL: https://github.com/apache/hbase/pull/479#discussion_r313478081
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
##
@@ -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()) {
Review comment:
let me think how to achieve this
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp
ByteBuffer allocation in FileIOEngine#read
URL: https://github.com/apache/hbase/pull/479#discussion_r313478218
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
##
@@ -689,7 +691,7 @@ private void freeEntireBuckets(int
completelyFreeBucketsNeeded) {
// this set is small around O(Handler Count) unless something else is
wrong
Set inUseBuckets = new HashSet<>();
backingMap.forEach((k, be) -> {
-if (be.isRpcRef()) {
+if (ioEngine.usesSharedMemory() && be.isRpcRef()) {
Review comment:
make sense
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
chenxu14 commented on a change in pull request #479: HBASE-22802 Avoid temp
ByteBuffer allocation in FileIOEngine#read
URL: https://github.com/apache/hbase/pull/479#discussion_r313474978
##
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/nio/ByteBuff.java
##
@@ -450,10 +451,37 @@ public boolean release() {
*/
public abstract int read(ReadableByteChannel channel) throws IOException;
+ /**
+ * Reads bytes from FileChannel into this ByteBuff
+ */
+ public abstract int read(FileChannel channel, long offset) throws
IOException;
+
+ /**
+ * Write this ByteBuff's data into target file
+ */
+ public abstract int write(FileChannel channel, long offset) throws
IOException;
+
+ /**
+ * function interface for Channel read
+ */
+ @FunctionalInterface
+ interface ChannelReader {
+int read(ReadableByteChannel channel, ByteBuffer buf, long offset) throws
IOException;
Review comment:
any good idea to abstract this？because FileChannel need this, so provide it
as an arg
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
