You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/06/08 02:18:12 UTC
[GitHub] [hbase] bharathv commented on a change in pull request #1257: HBASE 23887 Up to 3x increase BlockCache performance
bharathv commented on a change in pull request #1257:
URL: https://github.com/apache/hbase/pull/1257#discussion_r436423732
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
##########
@@ -153,6 +153,18 @@
private static final String LRU_MAX_BLOCK_SIZE = "hbase.lru.max.block.size";
private static final long DEFAULT_MAX_BLOCK_SIZE = 16L * 1024L * 1024L;
+ private static final String LRU_CACHE_HEAVY_EVICTION_COUNT_LIMIT
+ = "hbase.lru.cache.heavy.eviction.count.limit";
+ private static final int DEFAULT_LRU_CACHE_HEAVY_EVICTION_COUNT_LIMIT = 10;
+
+ private static final String LRU_CACHE_HEAVY_EVICTION_MB_SIZE_LIMIT
+ = "hbase.lru.cache.heavy.eviction.mb.size.limit";
+ private static final long DEFAULT_LRU_CACHE_HEAVY_EVICTION_MB_SIZE_LIMIT = 500;
+
+ private static final String LRU_CACHE_HEAVY_EVICTION_OVERHEAD_COEFFICIENT
+ = "hbase.lru.cache.heavy.eviction.overhead.coefficient";
+ private static final float DEFAULT_LRU_CACHE_HEAVY_EVICTION_OVERHEAD_COEFFICIENT = 0.01f;
+
/**
Review comment:
I think it would be preferable to have defaults with the optimization disabled (meaning 100% cache block %) to avoid any surprises during upgrade. They are tunable anyway, so we can have some documentation around it, for those who are interested in further tuning the configs.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
##########
@@ -400,6 +451,16 @@ private Cacheable asReferencedHeapBlock(Cacheable buf) {
*/
@Override
public void cacheBlock(BlockCacheKey cacheKey, Cacheable buf, boolean inMemory) {
+
+ // Don't cache this DATA block when too many blocks evict
+ // and if we have limit on percent of blocks to cache.
Review comment:
Is this comment stale given your new approach to 'autoscale' ?
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java
##########
@@ -1026,8 +1045,81 @@ public void testMultiThreadGetAndEvictBlock() throws Exception {
0.33f, // multi
0.34f, // memory
1.2f, // limit
- false, 1024);
+ false, 1024,
+ 10,
+ 500,
+ 0.01f);
testMultiThreadGetAndEvictBlockInternal(cache);
}
-}
+ public void testSkipCacheDataBlocksInteral(int heavyEvictionCountLimit) throws Exception {
+ long maxSize = 100000000;
+ int numBlocks = 100000;
+ final long blockSize = calculateBlockSizeDefault(maxSize, numBlocks);
+ assertTrue("calculateBlockSize appears broken.", blockSize * numBlocks <= maxSize);
+
+ final LruBlockCache cache =
+ new LruBlockCache(maxSize, blockSize, true, (int) Math.ceil(1.2 * maxSize / blockSize),
Review comment:
nit: indents..
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
##########
@@ -655,23 +716,29 @@ long getOverhead() {
/**
* Eviction method.
*/
Review comment:
Update javadoc about the return value.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
##########
@@ -947,7 +1019,47 @@ public void run() {
}
LruBlockCache cache = this.cache.get();
if (cache == null) break;
- cache.evict();
+ bytesFreed = cache.evict();
+ long stopTime = System.currentTimeMillis();
+ // If heavy cleaning BlockCache control.
+ // It helps avoid put too many blocks into BlockCache
+ // when evict() works very active.
+ if (stopTime - startTime <= 1000 * 10 - 1) {
+ mbFreedSum += bytesFreed/1024/1024;
+ } else {
+ freedDataOverheadPercent =
Review comment:
Whats the -100 part in the ending there? I think what you are computing here is what % of evictionSizeMbLimit that is filled?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
##########
@@ -947,7 +1019,47 @@ public void run() {
}
LruBlockCache cache = this.cache.get();
if (cache == null) break;
- cache.evict();
+ bytesFreed = cache.evict();
+ long stopTime = System.currentTimeMillis();
+ // If heavy cleaning BlockCache control.
+ // It helps avoid put too many blocks into BlockCache
+ // when evict() works very active.
+ if (stopTime - startTime <= 1000 * 10 - 1) {
Review comment:
I don't understand the first 'if', what does it mean if the eviction takes less that 10s? Also whats specific about 10s?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
##########
@@ -153,6 +153,18 @@
private static final String LRU_MAX_BLOCK_SIZE = "hbase.lru.max.block.size";
private static final long DEFAULT_MAX_BLOCK_SIZE = 16L * 1024L * 1024L;
+ private static final String LRU_CACHE_HEAVY_EVICTION_COUNT_LIMIT
Review comment:
nit: fix indents. 2/4 rule. 2 for nested and 4 for continuous indents..
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
##########
@@ -947,7 +1019,47 @@ public void run() {
}
LruBlockCache cache = this.cache.get();
if (cache == null) break;
- cache.evict();
+ bytesFreed = cache.evict();
+ long stopTime = System.currentTimeMillis();
+ // If heavy cleaning BlockCache control.
+ // It helps avoid put too many blocks into BlockCache
+ // when evict() works very active.
+ if (stopTime - startTime <= 1000 * 10 - 1) {
+ mbFreedSum += bytesFreed/1024/1024;
+ } else {
+ freedDataOverheadPercent =
+ (int) (mbFreedSum * 100 / cache.heavyEvictionMbSizeLimit) - 100;
+ if (mbFreedSum > cache.heavyEvictionMbSizeLimit) {
+ heavyEvictionCount++;
+ if (heavyEvictionCount > cache.heavyEvictionCountLimit) {
+ int ch = (int) (freedDataOverheadPercent * cache.heavyEvictionOverheadCoefficient);
+ ch = ch > 15 ? 15 : ch;
Review comment:
I don't understand this part (if/else)...again I think comments could help..
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
##########
@@ -338,6 +373,17 @@ public LruBlockCache(long maxSize, long blockSize, boolean evictionThread,
} else {
this.evictionThread = null;
}
+
+ // check the bounds
+ this.heavyEvictionCountLimit = heavyEvictionCountLimit < 0 ? 0 : heavyEvictionCountLimit;
+ this.heavyEvictionMbSizeLimit = heavyEvictionMbSizeLimit < 1 ? 1 : heavyEvictionMbSizeLimit;
+ this.cacheDataBlockPercent = 100;
Review comment:
Hardcoded? I think the definition of this changed since your lat version.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
##########
@@ -947,7 +1019,47 @@ public void run() {
}
LruBlockCache cache = this.cache.get();
if (cache == null) break;
- cache.evict();
+ bytesFreed = cache.evict();
+ long stopTime = System.currentTimeMillis();
+ // If heavy cleaning BlockCache control.
Review comment:
I think this needs a detailed comment about when/how the optimization kicks in and some notes on the intuition (preferably with a jira reference).. The comment can either be here or in the class comment javadoc.
----------------------------------------------------------------
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:
users@infra.apache.org