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 Mon, 02 Apr 2012 17:35:23 GMT

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

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


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


Looks good.  Some comments below.


src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14311>

    How do we know this buffer is big enough?  Maybe should add an override that takes an
offset into the buffer?



src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14312>

    I like this refactoring.



src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14313>

    How do I know the buffer is big enough?



src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14315>

    Why not create a ByteBuffer?  or called ByteBuffer wrap?  Why not call it toByteBuffer?



src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14318>

    Is this an override?



src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14317>

    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...)



src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14319>

    How do I know the buffer is big enough?  Should there be an override that takes an offset?



src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14320>

    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.



src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14321>

    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?



src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14322>

    This should be lazily instantiated



src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14323>

    Please follow convention that the rest of the file has.



src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14324>

    ditto



src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14325>

    This comment should be on the @return javadoc rather than here.



src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14326>

    Rename hasContent or isNotEmpty or isNotEmptyColumn



src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14327>

    Why we have both isNonEmptyColumn and isEmptyColumn?  Why not just one and then check
return with a !?



src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14328>

    Rename hasColumn or isColumn.



src/test/java/org/apache/hadoop/hbase/client/TestResult.java
<https://reviews.apache.org/r/4607/#comment14310>

    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.



src/test/java/org/apache/hadoop/hbase/client/TestResult.java
<https://reviews.apache.org/r/4607/#comment14329>

    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.


- Michael


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
>
>
> 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