[GitHub] [hbase] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r316608671
##
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:
OK, you have a clear comment here. seems OK now.
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r314965000
##
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/CompositeRefCnt.java
##
@@ -0,0 +1,65 @@
+/**
+ * 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.hadoop.hbase.io.ByteBuffAllocator.Recycler;
+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;
+
+/**
+ * Exclusive HFileBlock use this to summarize RPC refcount, see HBASE-22802
+ */
+@InterfaceAudience.Private
+public class CompositeRefCnt extends RefCnt {
+
+ private Optional innerRefCnt;
+
+ public CompositeRefCnt(Recycler recycler) {
+super(recycler);
+ }
+
+ public CompositeRefCnt(RefCnt orignal, RefCnt inner) {
+super(orignal.getRecycler());
+this.innerRefCnt = Optional.ofNullable(inner);
+ }
+
+ @VisibleForTesting
+ public Optional getInnerRefCnt() {
+return this.innerRefCnt;
+ }
+
+ @Override
+ public boolean release() {
+boolean innerRes = true;
+if (innerRefCnt.isPresent()) {
+ innerRes = innerRefCnt.get().release();
+}
+return super.release() && innerRes;
+ }
+
+ @Override
+ public ReferenceCounted retain() {
+if (innerRefCnt.isPresent()) {
Review comment:
```
innerRefCnt.map(innerRefCnt::retain).orElse(super.retain);
```
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r314965005
##
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/MultiByteBuff.java
##
@@ -53,6 +55,22 @@
private int markedItemIndex = -1;
private final int[] itemBeginPos;
+ private Iterator buffsIterator = new Iterator() {
+@Override
+public boolean hasNext() {
+ return curItemIndex < limitedItemIndex ||
+ (curItemIndex == limitedItemIndex &&
items[curItemIndex].hasRemaining());
Review comment:
Seems good now.
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r314964870
##
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:
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) {
```
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r314965110 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/nio/CompositeRefCnt.java ## @@ -0,0 +1,65 @@ +/** + * 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.hadoop.hbase.io.ByteBuffAllocator.Recycler; +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; + +/** + * Exclusive HFileBlock use this to summarize RPC refcount, see HBASE-22802 + */ Review comment: BTW, seems we needn't consider the atomic for two refCnt#release or refCnt#retain 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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r314964982
##
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/CompositeRefCnt.java
##
@@ -0,0 +1,65 @@
+/**
+ * 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.hadoop.hbase.io.ByteBuffAllocator.Recycler;
+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;
+
+/**
+ * Exclusive HFileBlock use this to summarize RPC refcount, see HBASE-22802
+ */
+@InterfaceAudience.Private
+public class CompositeRefCnt extends RefCnt {
+
+ private Optional innerRefCnt;
+
+ public CompositeRefCnt(Recycler recycler) {
+super(recycler);
+ }
+
+ public CompositeRefCnt(RefCnt orignal, RefCnt inner) {
+super(orignal.getRecycler());
+this.innerRefCnt = Optional.ofNullable(inner);
+ }
+
+ @VisibleForTesting
+ public Optional getInnerRefCnt() {
+return this.innerRefCnt;
+ }
+
+ @Override
+ public boolean release() {
+boolean innerRes = true;
Review comment:
Just the following (not compile, should be similar):
```
return super.release() && innerRefCnt.map(innerRefCnt::release).orElse(true);
```
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r314964906 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/nio/CompositeRefCnt.java ## @@ -0,0 +1,65 @@ +/** + * 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.hadoop.hbase.io.ByteBuffAllocator.Recycler; +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; + +/** + * Exclusive HFileBlock use this to summarize RPC refcount, see HBASE-22802 + */ Review comment: The javadoc need more detail to explain while we need the CompositeRefCnt I think, please explain the relationship between BucketEntry & HFIleBlock 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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r314965031
##
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/MultiByteBuff.java
##
@@ -53,6 +55,22 @@
private int markedItemIndex = -1;
private final int[] itemBeginPos;
+ private Iterator buffsIterator = new Iterator() {
+@Override
+public boolean hasNext() {
+ return curItemIndex < limitedItemIndex ||
+ (curItemIndex == limitedItemIndex &&
items[curItemIndex].hasRemaining());
+}
+
+@Override
+
@edu.umd.cs.findbugs.annotations.SuppressWarnings(value="IT_NO_SUCH_ELEMENT",
Review comment:
I think here can throw the NoSuchElementException if curItemIndex exceed ?
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r314163344
##
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:
One concern here, assume the following steps:
1. currentByteBuffer = it#next();
2. it#hasNext() must return true;
3. we write the current ByteBuffer & fullfill it, then its hasRemaining
will be false
4. 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 ?
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r314163983
##
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/nio/RefCnt.java
##
@@ -60,4 +64,30 @@ protected final void deallocate() {
public final ReferenceCounted touch(Object hint) {
throw new UnsupportedOperationException();
}
+
+ public void setInnerRefCnt(Optional refCnt) {
Review 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 ...
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r314163698
##
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:
Have no better idea now, maybe we can keep the current way :-)
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r314161328
##
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() {
Review comment:
The iterator looks great here.
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r313260023
##
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:
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.
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r313256448
##
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:
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 ?
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r313251457
##
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/MultiByteBuff.java
##
@@ -1064,28 +1065,58 @@ public void get(ByteBuffer out, int sourceOffset, int
length) {
return output;
}
- @Override
- public int read(ReadableByteChannel channel) throws IOException {
+ private int internalRead(ReadableByteChannel channel, long offset,
+ ChannelReader reader) throws IOException {
checkRefCount();
int total = 0;
while (true) {
- // Read max possible into the current BB
- int len = channelRead(channel, this.curItem);
- if (len > 0)
+ int len = read(channel, this.curItem, offset, reader);
Review 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 ?
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r313252311
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/FileIOEngine.java
##
@@ -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)
throws IOException;
}
private static class FileReadAccessor implements FileAccessor {
@Override
-public int access(FileChannel fileChannel, ByteBuffer byteBuffer,
+public int access(FileChannel fileChannel, ByteBuff byteBuffer,
long accessOffset) throws IOException {
- return fileChannel.read(byteBuffer, accessOffset);
+ return byteBuffer.read(fileChannel, accessOffset);
}
}
private static class FileWriteAccessor implements FileAccessor {
@Override
-public int access(FileChannel fileChannel, ByteBuffer byteBuffer,
+public int access(FileChannel fileChannel, ByteBuff byteBuffer,
Review comment:
ditto
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r313251801
##
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/nio/MultiByteBuff.java
##
@@ -1064,28 +1065,58 @@ public void get(ByteBuffer out, int sourceOffset, int
length) {
return output;
}
- @Override
- public int read(ReadableByteChannel channel) throws IOException {
+ private int internalRead(ReadableByteChannel channel, long offset,
+ ChannelReader reader) throws IOException {
checkRefCount();
int total = 0;
while (true) {
- // Read max possible into the current BB
- int len = channelRead(channel, this.curItem);
- if (len > 0)
+ int len = read(channel, this.curItem, offset, reader);
+ if (len > 0) {
total += len;
+offset += len;
+ }
if (this.curItem.hasRemaining()) {
-// We were not able to read enough to fill the current BB itself.
Means there is no point in
-// doing more reads from Channel. Only this much there for now.
break;
} else {
-if (this.curItemIndex >= this.limitedItemIndex) break;
+if (this.curItemIndex >= this.limitedItemIndex) {
+ break;
+}
this.curItemIndex++;
this.curItem = this.items[this.curItemIndex];
}
}
return total;
}
+ @Override
+ public int read(ReadableByteChannel channel) throws IOException {
+return internalRead(channel, 0, CHANNEL_READER);
+ }
+
+ @Override
+ public int read(FileChannel channel, long offset) throws IOException {
+return internalRead(channel, offset, FILE_READER);
+ }
+
+ @Override
+ public int write(FileChannel channel, long offset) throws IOException {
Review comment:
Here we can also use the Iterator if we have one .
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r313252283
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/FileIOEngine.java
##
@@ -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)
throws IOException;
}
private static class FileReadAccessor implements FileAccessor {
@Override
-public int access(FileChannel fileChannel, ByteBuffer byteBuffer,
+public int access(FileChannel fileChannel, ByteBuff byteBuffer,
Review comment:
ditto
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r313252250
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/FileIOEngine.java
##
@@ -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)
Review comment:
byteBuffer -> byteBuff or buff ?
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r313252739
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileScannerImplReferenceCount.java
##
@@ -71,6 +78,15 @@
@Rule
public TestName CASE = new TestName();
+ @Parameters(name = "{index}: ioengine={0}")
+ public static Collection data() {
Review comment:
Looks good here.
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] openinx commented on a change in pull request #479: HBASE-22802 Avoid temp ByteBuffer allocation in FileIOEngine#read
openinx 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_r313250391
##
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:
The ReadableByteChannel channel cannot accept the argument with offset
position ? Looks strange here we provide a offset argument...
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
