Fix CalcHashBits (#10295)

Summary:
We fix two bugs in CalcHashBits. The first one is an off-by-one error: the desired number of table slots is the real number ``capacity / (kLoadFactor * handle_charge)``, which should not be rounded down. The second one is that we should disallow inputs that set the element charge to 0, namely ``estimated_value_size == 0 && metadata_charge_policy == kDontChargeCacheMetadata``.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/10295

Test Plan: CalcHashBits is tested by CalcHashBitsTest (in lru_cache_test.cc). The test now iterates over many more inputs; it covers, in particular, the rounding error edge case. Overall, the test is now more robust. Run ``make -j24 check``.

Reviewed By: pdillinger

Differential Revision: D37573797

Pulled By: guidotag

fbshipit-source-id: ea4f4439f7196ab1c1afb88f566fe92850537262

cache/clock_cache.cc

@@ -289,14 +289,11 @@ int ClockCacheShard::CalcHashBits(
     CacheMetadataChargePolicy metadata_charge_policy) {
   size_t handle_charge =
       CalcEstimatedHandleCharge(estimated_value_size, metadata_charge_policy);
+  assert(handle_charge > 0);
   uint32_t num_entries =
-      static_cast<uint32_t>(capacity / (kLoadFactor * handle_charge));
-
-  if (num_entries == 0) {
-    return 0;
-  }
-  int hash_bits = FloorLog2(num_entries);
-  return hash_bits + (size_t{1} << hash_bits < num_entries ? 1 : 0);
+      static_cast<uint32_t>(capacity / (kLoadFactor * handle_charge)) + 1;
+  assert(num_entries <= uint32_t{1} << 31);
+  return FloorLog2((num_entries << 1) - 1);
 }
 
 void ClockCacheShard::SetCapacity(size_t capacity) {
@@ -516,6 +513,8 @@ ClockCache::ClockCache(size_t capacity, size_t estimated_value_size,
                        int num_shard_bits, bool strict_capacity_limit,
                        CacheMetadataChargePolicy metadata_charge_policy)
     : ShardedCache(capacity, num_shard_bits, strict_capacity_limit) {
+  assert(estimated_value_size > 0 ||
+         metadata_charge_policy != kDontChargeCacheMetadata);
   num_shards_ = 1 << num_shard_bits;
   shards_ = reinterpret_cast<ClockCacheShard*>(
       port::cacheline_aligned_alloc(sizeof(ClockCacheShard) * num_shards_));

cache/fast_lru_cache.cc

@@ -290,14 +290,11 @@ int LRUCacheShard::CalcHashBits(
     CacheMetadataChargePolicy metadata_charge_policy) {
   size_t handle_charge =
       CalcEstimatedHandleCharge(estimated_value_size, metadata_charge_policy);
+  assert(handle_charge > 0);
   uint32_t num_entries =
-      static_cast<uint32_t>(capacity / (kLoadFactor * handle_charge));
-
-  if (num_entries == 0) {
-    return 0;
-  }
-  int hash_bits = FloorLog2(num_entries);
-  return hash_bits + (size_t{1} << hash_bits < num_entries ? 1 : 0);
+      static_cast<uint32_t>(capacity / (kLoadFactor * handle_charge)) + 1;
+  assert(num_entries <= uint32_t{1} << 31);
+  return FloorLog2((num_entries << 1) - 1);
 }
 
 void LRUCacheShard::SetCapacity(size_t capacity) {
@@ -530,6 +527,8 @@ LRUCache::LRUCache(size_t capacity, size_t estimated_value_size,
                    int num_shard_bits, bool strict_capacity_limit,
                    CacheMetadataChargePolicy metadata_charge_policy)
     : ShardedCache(capacity, num_shard_bits, strict_capacity_limit) {
+  assert(estimated_value_size > 0 ||
+         metadata_charge_policy != kDontChargeCacheMetadata);
   num_shards_ = 1 << num_shard_bits;
   shards_ = reinterpret_cast<LRUCacheShard*>(
       port::cacheline_aligned_alloc(sizeof(LRUCacheShard) * num_shards_));

cache/lru_cache_test.cc

@@ -287,25 +287,37 @@ TEST_F(FastLRUCacheTest, ValidateKeySize) {
 }
 
 TEST_F(FastLRUCacheTest, CalcHashBitsTest) {
-  size_t capacity = 1024;
-  size_t estimated_value_size = 1;
-  CacheMetadataChargePolicy metadata_charge_policy = kDontChargeCacheMetadata;
-  double max_occupancy =
-      CalcMaxOccupancy(capacity, estimated_value_size, metadata_charge_policy);
-  int hash_bits = CalcHashBitsWrapper(capacity, estimated_value_size,
+  size_t capacity;
+  size_t estimated_value_size;
+  double max_occupancy;
+  int hash_bits;
+  CacheMetadataChargePolicy metadata_charge_policy;
+
+  // Vary the cache capacity, fix the element charge.
+  for (int i = 0; i < 2048; i++) {
+    capacity = i;
+    estimated_value_size = 0;
+    metadata_charge_policy = kFullChargeCacheMetadata;
+    max_occupancy = CalcMaxOccupancy(capacity, estimated_value_size,
+                                     metadata_charge_policy);
+    hash_bits = CalcHashBitsWrapper(capacity, estimated_value_size,
                                     metadata_charge_policy);
     EXPECT_TRUE(TableSizeIsAppropriate(hash_bits, max_occupancy));
+  }
 
+  // Fix the cache capacity, vary the element charge.
+  for (int i = 0; i < 1024; i++) {
     capacity = 1024;
-  estimated_value_size = 1;
+    estimated_value_size = i;
     metadata_charge_policy = kFullChargeCacheMetadata;
-  max_occupancy =
-      CalcMaxOccupancy(capacity, estimated_value_size, metadata_charge_policy);
+    max_occupancy = CalcMaxOccupancy(capacity, estimated_value_size,
+                                     metadata_charge_policy);
     hash_bits = CalcHashBitsWrapper(capacity, estimated_value_size,
                                     metadata_charge_policy);
     EXPECT_TRUE(TableSizeIsAppropriate(hash_bits, max_occupancy));
+  }
 
-  // No elements fit in cache.
+  // Zero-capacity cache, and only values have charge.
   capacity = 0;
   estimated_value_size = 1;
   metadata_charge_policy = kDontChargeCacheMetadata;
@@ -313,25 +325,25 @@ TEST_F(FastLRUCacheTest, CalcHashBitsTest) {
                                   metadata_charge_policy);
   EXPECT_TRUE(TableSizeIsAppropriate(hash_bits, 0 /* max_occupancy */));
 
-  // Set the capacity just below a single handle. Because the load factor is <
-  // 100% at least one handle will fit in the table.
-  estimated_value_size = 1;
-  size_t handle_charge = CalcEstimatedHandleChargeWrapper(
-      8192 /* estimated_value_size */, kDontChargeCacheMetadata);
-  capacity = handle_charge - 1;
-  // The load factor should be bounded away from 100%.
-  assert(static_cast<size_t>(capacity / fast_lru_cache::kLoadFactor) >
-         handle_charge);
-  metadata_charge_policy = kDontChargeCacheMetadata;
-  max_occupancy =
-      CalcMaxOccupancy(capacity, estimated_value_size, metadata_charge_policy);
+  // Zero-capacity cache, and only metadata has charge.
+  capacity = 0;
+  estimated_value_size = 0;
+  metadata_charge_policy = kFullChargeCacheMetadata;
   hash_bits = CalcHashBitsWrapper(capacity, estimated_value_size,
                                   metadata_charge_policy);
-  EXPECT_TRUE(TableSizeIsAppropriate(hash_bits, max_occupancy));
+  EXPECT_TRUE(TableSizeIsAppropriate(hash_bits, 0 /* max_occupancy */));
+
+  // Small cache, large elements.
+  capacity = 1024;
+  estimated_value_size = 8192;
+  metadata_charge_policy = kFullChargeCacheMetadata;
+  hash_bits = CalcHashBitsWrapper(capacity, estimated_value_size,
+                                  metadata_charge_policy);
+  EXPECT_TRUE(TableSizeIsAppropriate(hash_bits, 0 /* max_occupancy */));
 
   // Large capacity.
   capacity = 31924172;
-  estimated_value_size = 321;
+  estimated_value_size = 8192;
   metadata_charge_policy = kFullChargeCacheMetadata;
   max_occupancy =
       CalcMaxOccupancy(capacity, estimated_value_size, metadata_charge_policy);