Repository: hbase Updated Branches: refs/heads/0.98 ecc1a886e -> ad064c718
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/ad064c71 Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/ad064c71 Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/ad064c71 Branch: refs/heads/0.98 Commit: ad064c7184ff17792c31318e9c6838cf723f191f Parents: ecc1a88 Author: Yu Li <[email protected]> Authored: Tue Jul 12 16:58:23 2016 +0800 Committer: Yu Li <[email protected]> Committed: Tue Jul 12 16:58:23 2016 +0800 ---------------------------------------------------------------------- .../hadoop/hbase/regionserver/MemStore.java | 38 +++++++++++++++----- .../hadoop/hbase/regionserver/MemStoreLAB.java | 11 ++++++ .../hadoop/hbase/regionserver/TestMemStore.java | 19 ++++++++++ 3 files changed, 59 insertions(+), 9 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hbase/blob/ad064c71/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java index 549f15e..9c63fc5 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java @@ -47,6 +47,8 @@ import org.apache.hadoop.hbase.util.ClassSize; import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.cloudera.htrace.Trace; +import com.google.common.annotations.VisibleForTesting; + /** * The MemStore holds in-memory modifications to the Store. Modifications * are {@link KeyValue}s. When asked to flush, current memstore is moved @@ -228,7 +230,8 @@ public class MemStore implements HeapSize { */ long add(final KeyValue kv) { KeyValue toAdd = maybeCloneWithAllocator(kv); - return internalAdd(toAdd); + boolean mslabUsed = (toAdd != kv); + return internalAdd(toAdd, mslabUsed); } long timeOfOldestEdit() { @@ -258,14 +261,34 @@ public class MemStore implements HeapSize { * allocator, and doesn't take the lock. * * Callers should ensure they already have the read lock taken + * @param toAdd the kv to add + * @param mslabUsed whether MSLAB is used for the kv + * @return the heap size change in bytes */ - private long internalAdd(final KeyValue toAdd) { - long s = heapSizeChange(toAdd, addToKVSet(toAdd)); + private long internalAdd(final KeyValue toAdd, boolean mslabUsed) { + boolean notPresent = addToKVSet(toAdd); + long s = heapSizeChange(toAdd, notPresent); + // 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 (!notPresent && mslabUsed) { + s += getCellLength(toAdd); + } timeRangeTracker.includeTimestamp(toAdd); this.size.addAndGet(s); return s; } + /** + * Get cell length after serialized in {@link KeyValue} + * @param cell The cell to get length + * @return the serialized cell length + */ + @VisibleForTesting + int getCellLength(Cell cell) { + return KeyValueUtil.length(cell); + } + private KeyValue maybeCloneWithAllocator(KeyValue kv) { if (allocator == null) { return kv; @@ -320,12 +343,9 @@ public class MemStore implements HeapSize { * @return approximate size of the passed key and value. */ long delete(final KeyValue delete) { - long s = 0; KeyValue toAdd = maybeCloneWithAllocator(delete); - s += heapSizeChange(toAdd, addToKVSet(toAdd)); - timeRangeTracker.includeTimestamp(toAdd); - this.size.addAndGet(s); - return s; + boolean mslabUsed = (toAdd != delete); + return internalAdd(toAdd, mslabUsed); } /** @@ -564,7 +584,7 @@ public class MemStore implements HeapSize { // test that triggers the pathological case if we don't avoid MSLAB // here. KeyValue kv = KeyValueUtil.ensureKeyValue(cell); - long addedSize = internalAdd(kv); + long addedSize = internalAdd(kv, false); // Get the KeyValues for the row/family/qualifier regardless of timestamp. // For this case we want to clean up any other puts http://git-wip-us.apache.org/repos/asf/hbase/blob/ad064c71/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreLAB.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreLAB.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreLAB.java index d4e96e8..4f776a6 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreLAB.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreLAB.java @@ -27,6 +27,7 @@ import java.util.concurrent.atomic.AtomicReference; import org.apache.hadoop.hbase.classification.InterfaceAudience; import org.apache.hadoop.conf.Configuration; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; /** @@ -204,6 +205,11 @@ public class MemStoreLAB { } } + @VisibleForTesting + Chunk getCurrentChunk() { + return this.curChunk.get(); + } + /** * A chunk of memory out of which allocations are sliced. */ @@ -309,6 +315,11 @@ public class 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/ad064c71/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java index 7fdade9..04dce36 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java @@ -85,6 +85,25 @@ public class TestMemStore extends TestCase { assertTrue(Bytes.toString(found.getValue()), CellUtil.matchingValue(samekey, found)); } + 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(MemStore.heapSizeChange(kv, true), sizeChangeForFirstCell); + if (this.memstore.allocator != null) { + // make sure memstore size increased when using MSLAB + assertEquals(memstore.getCellLength(kv), sizeChangeForSecondCell); + // make sure chunk size increased even when writing the same cell, if using MSLAB + assertEquals(2 * memstore.getCellLength(kv), + this.memstore.allocator.getCurrentChunk().getNextFreeOffset()); + } else { + // make sure no memstore size change w/o MSLAB + assertEquals(0, sizeChangeForSecondCell); + } + } + /** * Test memstore snapshot happening while scanning. * @throws IOException
