HBASE-21314 The implementation of BitSetNode is not efficient

hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/store/BitSetNode.java

@@ -57,7 +57,11 @@ class BitSetNode {
   private static final long WORD_MASK = 0xffffffffffffffffL;
   private static final int ADDRESS_BITS_PER_WORD = 6;
   private static final int BITS_PER_WORD = 1 << ADDRESS_BITS_PER_WORD;
-  private static final int MAX_NODE_SIZE = 1 << ADDRESS_BITS_PER_WORD;
+  // The BitSetNode itself has 48 bytes overhead, which is the size of 6 longs, so here we use a max
+  // node size 4, which is 8 longs since we have an array for modified and also an array for
+  // deleted. The assumption here is that most procedures will be deleted soon so we'd better keep
+  // the BitSetNode small.
+  private static final int MAX_NODE_SIZE = 4 << ADDRESS_BITS_PER_WORD;
 
   /**
    * Mimics {@link ProcedureStoreTracker#partial}. It will effect how we fill the new deleted bits
@@ -162,6 +166,9 @@ class BitSetNode {
     return start;
   }
 
+  /**
+   * Inclusive.
+   */
   public long getEnd() {
     return start + (modified.length << ADDRESS_BITS_PER_WORD) - 1;
   }
@@ -176,6 +183,8 @@ class BitSetNode {
     if (wordIndex >= deleted.length) {
       return DeleteState.MAYBE;
     }
+    // The left shift of java only takes care of the lowest several bits(5 for int and 6 for long),
+    // so here we can use bitmapIndex directly, without mod 64
     return (deleted[wordIndex] & (1L << bitmapIndex)) != 0 ? DeleteState.YES : DeleteState.NO;
   }
 
@@ -185,6 +194,8 @@ class BitSetNode {
     if (wordIndex >= modified.length) {
       return false;
     }
+    // The left shift of java only takes care of the lowest several bits(5 for int and 6 for long),
+    // so here we can use bitmapIndex directly, without mod 64
     return (modified[wordIndex] & (1L << bitmapIndex)) != 0;
   }
 
@@ -269,72 +280,72 @@ class BitSetNode {
   // ========================================================================
   // Grow/Merge Helpers
   // ========================================================================
-  public boolean canGrow(final long procId) {
+  public boolean canGrow(long procId) {
-    return Math.abs(procId - start) < MAX_NODE_SIZE;
+    if (procId <= start) {
+      return getEnd() - procId < MAX_NODE_SIZE;
+    } else {
+      return procId - start < MAX_NODE_SIZE;
+    }
   }
 
-  public boolean canMerge(final BitSetNode rightNode) {
+  public boolean canMerge(BitSetNode rightNode) {
     // Can just compare 'starts' since boundaries are aligned to multiples of BITS_PER_WORD.
     assert start < rightNode.start;
     return (rightNode.getEnd() - start) < MAX_NODE_SIZE;
   }
 
-  public void grow(final long procId) {
+  public void grow(long procId) {
-    int delta, offset;
+    // make sure you have call canGrow first before calling this method
-
+    assert canGrow(procId);
     if (procId < start) {
-      // add to head
+      // grow to left
       long newStart = alignDown(procId);
-      delta = (int) (start - newStart) >> ADDRESS_BITS_PER_WORD;
+      int delta = (int) (start - newStart) >> ADDRESS_BITS_PER_WORD;
-      offset = delta;
       start = newStart;
+      long[] newModified = new long[modified.length + delta];
+      System.arraycopy(modified, 0, newModified, delta, modified.length);
+      modified = newModified;
+      long[] newDeleted = new long[deleted.length + delta];
+      if (!partial) {
+        for (int i = 0; i < delta; i++) {
+          newDeleted[i] = WORD_MASK;
+        }
+      }
+      System.arraycopy(deleted, 0, newDeleted, delta, deleted.length);
+      deleted = newDeleted;
     } else {
-      // Add to tail
+      // grow to right
       long newEnd = alignUp(procId + 1);
-      delta = (int) (newEnd - getEnd()) >> ADDRESS_BITS_PER_WORD;
+      int delta = (int) (newEnd - getEnd()) >> ADDRESS_BITS_PER_WORD;
-      offset = 0;
+      int newSize = modified.length + delta;
+      long[] newModified = Arrays.copyOf(modified, newSize);
+      modified = newModified;
+      long[] newDeleted = Arrays.copyOf(deleted, newSize);
+      if (!partial) {
+        for (int i = deleted.length; i < newSize; i++) {
+          newDeleted[i] = WORD_MASK;
+        }
+      }
+      deleted = newDeleted;
     }
-
-    long[] newBitmap;
-    int oldSize = modified.length;
-
-    newBitmap = new long[oldSize + delta];
-    for (int i = 0; i < newBitmap.length; ++i) {
-      newBitmap[i] = 0;
-    }
-    System.arraycopy(modified, 0, newBitmap, offset, oldSize);
-    modified = newBitmap;
-
-    newBitmap = new long[deleted.length + delta];
-    for (int i = 0; i < newBitmap.length; ++i) {
-      newBitmap[i] = partial ? 0 : WORD_MASK;
-    }
-    System.arraycopy(deleted, 0, newBitmap, offset, oldSize);
-    deleted = newBitmap;
   }
 
-  public void merge(final BitSetNode rightNode) {
+  public void merge(BitSetNode rightNode) {
-    int delta = (int) (rightNode.getEnd() - getEnd()) >> ADDRESS_BITS_PER_WORD;
+    assert start < rightNode.start;
-
+    int newSize = (int) (rightNode.getEnd() - start + 1) >> ADDRESS_BITS_PER_WORD;
-    long[] newBitmap;
+    long[] newModified = Arrays.copyOf(modified, newSize);
-    int oldSize = modified.length;
+    System.arraycopy(rightNode.modified, 0, newModified, newSize - rightNode.modified.length,
-    int newSize = (delta - rightNode.modified.length);
+      rightNode.modified.length);
-    int offset = oldSize + newSize;
+    long[] newDeleted = Arrays.copyOf(deleted, newSize);
-
+    System.arraycopy(rightNode.deleted, 0, newDeleted, newSize - rightNode.deleted.length,
-    newBitmap = new long[oldSize + delta];
+      rightNode.deleted.length);
-    System.arraycopy(modified, 0, newBitmap, 0, oldSize);
+    if (!partial) {
-    System.arraycopy(rightNode.modified, 0, newBitmap, offset, rightNode.modified.length);
+      for (int i = deleted.length, n = newSize - rightNode.deleted.length; i < n; i++) {
-    modified = newBitmap;
+        newDeleted[i] = WORD_MASK;
-
+      }
-    newBitmap = new long[oldSize + delta];
-    System.arraycopy(deleted, 0, newBitmap, 0, oldSize);
-    System.arraycopy(rightNode.deleted, 0, newBitmap, offset, rightNode.deleted.length);
-    deleted = newBitmap;
-
-    for (int i = 0; i < newSize; ++i) {
-      modified[offset + i] = 0;
-      deleted[offset + i] = partial ? 0 : WORD_MASK;
     }
+    modified = newModified;
+    deleted = newDeleted;
   }
 
   @Override

hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/store/TestBitSetNode.java

@@ -18,9 +18,12 @@
 package org.apache.hadoop.hbase.procedure2.store;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.procedure2.Procedure;
+import org.apache.hadoop.hbase.procedure2.store.ProcedureStoreTracker.DeleteState;
 import org.apache.hadoop.hbase.testclassification.MasterTests;
 import org.apache.hadoop.hbase.testclassification.SmallTests;
 import org.junit.ClassRule;
@@ -56,4 +59,49 @@ public class TestBitSetNode {
     assertEquals(Procedure.NO_PROC_ID, node.getActiveMinProcId());
     assertEquals(Procedure.NO_PROC_ID, node.getActiveMaxProcId());
   }
+
+  @Test
+  public void testGrow() {
+    BitSetNode node = new BitSetNode(1000, false);
+    // contains, do not need to grow but should not fail
+    assertTrue(node.canGrow(1024));
+    assertTrue(node.canGrow(900));
+    assertTrue(node.canGrow(1100));
+    assertFalse(node.canGrow(100));
+    assertFalse(node.canGrow(10000));
+
+    // grow to right
+    node.grow(1100);
+    assertTrue(node.contains(1100));
+    assertTrue(node.isModified(1000));
+    // grow to left
+    node.grow(900);
+    assertTrue(node.contains(900));
+    assertTrue(node.isModified(1000));
+    for (long i = node.getStart(); i <= node.getEnd(); i++) {
+      if (i != 1000) {
+        assertEquals(DeleteState.YES, node.isDeleted(i));
+      } else {
+        assertEquals(DeleteState.NO, node.isDeleted(i));
+      }
+    }
+  }
+
+  @Test
+  public void testMerge() {
+    BitSetNode node = new BitSetNode(1000, false);
+    assertTrue(node.canMerge(new BitSetNode(1200, false)));
+    assertFalse(node.canMerge(new BitSetNode(10000, false)));
+    BitSetNode rightNode = new BitSetNode(1200, false);
+    node.merge(rightNode);
+    assertTrue(node.isModified(1000));
+    assertTrue(node.isModified(1200));
+    for (long i = node.getStart(); i <= node.getEnd(); i++) {
+      if (i != 1000 && i != 1200) {
+        assertEquals(DeleteState.YES, node.isDeleted(i));
+      } else {
+        assertEquals(DeleteState.NO, node.isDeleted(i));
+      }
+    }
+  }
 }