[jira] [Commented] (HBASE-20211) ReadOnlyBufferException In UnsafeAccess

["ramkrishna.s.vasudevan (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HBASE-20211?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16401474#comment-16401474 ] 

ramkrishna.s.vasudevan commented on HBASE-20211:
------------------------------------------------

Thanks for the analysis. I just saw the parent JIRA that is linked to this. 
So the intention is to handle the ReadOnlyBuffer case is it? Currently we don't have such a use in our code so that is one reason for doing it that way. 
Ya if you see UnsafeAccess as a standalone util class your suggestion makes sense.
bq.Which is almost exactly what the ByteBuffer relative bulk get method does anyway, so there is no savings here, just overheard and complexity.
One important diff is that it could change the position if we use relative get() na. But here we don't do it. 
And other thing is we found that BB code has some 'prechecks' which we thought was unnecessary because we are in control of these buffers. 

> ReadOnlyBufferException In UnsafeAccess
> ---------------------------------------
>
>                 Key: HBASE-20211
>                 URL: https://issues.apache.org/jira/browse/HBASE-20211
>             Project: HBase
>          Issue Type: Bug
>          Components: hbase
>    Affects Versions: 2.0.0
>            Reporter: BELUGA BEHR
>            Priority: Minor
>
> If you trace the BBUtils API, what you see is this code:
> {code:java}
>   public static void copyFromBufferToArray(byte[] out, ByteBuffer in, int sourceOffset,
>       int destinationOffset, int length) {
>     if (in.hasArray()) {
>       System.arraycopy(in.array(), sourceOffset + in.arrayOffset(), out, destinationOffset, length);
>     } else if (UNSAFE_AVAIL) {
>       UnsafeAccess.copy(in, sourceOffset, out, destinationOffset, length);
>     } else {
>       ByteBuffer inDup = in.duplicate();
>       inDup.position(sourceOffset);
>       inDup.get(out, destinationOffset, length);
>     }
>   }
> {code}
> A ByteBuffer is being used here, which is not read-only, so it actually hits on the first condition and executes this code:
> {quote}System.arraycopy(in.array(), sourceOffset + in.arrayOffset(), out, destinationOffset, length);
> {quote}
> Which is almost exactly what the {{ByteBuffer}} relative bulk get method does anyway, so there is no savings here, just overheard and complexity.
> In regards to the second condition... there is a bug there that I just noticed.
> {code:java|title=org.apache.hadoop.hbase.util.UnsafeAccess}
>   public static void copy(ByteBuffer src, int srcOffset, byte[] dest, int destOffset,
>       int length) {
>     long srcAddress = srcOffset;
>     Object srcBase = null;
>     if (src.isDirect()) {
>       srcAddress = srcAddress + ((DirectBuffer) src).address();
>     } else {
>       srcAddress = srcAddress + BYTE_ARRAY_BASE_OFFSET + src.arrayOffset();
>       srcBase = src.array();
>     }
>     long destAddress = destOffset + BYTE_ARRAY_BASE_OFFSET;
>     unsafeCopy(srcBase, srcAddress, dest, destAddress, length);
>   }
> {code}
> This issue here is the [arrayOffset()|https://docs.oracle.com/javase/8/docs/api/java/nio/ByteBuffer.html#arrayOffset--] call. The JavaDocs here say:
> {quote}Invoke the hasArray method before invoking this method in order to ensure that this buffer has an accessible backing array.
> {quote}
> However, as we saw in the previous method, if _hasArray_ returns true, we do _System.arraycopy,_ so the only reason we would be in this _copy_ code is if there was no access to the backing array, yet here it is, depending on it having such access. That could cause problems with Read-Only ByteBuffers that does not affect the _relative bulk get method_.
> {code:java}
> public class Test {
>   public static void main(String[] args) throws IOException {
>     ByteArrayOutputStream baos = new ByteArrayOutputStream();
>     ByteBufferWriterOutputStream bbwos = new ByteBufferWriterOutputStream(baos);
>     ByteBuffer bbSmall = ByteBuffer.wrap(new byte[512]).asReadOnlyBuffer();
>     bbwos.write(bbSmall, 0, 512);
>     bbwos.close();
>   }
> }
> Exception in thread "main" java.nio.ReadOnlyBufferException
> 	at java.nio.ByteBuffer.arrayOffset(ByteBuffer.java:1024)
> 	at org.apache.hadoop.hbase.util.UnsafeAccess.copy(UnsafeAccess.java:398)
> 	at org.apache.hadoop.hbase.util.ByteBufferUtils.copyFromBufferToArray(ByteBufferUtils.java:54)
> 	at org.apache.hadoop.hbase.io.ByteBufferWriterOutputStream.write(ByteBufferWriterOutputStream.java:59)
> 	at org.apache.hadoop.hbase.io.Test.main(Test.java:14)
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)