From 9cf012cd0086e054a66f54cb54771af2381fade0 Mon Sep 17 00:00:00 2001 From: Yu Li Date: Sun, 10 Jul 2016 13:45:17 +0800 Subject: [PATCH] HBASE-16194 Should count in MSLAB chunk allocation into heap size change when adding duplicate cells --- .../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 +++++++++++++--- .../regionserver/TestDefaultMemStore.java | 26 +++++++++++++++++++ 6 files changed, 73 insertions(+), 11 deletions(-) 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 d25f96065e3..79e95af0fde 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; 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 f22a6e5c1ac..625811af3a4 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(); + } } } 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 65d3af634a1..23f792bd170 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(); 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 63376574f4c..6e54060b50a 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); } /** 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 dd824c1f95d..adb101efffb 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; } 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 16144620918..7285c674da9 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