hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-5625) Avoid byte buffer allocations when reading a value from a Result object
Date Wed, 04 Apr 2012 17:09:30 GMT

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

jiraposter@reviews.apache.org commented on HBASE-5625:
------------------------------------------------------



bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > Looks good.  Some comments below.

Thanks! Please read the changes below.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 531
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line531>
bq.  >
bq.  >     How do we know this buffer is big enough?  Maybe should add an override that
takes an offset into the buffer?

Added overload. Added exception comment for when there is insufficient space remaining in
the buffer.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 1443
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line1443>
bq.  >
bq.  >     Is this an override?

Yes. Modified the original method to call the new one with default parameters.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 1448
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line1448>
bq.  >
bq.  >     Why pull out all these values here?  Maybe we'll fail the first test (q1 is
not used by the first test and these extractions cost.. they create byte arrays...)

Moved.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 2001
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line2001>
bq.  >
bq.  >     How do I know the buffer is big enough?  Should there be an override that takes
an offset?

Added buffer offset parameter. We know exactly how many bytes we have to write and how much
free space we have in the buffer. An 'IllegalArgumentException' is thrown when there isn't
enough free space. Is that what you were asking?


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 2002
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line2002>
bq.  >
bq.  >     Please follow the formatting that is present in the rest of the file.  Notice
placement of exceptions and params.  Do not add your own style.  Thanks.

Hope I fixed this.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 251
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line251>
bq.  >
bq.  >     Please follow convention that the rest of the file has.

Hope I fixed this.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 255
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line255>
bq.  >
bq.  >     ditto

Hope I fixed this.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 478
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line478>
bq.  >
bq.  >     Rename hasColumn or isColumn.

This overloads an original method. A refactoring here would be a major non-backwards compatible
change.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 398
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line398>
bq.  >
bq.  >     Rename hasContent or isNotEmpty or isNotEmptyColumn

This method was named to mirror 'containsColumn()'. See below.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 431
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line431>
bq.  >
bq.  >     Why we have both isNonEmptyColumn and isEmptyColumn?  Why not just one and then
check return with a !?

They are not complementary.
  containsColumn = value exists
  containsEmptyColumn = value exists & is empty byte array
  containsNonEmptyColumn = value exists & is not empty byte array
The value could be missing, in which case all methods would return false.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/client/TestResult.java, line 117
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97956#file97956line117>
bq.  >
bq.  >     Or leave it and change the name of the test so it doesn't have the test prefix.
 Add a main and have it call this.  I think the method useful.  Might as well keep it.

Refactored as 'doReadBenchmark()'; called from 'main()';


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 2056
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line2056>
bq.  >
bq.  >     This is a critical base class.  I'm nervous when it gets refactored.  You have
tests for each of your changes?  And all the old KV tests pass?

Added checks in 'TestKeyValue.testFirstLastOnRow()' for the new 'KeyValue.createFirstOnRow()'
methods.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 616
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line616>
bq.  >
bq.  >     How do I know the buffer is big enough?

Added exception comment for when there is insufficient space remaining in the buffer. Is that
what you meant?


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 297
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line297>
bq.  >
bq.  >     This comment should be on the @return javadoc rather than here.

This was copied from original. Should I remove both occurences?


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/client/TestResult.java, line 178
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97956#file97956line178>
bq.  >
bq.  >     I think there needs to be tests for the new KV functionality because KV is a
fundamental type and we can't mess it up.

Moved 'loadValue()' checks to their own test methods. Also took the liberty to move 'getColumn()'
checks in the same manner in order to keep consistency.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 1138
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line1138>
bq.  >
bq.  >     Why not create a ByteBuffer?  or called ByteBuffer wrap?  Why not call it toByteBuffer?

You need to be able to pass your own buffer when you have to compose multiple values.
I added a new method that uses 'ByteBuffer.wrap()'. I feel that this method should contain
the word 'value' in its name so as not to create the impression that it is writing the entire
underlying 'KeyValue' structure to the buffer, so I called it 'getValueAsByteBuffer()'. Please
comment if it is inadequate.


bq.  On 2012-04-02 17:34:38, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 87
bq.  > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line87>
bq.  >
bq.  >     This should be lazily instantiated

Fixed.


- Tudor


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4607/#review6623
-----------------------------------------------------------


On 2012-04-02 14:22:48, Tudor Scurtu wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/4607/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2012-04-02 14:22:48)
bq.  
bq.  
bq.  Review request for hbase.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  When calling Result.getValue(), an extra dummy KeyValue and its associated underlying
byte array are allocated, as well as a persistent buffer that will contain the returned value.
bq.  
bq.  These can be avoided by reusing a static array for the dummy object and by passing a
ByteBuffer object as a value destination buffer to the read method.
bq.  
bq.  
bq.  This addresses bug HBASE-5625.
bq.      https://issues.apache.org/jira/browse/HBASE-5625
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    src/main/java/org/apache/hadoop/hbase/KeyValue.java 243d76f 
bq.    src/main/java/org/apache/hadoop/hbase/client/Result.java df0b3ef 
bq.    src/test/java/org/apache/hadoop/hbase/client/TestResult.java f9e29c2 
bq.  
bq.  Diff: https://reviews.apache.org/r/4607/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  Added value check to TestResult#testBasic and TestResult.testMultiVersion.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Tudor
bq.  
bq.


                
> Avoid byte buffer allocations when reading a value from a Result object
> -----------------------------------------------------------------------
>
>                 Key: HBASE-5625
>                 URL: https://issues.apache.org/jira/browse/HBASE-5625
>             Project: HBase
>          Issue Type: Improvement
>          Components: client
>    Affects Versions: 0.92.1
>            Reporter: Tudor Scurtu
>            Assignee: Tudor Scurtu
>              Labels: patch
>             Fix For: 0.96.0
>
>         Attachments: 5625.txt, 5625v2.txt, 5625v3.txt, 5625v4.txt, 5625v5.txt, 5625v6.txt
>
>
> When calling Result.getValue(), an extra dummy KeyValue and its associated underlying
byte array are allocated, as well as a persistent buffer that will contain the returned value.
> These can be avoided by reusing a static array for the dummy object and by passing a
ByteBuffer object as a value destination buffer to the read method.
> The current functionality is maintained, and we have added a separate method call stack
that employs the described changes. I will provide more details with the patch.
> Running tests with a profiler, the reduction of read time seems to be of up to 40%.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message