Mailing-List: contact commits-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hbase.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: liyu@apache.org
To: commits@hbase.apache.org
Message-Id: <92b5ff44a7ce43d2b3856292153e137e@git.apache.org>
Subject: hbase git commit: HBASE-16194 Should count in MSLAB chunk allocation
 into heap size change when adding duplicate cells
Date: Mon, 11 Jul 2016 13:59:19 +0000 (UTC)
archived-at: Mon, 11 Jul 2016 13:59:21 -0000

Repository: hbase
Updated Branches:
  refs/heads/branch-1.1 73189eb80 -> 25c7dee21


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/25c7dee2
Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/25c7dee2
Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/25c7dee2

Branch: refs/heads/branch-1.1
Commit: 25c7dee21e7060f3be586bcccc05539ae5954c0a
Parents: 73189eb
Author: Yu Li <liyu@apache.org>
Authored: Sun Jul 10 14:09:02 2016 +0800
Committer: Yu Li <liyu@apache.org>
Committed: Mon Jul 11 21:46:55 2016 +0800

----------------------------------------------------------------------
 .../hbase/regionserver/DefaultMemStore.java     | 38 ++++++++++++++------
 .../hbase/regionserver/HeapMemStoreLAB.java     | 11 ++++++
 .../hbase/regionserver/TestDefaultMemStore.java | 21 +++++++++++
 3 files changed, 60 insertions(+), 10 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/hbase/blob/25c7dee2/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java
index 3da0c0b..7787b23 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java
@@ -49,6 +49,8 @@ import org.apache.hadoop.hbase.util.Pair;
 import org.apache.hadoop.hbase.util.ReflectionUtils;
 import org.apache.htrace.Trace;
 
+import com.google.common.annotations.VisibleForTesting;
+
 /**
  * The MemStore holds in-memory modifications to the Store.  Modifications
  * are {@link Cell}s.  When asked to flush, current memstore is moved
@@ -223,7 +225,8 @@ public class DefaultMemStore implements MemStore {
   @Override
   public Pair<Long, Cell> add(Cell cell) {
     Cell toAdd = maybeCloneWithAllocator(cell);
-    return new Pair<Long, Cell>(internalAdd(toAdd), toAdd);
+    boolean mslabUsed = (toAdd != cell);
+    return new Pair<Long, Cell>(internalAdd(toAdd, mslabUsed), toAdd);
   }
 
   @Override
@@ -254,20 +257,38 @@ public class DefaultMemStore 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 mslabUsed whether using MSLAB
+   * @return the heap size change in bytes
    */
-  private long internalAdd(final Cell toAdd) {
-    long s = heapSizeChange(toAdd, addToCellSet(toAdd));
+  private long internalAdd(final Cell toAdd, boolean mslabUsed) {
+    boolean notPresent = addToCellSet(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}
+   */
+  @VisibleForTesting
+  int getCellLength(Cell cell) {
+    return KeyValueUtil.length(cell);
+  }
+
   private Cell maybeCloneWithAllocator(Cell cell) {
     if (allocator == null) {
       return cell;
     }
 
-    int len = KeyValueUtil.length(cell);
+    int len = getCellLength(cell);
     ByteRange alloc = allocator.allocateBytes(len);
     if (alloc == null) {
       // The allocation was too large, allocator decided
@@ -318,12 +339,9 @@ public class DefaultMemStore implements MemStore {
    */
   @Override
   public long delete(Cell deleteCell) {
-    long s = 0;
     Cell toAdd = maybeCloneWithAllocator(deleteCell);
-    s += heapSizeChange(toAdd, addToCellSet(toAdd));
-    timeRangeTracker.includeTimestamp(toAdd);
-    this.size.addAndGet(s);
-    return s;
+    boolean mslabUsed = (toAdd != deleteCell);
+    return internalAdd(toAdd, mslabUsed);
   }
 
   /**
@@ -564,7 +582,7 @@ public class DefaultMemStore 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

http://git-wip-us.apache.org/repos/asf/hbase/blob/25c7dee2/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/25c7dee2/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 4d37520..8df25eb 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
@@ -88,6 +88,27 @@ public class TestDefaultMemStore 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).getFirst();
+    long sizeChangeForSecondCell = this.memstore.add(kv).getFirst();
+    // make sure memstore size increase won't double-count MSLAB chunk size
+    assertEquals(DefaultMemStore.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
+      if (this.memstore.allocator instanceof HeapMemStoreLAB) {
+        assertEquals(2 * memstore.getCellLength(kv),
+          ((HeapMemStoreLAB) 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