hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-4027) Enable direct byte buffers LruBlockCache
Date Fri, 12 Aug 2011 17:16:34 GMT

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

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



bq.  On 2011-08-09 22:05:13, Todd Lipcon wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java, line 64
bq.  > <https://reviews.apache.org/r/1214/diff/8/?file=31767#file31767line64>
bq.  >
bq.  >     does it ever make sense to have offHeapSize < onHeapSize? Perhaps we should
have a Preconditions check here?
bq.  
bq.  Li Pi wrote:
bq.      Its useful for testing, I guess? And 8gb heap with 2gb of offheap > 8gb heap without
2gb of offheap.

is that true? wouldn't the 2G offheap always have a subset of what's in heap?


bq.  On 2011-08-09 22:05:13, Todd Lipcon wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java, line 67
bq.  > <https://reviews.apache.org/r/1214/diff/8/?file=31767#file31767line67>
bq.  >
bq.  >     missing space - " bytes ..."
bq.  
bq.  Li Pi wrote:
bq.      The string would end up being: "Creating off-heap cache of size 1.9G bytes. Should
it be different?

oh, I see - you want it to say "1.9mbytes" instead of "1.9m bytes". That's fine


bq.  On 2011-08-09 22:05:13, Todd Lipcon wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java, lines
60-67
bq.  > <https://reviews.apache.org/r/1214/diff/8/?file=31771#file31771line60>
bq.  >
bq.  >     vertically collapse this - one line per param
bq.  
bq.  Li Pi wrote:
bq.      80 char limit?.  I don't know why eclipse keeps doing this. Will fix!

i think it'll still fit in 80 chars without all that vertical space


bq.  On 2011-08-09 22:05:13, Todd Lipcon wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java, lines
101-103
bq.  > <https://reviews.apache.org/r/1214/diff/8/?file=31771#file31771line101>
bq.  >
bq.  >     when you check up front here, you end up doing two lookups in backingmap. Since
this is just a safety check, you could instead check the return value of put() below. Something
like:
bq.  >     
bq.  >     ByteBuffer storedBlock = ...allloc
bq.  >     ... fill it in...
bq.  >     ByteBuffer alreadyCached = backingMap.put(blockName, storedBlock);
bq.  >     if (alreadyCached != null) {
bq.  >       // we didn't insert the new one, so free it and throw an exception
bq.  >       backingStore.free(storedBlock);
bq.  >       throw new RuntimeException("already cached xxxxx");
bq.  >     }
bq.  >     
bq.  >     make sense?
bq.  
bq.  Li Pi wrote:
bq.      Doesn't put overwrite the previous value? I guess in this case, it doesn't matter,
because you'd just be caching the same thing twice.  According to mapmaker: "If the map previously
contained a mapping for the key, the old value is replaced by the specified value."
bq.      
bq.      Good change, still though, I changed it to free alreadyCached instead.

ah, sorry, I meant putIfAbsent from ConcurrentMap. If you free alreadyCached, you might free
something that someone's using, right?


bq.  On 2011-08-09 22:05:13, Todd Lipcon wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java, lines
118-119
bq.  > <https://reviews.apache.org/r/1214/diff/8/?file=31771#file31771line118>
bq.  >
bq.  >     I think there's a bug here if you have multiple users hammering the same contentBlock
-- two people can get to "rewind()" at the same time. You probably need synchronized(contentBlock)
around these two lines. See if you can add a unit test which puts just one block in the cache
and starts several threads which hammer it - I bet you eventually one of the blocks comes
back returned as all 0x0000
bq.  
bq.  Li Pi wrote:
bq.      changed it to     returnBlock.put(contentBlock.duplicate()) instead.

don't you need to duplicate it before you call rewind? also, did you add a test that catches
this bug, in case there are other similar bugs that I didn't spot?


bq.  On 2011-08-09 22:05:13, Todd Lipcon wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java, line 114
bq.  > <https://reviews.apache.org/r/1214/diff/8/?file=31773#file31773line114>
bq.  >
bq.  >     > 0, not == 1 -- the contract of compareTo is just that it returns positive,
not that it returns exactly 1
bq.  
bq.  Li Pi wrote:
bq.      http://download.oracle.com/javase/6/docs/api/java/math/BigDecimal.html#compareTo(java.math.BigDecimal)
bq.      
bq.      Returns:
bq.      -1, 0, or 1 as this BigDecimal is numerically less than, equal to, or greater than
val.
bq.

http://download.oracle.com/javase/1.4.2/docs/api/java/lang/Comparable.html#compareTo(java.lang.Object)

"Returns:
a negative integer, zero, or a positive integer as this object is less than, equal to, or
greater than the specified object."

so even though sun's BigDecimal happens to return exactly 1, the convention is to check compareTo(...)
> 0, because the usual interface is pos/zero/negative, not 1/0/-1


bq.  On 2011-08-09 22:05:13, Todd Lipcon wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCacheTestUtils.java, line
45
bq.  > <https://reviews.apache.org/r/1214/diff/8/?file=31780#file31780line45>
bq.  >
bq.  >     hrm, this is identical to the other method?
bq.  
bq.  Li Pi wrote:
bq.      Ones for SingleSizeCache, which takes ByteBuffers, the other is for a BlockCache,
which (hypothetically) takes anything with HeapSize.

but you should be able to extract a method, even if the method has to take a type parameter.
too much dup code


- Todd


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


On 2011-08-12 08:41:37, Li Pi wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/1214/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2011-08-12 08:41:37)
bq.  
bq.  
bq.  Review request for hbase, Todd Lipcon, Ted Yu, Michael Stack, Jonathan Gray, and Li Pi.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  Review request - I apparently can't edit tlipcon's earlier posting of my diff, so creating
a new one.
bq.  
bq.  
bq.  This addresses bug HBase-4027.
bq.      https://issues.apache.org/jira/browse/HBase-4027
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    conf/hbase-env.sh 2d55d27 
bq.    src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java 2d4002c 
bq.    src/main/java/org/apache/hadoop/hbase/io/hfile/CacheStats.java PRE-CREATION 
bq.    src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java PRE-CREATION 
bq.    src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java 097dc50 
bq.    src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java 1338453 
bq.    src/main/java/org/apache/hadoop/hbase/io/hfile/SimpleBlockCache.java 886c31d 
bq.    src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java PRE-CREATION

bq.    src/main/java/org/apache/hadoop/hbase/io/hfile/slab/Slab.java PRE-CREATION 
bq.    src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java PRE-CREATION 
bq.    src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabItemEvictionWatcher.java PRE-CREATION

bq.    src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 7a917da 
bq.    src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 7b7bf73 
bq.    src/main/java/org/apache/hadoop/hbase/util/DirectMemoryUtils.java PRE-CREATION 
bq.    src/test/java/org/apache/hadoop/hbase/io/hfile/HFileBlockCacheTestUtils.java PRE-CREATION

bq.    src/test/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCacheTestUtils.java PRE-CREATION

bq.    src/test/java/org/apache/hadoop/hbase/io/hfile/slab/TestSingleSizeCache.java PRE-CREATION

bq.    src/test/java/org/apache/hadoop/hbase/io/hfile/slab/TestSlab.java PRE-CREATION 
bq.    src/test/java/org/apache/hadoop/hbase/io/hfile/slab/TestSlabCache.java PRE-CREATION

bq.    src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java 4387170 
bq.  
bq.  Diff: https://reviews.apache.org/r/1214/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  Ran benchmarks against it in HBase standalone mode. Wrote test cases for all classes,
multithreaded test cases exist for the cache.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Li
bq.  
bq.



> Enable direct byte buffers LruBlockCache
> ----------------------------------------
>
>                 Key: HBASE-4027
>                 URL: https://issues.apache.org/jira/browse/HBASE-4027
>             Project: HBase
>          Issue Type: Improvement
>            Reporter: Jason Rutherglen
>            Assignee: Li Pi
>            Priority: Minor
>         Attachments: 4027-v5.diff, 4027v7.diff, HBase-4027 (1).pdf, HBase-4027.pdf, HBase4027v8.diff,
HBase4027v9.diff, hbase-4027-v10.5.diff, hbase-4027-v10.diff, hbase-4027v10.6.diff, hbase-4027v6.diff,
hbase4027v11.5.diff, hbase4027v11.diff, slabcachepatch.diff, slabcachepatchv2.diff, slabcachepatchv3.1.diff,
slabcachepatchv3.2.diff, slabcachepatchv3.diff, slabcachepatchv4.5.diff, slabcachepatchv4.diff
>
>
> Java offers the creation of direct byte buffers which are allocated outside of the heap.
> They need to be manually free'd, which can be accomplished using an documented {{clean}}
method.
> The feature will be optional.  After implementing, we can benchmark for differences in
speed and garbage collection observances.

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message