You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by li...@apache.org on 2016/07/10 06:25:45 UTC
hbase git commit: HBASE-16194 Should count in MSLAB chunk allocation
into heap size change when adding duplicate cells
Repository: hbase
Updated Branches:
refs/heads/master 632969787 -> 9cf012cd0
HBASE-16194 Should count in MSLAB chunk allocation into heap size change when adding duplicate cells
Project: http://git-wip-us.apache.org/repos/asf/hbase/repo
Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/9cf012cd
Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/9cf012cd
Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/9cf012cd
Branch: refs/heads/master
Commit: 9cf012cd0086e054a66f54cb54771af2381fade0
Parents: 6329697
Author: Yu Li <li...@apache.org>
Authored: Sun Jul 10 13:45:17 2016 +0800
Committer: Yu Li <li...@apache.org>
Committed: Sun Jul 10 13:45:17 2016 +0800
----------------------------------------------------------------------
.../hbase/regionserver/AbstractMemStore.java | 15 +++++++----
.../hbase/regionserver/HeapMemStoreLAB.java | 11 +++++++++
.../hbase/regionserver/MemStoreCompactor.java | 3 ++-
.../hbase/regionserver/MutableSegment.java | 6 +++--
.../hadoop/hbase/regionserver/Segment.java | 23 ++++++++++++++---
.../hbase/regionserver/TestDefaultMemStore.java | 26 ++++++++++++++++++++
6 files changed, 73 insertions(+), 11 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/hbase/blob/9cf012cd/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/AbstractMemStore.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/AbstractMemStore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/AbstractMemStore.java
index d25f960..79e95af 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/AbstractMemStore.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/AbstractMemStore.java
@@ -109,7 +109,8 @@ public abstract class AbstractMemStore implements MemStore {
@Override
public long add(Cell cell) {
Cell toAdd = maybeCloneWithAllocator(cell);
- return internalAdd(toAdd);
+ boolean useMSLAB = (toAdd != cell);
+ return internalAdd(toAdd, useMSLAB);
}
/**
@@ -156,7 +157,8 @@ public abstract class AbstractMemStore implements MemStore {
@Override
public long delete(Cell deleteCell) {
Cell toAdd = maybeCloneWithAllocator(deleteCell);
- long s = internalAdd(toAdd);
+ boolean useMSLAB = (toAdd != deleteCell);
+ long s = internalAdd(toAdd, useMSLAB);
return s;
}
@@ -243,7 +245,7 @@ public abstract class AbstractMemStore implements MemStore {
// hitting OOME - see TestMemStore.testUpsertMSLAB for a
// test that triggers the pathological case if we don't avoid MSLAB
// here.
- long addedSize = internalAdd(cell);
+ long addedSize = internalAdd(cell, false);
// Get the Cells for the row/family/qualifier regardless of timestamp.
// For this case we want to clean up any other puts
@@ -387,9 +389,12 @@ public abstract class AbstractMemStore implements MemStore {
* allocator, and doesn't take the lock.
*
* Callers should ensure they already have the read lock taken
+ * @param toAdd the cell to add
+ * @param useMSLAB whether using MSLAB
+ * @return the heap size change in bytes
*/
- private long internalAdd(final Cell toAdd) {
- long s = active.add(toAdd);
+ private long internalAdd(final Cell toAdd, final boolean useMSLAB) {
+ long s = active.add(toAdd, useMSLAB);
setOldestEditTimeToNow();
checkActiveSize();
return s;
http://git-wip-us.apache.org/repos/asf/hbase/blob/9cf012cd/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
index f22a6e5..625811a 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
@@ -29,6 +29,7 @@ import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.util.ByteRange;
import org.apache.hadoop.hbase.util.SimpleMutableByteRange;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
/**
@@ -206,6 +207,11 @@ public class HeapMemStoreLAB implements MemStoreLAB {
}
}
+ @VisibleForTesting
+ Chunk getCurrentChunk() {
+ return this.curChunk.get();
+ }
+
/**
* A chunk of memory out of which allocations are sliced.
*/
@@ -311,5 +317,10 @@ public class HeapMemStoreLAB implements MemStoreLAB {
" allocs=" + allocCount.get() + "waste=" +
(data.length - nextFreeOffset.get());
}
+
+ @VisibleForTesting
+ int getNextFreeOffset() {
+ return this.nextFreeOffset.get();
+ }
}
}
http://git-wip-us.apache.org/repos/asf/hbase/blob/9cf012cd/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
index 65d3af6..23f792b 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
@@ -197,7 +197,8 @@ class MemStoreCompactor {
// The scanner is doing all the elimination logic
// now we just copy it to the new segment
Cell newKV = result.maybeCloneWithAllocator(c);
- result.internalAdd(newKV);
+ boolean useMSLAB = (newKV != c);
+ result.internalAdd(newKV, useMSLAB);
}
kvs.clear();
http://git-wip-us.apache.org/repos/asf/hbase/blob/9cf012cd/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MutableSegment.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MutableSegment.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MutableSegment.java
index 6337657..6e54060 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MutableSegment.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MutableSegment.java
@@ -35,10 +35,12 @@ public class MutableSegment extends Segment {
/**
* Adds the given cell into the segment
+ * @param cell the cell to add
+ * @param useMSLAB whether using MSLAB
* @return the change in the heap size
*/
- public long add(Cell cell) {
- return internalAdd(cell);
+ public long add(Cell cell, boolean useMSLAB) {
+ return internalAdd(cell, useMSLAB);
}
/**
http://git-wip-us.apache.org/repos/asf/hbase/blob/9cf012cd/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Segment.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Segment.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Segment.java
index dd824c1..adb101e 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Segment.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Segment.java
@@ -31,6 +31,8 @@ import org.apache.hadoop.hbase.classification.InterfaceAudience;
import org.apache.hadoop.hbase.client.Scan;
import org.apache.hadoop.hbase.util.ByteRange;
+import com.google.common.annotations.VisibleForTesting;
+
/**
* This is an abstraction of a segment maintained in a memstore, e.g., the active
* cell set or its snapshot.
@@ -136,7 +138,7 @@ public abstract class Segment {
return cell;
}
- int len = KeyValueUtil.length(cell);
+ int len = getCellLength(cell);
ByteRange alloc = getMemStoreLAB().allocateBytes(len);
if (alloc == null) {
// The allocation was too large, allocator decided
@@ -150,6 +152,14 @@ public abstract class Segment {
return newKv;
}
+ /**
+ * Get cell length after serialized in {@link KeyValue}
+ */
+ @VisibleForTesting
+ int getCellLength(Cell cell) {
+ return KeyValueUtil.length(cell);
+ }
+
public abstract boolean shouldSeek(Scan scan, long oldestUnexpiredTS);
public abstract long getMinTimestamp();
@@ -240,9 +250,15 @@ public abstract class Segment {
return comparator;
}
- protected long internalAdd(Cell cell) {
+ protected long internalAdd(Cell cell, boolean useMSLAB) {
boolean succ = getCellSet().add(cell);
long s = AbstractMemStore.heapSizeChange(cell, succ);
+ // If there's already a same cell in the CellSet and we are using MSLAB, we must count in the
+ // MSLAB allocation size as well, or else there will be memory leak (occupied heap size larger
+ // than the counted number)
+ if (!succ && useMSLAB) {
+ s += getCellLength(cell);
+ }
updateMetaInfo(cell, s);
return s;
}
@@ -269,7 +285,8 @@ public abstract class Segment {
return getCellSet().tailSet(firstCell);
}
- private MemStoreLAB getMemStoreLAB() {
+ @VisibleForTesting
+ public MemStoreLAB getMemStoreLAB() {
return memStoreLAB;
}
http://git-wip-us.apache.org/repos/asf/hbase/blob/9cf012cd/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultMemStore.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultMemStore.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultMemStore.java
index 1614462..7285c67 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultMemStore.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultMemStore.java
@@ -21,6 +21,7 @@ package org.apache.hadoop.hbase.regionserver;
import com.google.common.base.Joiner;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.hadoop.conf.Configuration;
@@ -56,6 +57,7 @@ import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertNotNull;
+
import org.junit.experimental.categories.Category;
import org.junit.rules.TestName;
import org.junit.rules.TestRule;
@@ -110,6 +112,30 @@ public class TestDefaultMemStore {
assertTrue(Bytes.toString(found.getValueArray()), CellUtil.matchingValue(samekey, found));
}
+ @Test
+ public void testPutSameCell() {
+ byte[] bytes = Bytes.toBytes(getName());
+ KeyValue kv = new KeyValue(bytes, bytes, bytes, bytes);
+ long sizeChangeForFirstCell = this.memstore.add(kv);
+ long sizeChangeForSecondCell = this.memstore.add(kv);
+ // make sure memstore size increase won't double-count MSLAB chunk size
+ assertEquals(AbstractMemStore.heapSizeChange(kv, true), sizeChangeForFirstCell);
+ Segment segment = this.memstore.getActive();
+ MemStoreLAB msLab = segment.getMemStoreLAB();
+ if (msLab != null) {
+ // make sure memstore size increased even when writing the same cell, if using MSLAB
+ assertEquals(segment.getCellLength(kv), sizeChangeForSecondCell);
+ // make sure chunk size increased even when writing the same cell, if using MSLAB
+ if (msLab instanceof HeapMemStoreLAB) {
+ assertEquals(2 * segment.getCellLength(kv),
+ ((HeapMemStoreLAB) msLab).getCurrentChunk().getNextFreeOffset());
+ }
+ } else {
+ // make sure no memstore size change w/o MSLAB
+ assertEquals(0, sizeChangeForSecondCell);
+ }
+ }
+
/**
* Test memstore snapshot happening while scanning.
* @throws IOException