GEODE-9493: rework sizing of RedisString, RedisHash, RedisSet, and Re…

…disSortedSet (apache#6727) Removed SizeableObjectSizer and made the Sizeable redis classes abstract. They now require subclasses that implement sizeKey, sizeValue, or sizeElement. Instead of having hard coded constants stating what the size of a redis base class is, the code now uses JvmSizeUtils.memoryOverhead(Class) to statically compute the class size overhead. This allows the size to be correct for different jvm configs. Also simplified the sizing logic to use int and if needed use casts instead of narrow. Narrow was more expensive and would still cause problems as the size goes back down toward zero. The sizing of OrderStatisticsTree was simplified some more since it did not need an extra int field since we do not keep track of its element sizes. Also the size of the hashing strategy object is no longer accounted for since it is a singleton. OrderStatisticsTree has been simplified to not size the elements it contains. It is currently only used by RedisSortedSet and it does not need the element size to be computed (it already is done in the hash map it also uses). If in the future we need to also size the elements of an OrderStatisticsTree we can figure out the best way to do that then in that new context.
rnsasg · Aug 16, 2021 · 1210cbc · 1210cbc
1 parent 9bdfd13
commit 1210cbc
Show file tree

Hide file tree

Showing 26 changed files with 447 additions and 553 deletions.
diff --git a/...est/java/org/apache/geode/redis/internal/executor/hash/MemoryOverheadIntegrationTest.java b/...est/java/org/apache/geode/redis/internal/executor/hash/MemoryOverheadIntegrationTest.java
@@ -58,7 +58,7 @@ EnumMap<Measurement, Integer> expectedPerEntryOverhead() {
     result.put(Measurement.SET_ENTRY, 25);
     result.put(Measurement.HASH, 339);
     result.put(Measurement.HASH_ENTRY, 50);
-    result.put(Measurement.SORTED_SET, 455);
+    result.put(Measurement.SORTED_SET, 435);
     result.put(Measurement.SORTED_SET_ENTRY, 126);
 
     return result;

diff --git a/...ith-redis/src/integrationTest/resources/org/apache/geode/codeAnalysis/excludedClasses.txt b/...ith-redis/src/integrationTest/resources/org/apache/geode/codeAnalysis/excludedClasses.txt
@@ -5,7 +5,10 @@ org/apache/geode/redis/internal/collections/IndexibleTreeSet
 org/apache/geode/redis/internal/data/RedisDataMovedException
 org/apache/geode/redis/internal/data/RedisDataTypeMismatchException
 org/apache/geode/redis/internal/RedisException
+org/apache/geode/redis/internal/data/RedisHash$Hash
 org/apache/geode/redis/internal/data/RedisRestoreKeyExistsException
+org/apache/geode/redis/internal/data/RedisSet$MemberSet
 org/apache/geode/redis/internal/data/RedisSetCommandsFunctionExecutor$SetOp
+org/apache/geode/redis/internal/data/RedisSortedSet$MemberMap
 org/apache/geode/redis/internal/data/RedisStringCommandsFunctionExecutor$BitOp
 org/apache/geode/redis/internal/services/StripedExecutorService$State
diff --git a/...c/integrationTest/resources/org/apache/geode/codeAnalysis/sanctionedDataSerializables.txt b/...c/integrationTest/resources/org/apache/geode/codeAnalysis/sanctionedDataSerializables.txt
@@ -12,19 +12,19 @@ toData,8
 
 org/apache/geode/redis/internal/data/RedisHash,2
 toData,90
-fromData,61
+fromData,58
 
 org/apache/geode/redis/internal/data/RedisKey,2
 fromData,22
 toData,19
 
 org/apache/geode/redis/internal/data/RedisSet,2
 toData,55
-fromData,54
+fromData,51
 
 org/apache/geode/redis/internal/data/RedisSortedSet,2
 toData,93
-fromData,102
+fromData,86
 
 org/apache/geode/redis/internal/data/RedisString,2
 toData,23

diff --git a/...OpenCustomHashmapWithCursorBenchmark.java → ...al/collections/RedisHashMapBenchmark.java b/...OpenCustomHashmapWithCursorBenchmark.java → ...al/collections/RedisHashMapBenchmark.java
@@ -17,31 +17,31 @@
 import java.util.Iterator;
 import java.util.Random;
 
-import it.unimi.dsi.fastutil.bytes.ByteArrays;
 import org.openjdk.jmh.annotations.Benchmark;
 import org.openjdk.jmh.annotations.Param;
 import org.openjdk.jmh.annotations.Scope;
 import org.openjdk.jmh.annotations.Setup;
 import org.openjdk.jmh.annotations.State;
 import org.openjdk.jmh.infra.Blackhole;
 
-public class SizeableObject2ObjectOpenCustomHashmapWithCursorBenchmark {
+import org.apache.geode.redis.internal.data.RedisHash;
+
+public class RedisHashMapBenchmark {
 
   @State(Scope.Benchmark)
   public static class BenchmarkState {
     @Param({"1000", "10000", "100000"})
     private int numEntries;
     @Param({"32"})
     private int keySize;
-    private SizeableObject2ObjectOpenCustomHashMapWithCursor<byte[], byte[]> map;
+    private RedisHash.Hash map;
     private int cursor;
     private Iterator<byte[]> iterator;
 
     @Setup
     public void createMap() {
       Random random = new Random(0);
-      map = new SizeableObject2ObjectOpenCustomHashMapWithCursor<>(numEntries,
-          ByteArrays.HASH_STRATEGY);
+      map = new RedisHash.Hash(numEntries);
       for (int i = 0; i < numEntries; i++) {
         byte[] key = new byte[keySize];
         random.nextBytes(key);

diff --git a/...-redis/src/main/java/org/apache/geode/redis/internal/collections/OrderStatisticsTree.java b/...-redis/src/main/java/org/apache/geode/redis/internal/collections/OrderStatisticsTree.java
@@ -26,6 +26,7 @@
 package org.apache.geode.redis.internal.collections;
 
 
+import static org.apache.geode.internal.JvmSizeUtils.memoryOverhead;
 import static org.apache.geode.util.internal.UncheckedUtils.uncheckedCast;
 
 import java.util.Arrays;
@@ -38,7 +39,6 @@
 import java.util.Set;
 
 import org.apache.geode.annotations.VisibleForTesting;
-import org.apache.geode.redis.internal.data.SizeableObjectSizer;
 
 /**
  * This class implements an order statistic tree which is based on AVL-trees.
@@ -47,19 +47,17 @@
  * @author Rodion "rodde" Efremov
  * @version 1.6 (Feb 11, 2016)
  */
-public class OrderStatisticsTree<E extends Comparable<? super E>> implements OrderStatisticsSet<E> {
-
-  // The following constants were calculated using reflection. You can find the tests for these
-  // values in OrderStatisticsTreeTest, which shows the way these numbers were calculated. If our
-  // internal implementation changes, these values may be incorrect. An increase in overhead should
-  // be carefully considered.
-  public static final int ORDER_STATISTICS_TREE_BASE_SIZE = 32;
-  public static final int PER_ENTRY_OVERHEAD = 40;
+public class OrderStatisticsTree<E extends Comparable<? super E>>
+    implements OrderStatisticsSet<E> {
+
+  private static final int ORDER_STATISTICS_TREE_OVERHEAD =
+      memoryOverhead(OrderStatisticsTree.class);
+  @VisibleForTesting
+  static final int NODE_OVERHEAD = memoryOverhead(Node.class);
+
   private Node<E> root;
   private int size;
   private int modCount;
-  private int sizeInBytes = ORDER_STATISTICS_TREE_BASE_SIZE;
-  private static final SizeableObjectSizer elementSizer = new SizeableObjectSizer();
 
   @Override
   public Iterator<E> iterator() {
@@ -162,7 +160,6 @@ public boolean add(E element) {
       root = new Node<>(element);
       size = 1;
       modCount++;
-      incrementSize(element);
       return true;
     }
 
@@ -198,7 +195,6 @@ public boolean add(E element) {
     newNode.parent = parent;
     size++;
     modCount++;
-    incrementSize(element);
     Node<E> hi = parent;
     Node<E> lo = newNode;
 
@@ -269,7 +265,6 @@ public boolean remove(Object o) {
     }
 
     x = deleteNode(x);
-    decrementSize(x.key);
     fixAfterModification(x, false);
     size--;
     modCount++;
@@ -743,17 +738,14 @@ private int count(Node<E> node) {
     return leftTreeSize + 1 + rightTreeSize;
   }
 
-  void incrementSize(E element) {
-    sizeInBytes += elementSizer.sizeof(element) + PER_ENTRY_OVERHEAD;
-  }
-
-  void decrementSize(E element) {
-    sizeInBytes -= elementSizer.sizeof(element) + PER_ENTRY_OVERHEAD;
-  }
-
+  /**
+   * Returns the amount of memory used by this instance and its Node instances.
+   * Note that this does not include the memory used by the elements referenced
+   * by the nodes.
+   */
   @Override
   public int getSizeInBytes() {
-    return sizeInBytes;
+    return ORDER_STATISTICS_TREE_OVERHEAD + (size * NODE_OVERHEAD);
   }
 
   private static final class Node<T> {
@@ -825,7 +817,6 @@ public void remove() {
       checkConcurrentModification();
 
       Node<E> x = deleteNode(previousNode);
-      decrementSize(x.key);
       fixAfterModification(x, false);
 
       if (x == nextNode) {

diff --git a/...he/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursor.java b/...he/geode/redis/internal/collections/SizeableObject2ObjectOpenCustomHashMapWithCursor.java
@@ -15,31 +15,29 @@
 package org.apache.geode.redis.internal.collections;
 
 import static it.unimi.dsi.fastutil.HashCommon.mix;
+import static org.apache.geode.internal.JvmSizeUtils.memoryOverhead;
 
 import java.util.Map;
 
 import it.unimi.dsi.fastutil.objects.Object2ObjectMap;
 import it.unimi.dsi.fastutil.objects.Object2ObjectOpenCustomHashMap;
 
-import org.apache.geode.annotations.VisibleForTesting;
 import org.apache.geode.internal.size.Sizeable;
-import org.apache.geode.redis.internal.data.SizeableObjectSizer;
 
 /**
- * An extention of {@link Object2ObjectOpenCustomHashMap} that supports
+ * An extension of {@link Object2ObjectOpenCustomHashMap} that supports
  * a method of iteration where each scan operation returns an integer cursor
  * that allows future scan operations to start from that same point.
  *
  * The scan method provides the same guarantees as Redis's HSCAN, and in fact
  * uses the same algorithm.
  */
-public class SizeableObject2ObjectOpenCustomHashMapWithCursor<K, V>
+public abstract class SizeableObject2ObjectOpenCustomHashMapWithCursor<K, V>
     extends Object2ObjectOpenCustomHashMap<K, V> implements Sizeable {
 
   private static final long serialVersionUID = 9079713776660851891L;
-  public static final int BACKING_ARRAY_OVERHEAD_CONSTANT = 128;
-  public static final int BACKING_ARRAY_LENGTH_COEFFICIENT = 4;
-  private static final SizeableObjectSizer elementSizer = new SizeableObjectSizer();
+  public static final int OPEN_HASH_MAP_OVERHEAD =
+      memoryOverhead(SizeableObject2ObjectOpenCustomHashMapWithCursor.class);
 
   private int arrayContentsOverhead;
 
@@ -196,46 +194,39 @@ public V put(K k, V v) {
     V oldValue = super.put(k, v);
     if (oldValue == null) {
       // A create
-      arrayContentsOverhead += elementSizer.sizeof(k) + elementSizer.sizeof(v);
+      arrayContentsOverhead += sizeKey(k) + sizeValue(v);
     } else {
       // An update
-      arrayContentsOverhead += elementSizer.sizeof(v) - elementSizer.sizeof(oldValue);
+      arrayContentsOverhead += sizeValue(v) - sizeValue(oldValue);
     }
     return oldValue;
   }
 
   @Override
+  @SuppressWarnings("unchecked")
   public V remove(Object k) {
     V oldValue = super.remove(k);
     if (oldValue != null) {
-      arrayContentsOverhead -= elementSizer.sizeof(k) + elementSizer.sizeof(oldValue);
+      arrayContentsOverhead -= sizeKey((K) k) + sizeValue(oldValue);
     }
     return oldValue;
   }
 
   @Override
   public int getSizeInBytes() {
-    return arrayContentsOverhead + calculateBackingArraysOverhead();
-  }
-
-  @VisibleForTesting
-  int calculateBackingArraysOverhead() {
-    // This formula determined experimentally using tests.
-    return BACKING_ARRAY_OVERHEAD_CONSTANT
-        + BACKING_ARRAY_LENGTH_COEFFICIENT * getTotalBackingArrayLength();
-  }
-
-  @VisibleForTesting
-  int getArrayContentsOverhead() {
-    return arrayContentsOverhead;
-  }
+    // The size of the object referenced by the "strategy" field is not included
+    // here because in most cases it is a static singleton.
 
-  @VisibleForTesting
-  int getTotalBackingArrayLength() {
-    return key.length + value.length;
+    return OPEN_HASH_MAP_OVERHEAD + memoryOverhead(key) + memoryOverhead(value)
+        + arrayContentsOverhead;
   }
 
   public interface EntryConsumer<K, V, D> {
     void consume(D privateData, K key, V value);
   }
+
+  protected abstract int sizeKey(K key);
+
+  protected abstract int sizeValue(V value);
+
 }
diff --git a/...ain/java/org/apache/geode/redis/internal/collections/SizeableObjectOpenCustomHashSet.java b/...ain/java/org/apache/geode/redis/internal/collections/SizeableObjectOpenCustomHashSet.java
@@ -15,22 +15,21 @@
 package org.apache.geode.redis.internal.collections;
 
 
+import static org.apache.geode.internal.JvmSizeUtils.memoryOverhead;
+
 import java.util.Collection;
 import java.util.Iterator;
 
 import it.unimi.dsi.fastutil.objects.ObjectCollection;
 import it.unimi.dsi.fastutil.objects.ObjectOpenCustomHashSet;
 
-import org.apache.geode.annotations.VisibleForTesting;
 import org.apache.geode.internal.size.Sizeable;
-import org.apache.geode.redis.internal.data.SizeableObjectSizer;
 
-public class SizeableObjectOpenCustomHashSet<K> extends ObjectOpenCustomHashSet<K>
+public abstract class SizeableObjectOpenCustomHashSet<K> extends ObjectOpenCustomHashSet<K>
     implements Sizeable {
   private static final long serialVersionUID = 9174920505089089517L;
-  public static final int BACKING_ARRAY_OVERHEAD_CONSTANT = 92;
-  public static final int BACKING_ARRAY_LENGTH_COEFFICIENT = 4;
-  private static final SizeableObjectSizer elementSizer = new SizeableObjectSizer();
+  private static final int OPEN_HASH_SET_OVERHEAD =
+      memoryOverhead(SizeableObjectOpenCustomHashSet.class);
 
   private int memberOverhead;
 
@@ -96,38 +95,27 @@ public SizeableObjectOpenCustomHashSet(K[] a, Strategy<? super K> strategy) {
   public boolean add(K k) {
     boolean added = super.add(k);
     if (added) {
-      memberOverhead += elementSizer.sizeof(k);
+      memberOverhead += sizeElement(k);
     }
     return added;
   }
 
+  @SuppressWarnings("unchecked")
   @Override
   public boolean remove(Object k) {
     boolean removed = super.remove(k);
     if (removed) {
-      memberOverhead -= elementSizer.sizeof(k);
+      memberOverhead -= sizeElement((K) k);
     }
     return removed;
   }
 
   @Override
   public int getSizeInBytes() {
-    return memberOverhead + calculateBackingArrayOverhead();
-  }
-
-  @VisibleForTesting
-  int getMemberOverhead() {
-    return memberOverhead;
+    // The object referenced by the "strategy" field is not sized
+    // since it is usually a singleton instance.
+    return OPEN_HASH_SET_OVERHEAD + memoryOverhead(key) + memberOverhead;
   }
 
-  @VisibleForTesting
-  int getBackingArrayLength() {
-    return key.length;
-  }
-
-  @VisibleForTesting
-  int calculateBackingArrayOverhead() {
-    // This formula determined experimentally using tests
-    return BACKING_ARRAY_OVERHEAD_CONSTANT + (BACKING_ARRAY_LENGTH_COEFFICIENT * key.length);
-  }
+  protected abstract int sizeElement(K element);
 }