Disabled memory tier size setting

victoria-mcgrath · victoria-mcgrath · commit 441c35ac4622 · 2022-06-20T18:10:20.000-04:00
diff --git a/cachelib/allocator/CacheAllocatorConfig.h b/cachelib/allocator/CacheAllocatorConfig.h
@@ -378,9 +378,8 @@ class CacheAllocatorConfig {
   bool validateStrategy(
       const std::shared_ptr<PoolOptimizeStrategy>& strategy) const;
 
-  // check whether sizes are set for each memoriy tier and the
-  // sum of sizes matches total cache size
-  void validateMemoryTierSizes() const;
+  // check that memory tier ratios are set properly
+  bool validateMemoryTiers() const;
 
   // @return a map representation of the configs
   std::map<std::string, std::string> serialize() const;
@@ -391,6 +390,10 @@ class CacheAllocatorConfig {
   // Amount of memory for this cache instance (sum of all memory tiers' sizes)
   size_t size = 1 * 1024 * 1024 * 1024;
 
+  // The max number of memory cache tiers
+  // TODO: increase this number when multi-tier configs are enabled
+  const size_t maxCacheMemoryTiers = 1;
+
   // Directory for shared memory related metadata
   std::string cacheDir;
 
@@ -595,11 +598,9 @@ class CacheAllocatorConfig {
   friend CacheT;
 
  private:
-  CacheAllocatorConfig& setCacheSizeImpl(size_t _size);
-
   // Configuration for memory tiers.
   MemoryTierConfigs memoryTierConfigs{
-      {MemoryTierCacheConfig::fromShm().setRatio(1).setSize(size)}};
+      {MemoryTierCacheConfig::fromShm().setRatio(1)}};
 
   void mergeWithPrefix(
       std::map<std::string, std::string>& configMap,
@@ -623,8 +624,7 @@ CacheAllocatorConfig<T>& CacheAllocatorConfig<T>::setCacheName(
 }
 
 template <typename T>
-CacheAllocatorConfig<T>& CacheAllocatorConfig<T>::setCacheSizeImpl(
-    size_t _size) {
+CacheAllocatorConfig<T>& CacheAllocatorConfig<T>::setCacheSize(size_t _size) {
   size = _size;
   constexpr size_t maxCacheSizeWithCoredump = 64'424'509'440; // 60GB
   if (size <= maxCacheSizeWithCoredump) {
@@ -633,18 +633,6 @@ CacheAllocatorConfig<T>& CacheAllocatorConfig<T>::setCacheSizeImpl(
   return *this;
 }
 
-template <typename T>
-CacheAllocatorConfig<T>& CacheAllocatorConfig<T>::setCacheSize(size_t _size) {
-  if (memoryTierConfigs.size() == 1) {
-    memoryTierConfigs[0].setSize(_size);
-  } else {
-    throw std::invalid_argument(
-        "Cannot set cache size after configuring memory tiers.");
-  }
-
-  return setCacheSizeImpl(_size);
-}
-
 template <typename T>
 CacheAllocatorConfig<T>& CacheAllocatorConfig<T>::setDefaultAllocSizes(
     std::set<uint32_t> allocSizes) {
@@ -886,72 +874,16 @@ CacheAllocatorConfig<T>& CacheAllocatorConfig<T>::enableItemReaperInBackground(
 template <typename T>
 CacheAllocatorConfig<T>& CacheAllocatorConfig<T>::configureMemoryTiers(
     const MemoryTierConfigs& config) {
-  if (config.size() != 1) {
-    throw std::invalid_argument(
-        "Only single memory tier is currently supported.");
-  }
-  memoryTierConfigs = config;
-  size_t sumRatios = 0;
-  size_t sumSizes = 0;
-
-  for (const auto& tierConfig : memoryTierConfigs) {
-    auto tierSize = tierConfig.getSize();
-    auto tierRatio = tierConfig.getRatio();
-    if ((sumRatios && tierSize) || (sumSizes && tierRatio)) {
-      throw std::invalid_argument(
-          "For each memory tier either size or ratio must be set.");
-    }
-    sumRatios += tierRatio;
-    sumSizes += tierSize;
-  }
-
-  if (!getCacheSize()) {
-    if (sumRatios) {
-      throw std::invalid_argument(
-          "Total cache size must be specified when size ratios are "
-          "used to specify memory tier sizes.");
-    } else if (sumSizes) {
-      setCacheSizeImpl(sumSizes);
-    }
+  if (config.size() > maxCacheMemoryTiers) {
+    throw std::invalid_argument(folly::sformat(
+        "Too many memory tiers. The number of supported tiers is {}.",
+        maxCacheMemoryTiers));
   }
-
-  if (sumRatios) {
-    if (!getCacheSize()) {
-      throw std::invalid_argument(
-          "Total cache size must be specified when size ratios are "
-          "used to specify memory tier sizes.");
-    } else {
-      if (getCacheSize() < sumRatios) {
-        throw std::invalid_argument(
-            "Sum of all tier size ratios is greater than total cache size.");
-      }
-      // Convert ratios to sizes
-      sumSizes = 0;
-      size_t partitionSize = getCacheSize() / sumRatios;
-      for (auto& tierConfig : memoryTierConfigs) {
-        tierConfig.setSize(partitionSize * tierConfig.getRatio());
-        sumSizes += tierConfig.getSize();
-      }
-      if (getCacheSize() != sumSizes) {
-        // Adjust capacity of the last tier to account for rounding error
-        memoryTierConfigs.back().setSize(memoryTierConfigs.back().getSize() +
-                                         (getCacheSize() - sumSizes));
-        sumSizes = getCacheSize();
-      }
-    }
-  } else if (sumSizes) {
-    if (sumSizes != getCacheSize()) {
-      throw std::invalid_argument(
-          "Sum of tier sizes doesn't match total cache size. "
-          "Setting of cache total size is not required when per-tier "
-          "sizes are specified - it is calculated as sum of tier sizes.");
-    }
-  } else {
+  if (!config.size()) {
     throw std::invalid_argument(
-        "Either sum of all memory tiers sizes or sum of all ratios "
-        "must be greater than 0.");
+        "There must be at least one memory tier config.");
   }
-
+  memoryTierConfigs = config;
   return *this;
 }
 
@@ -1119,7 +1051,7 @@ const CacheAllocatorConfig<T>& CacheAllocatorConfig<T>::validate() const {
         "It's not allowed to enable both RemoveCB and ItemDestructor.");
   }
 
-  validateMemoryTierSizes();
+  validateMemoryTiers();
 
   return *this;
 }
@@ -1149,21 +1081,20 @@ bool CacheAllocatorConfig<T>::validateStrategy(
 }
 
 template <typename T>
-void CacheAllocatorConfig<T>::validateMemoryTierSizes() const {
-  size_t sumSizes = 0;
-
+bool CacheAllocatorConfig<T>::validateMemoryTiers() const {
+  size_t parts = 0;
   for (const auto& tierConfig : memoryTierConfigs) {
-    if (!tierConfig.getSize()) {
-      throw std::invalid_argument("Each cache tier must have size set.");
-    } else {
-      sumSizes += tierConfig.getSize();
+    if (!tierConfig.getRatio()) {
+      throw std::invalid_argument("Tier ratio must be an integer number >=1.");
     }
+    parts += tierConfig.getRatio();
   }
 
-  if (sumSizes != size) {
+  if (parts > size) {
     throw std::invalid_argument(
-        "Sum of sizes of all cache tiers must equal to total cache size.");
+        "Sum of tier ratios must be less than total cache size.");
   }
+  return true;
 }
 
 template <typename T>
diff --git a/cachelib/allocator/MemoryTierCacheConfig.h b/cachelib/allocator/MemoryTierCacheConfig.h
@@ -30,38 +30,43 @@ class MemoryTierCacheConfig {
     return MemoryTierCacheConfig();
   }
 
-  // Specifies size of this memory tier. Sizes of tiers  must be specified by
-  // either setting size explicitly or using ratio, mixing of the two is not
-  // supported.
-  MemoryTierCacheConfig& setSize(size_t _size) {
-    size = _size;
-    return *this;
-  }
-
   // Specifies ratio of this memory tier to other tiers. Absolute size
   // of each tier can be calculated as:
   // cacheSize * tierRatio / Sum of ratios for all tiers; the difference
   // between total cache size and sum of all tier sizes resulted from
   // round off error is accounted for when calculating the last tier's
   // size to make the totals equal.
   MemoryTierCacheConfig& setRatio(size_t _ratio) {
+    if (!_ratio) {
+      throw std::invalid_argument("Tier ratio must be an integer number >=1.");
+    }
     ratio = _ratio;
     return *this;
   }
 
   size_t getRatio() const noexcept { return ratio; }
 
-  size_t getSize() const noexcept { return size; }
+  size_t calculateTierSize(size_t totalCacheSize, size_t partitionNum) {
+    size_t partitionSize = 0;
+    if (partitionNum > totalCacheSize) {
+      throw std::invalid_argument(
+          "Ratio must be less or equal to total cache size.");
+    }
 
-  // Size of this memory tiers
-  size_t size{0};
+    if (!partitionNum) {
+      throw std::invalid_argument(
+          "The total number of tier ratios must be an integer number >=1.");
+    }
+
+    return getRatio() * (totalCacheSize / partitionNum);
+  }
 
   // Ratio is a number of parts of the total cache size to be allocated for this
   // tier. E.g. if X is a total cache size, Yi are ratios specified for memory
   // tiers, and Y is the sum of all Yi, then size of the i-th tier
   // Xi = (X / Y) * Yi. For examle, to configure 2-tier cache where each
   // tier is a half of the total cache size, set both tiers' ratios to 1.
-  size_t ratio{0};
+  size_t ratio{1};
 
  private:
   // TODO: introduce a container for tier settings when adding support for
diff --git a/cachelib/allocator/tests/CacheAllocatorConfigTest.cpp b/cachelib/allocator/tests/CacheAllocatorConfigTest.cpp
@@ -26,9 +26,7 @@ namespace tests {
 using AllocatorT = LruAllocator;
 using MemoryTierConfigs = CacheAllocatorConfig<AllocatorT>::MemoryTierConfigs;
 
-// TODO: Increase this constant when multi-tier configs are fully enabled
-size_t maxNumTiers = 1;
-size_t defaultTotalSize = 2 * Slab::kSize * maxNumTiers;
+size_t defaultTotalSize = 1 * 1024LL * 1024LL * 1024LL;
 
 class CacheAllocatorConfigTest : public testing::Test {};
 
@@ -37,14 +35,6 @@ MemoryTierConfigs generateTierConfigs(size_t numTiers,
   return MemoryTierConfigs(numTiers, config);
 }
 
-size_t sumTierSizes(const MemoryTierConfigs& configs) {
-  size_t sumSizes = 0;
-  for (const auto& config : configs) {
-    sumSizes += config.getSize();
-  }
-  return sumSizes;
-}
-
 TEST_F(CacheAllocatorConfigTest, MultipleTier0Config) {
   AllocatorT::Config config;
   // Throws if vector of tier configs is emptry
@@ -53,84 +43,27 @@ TEST_F(CacheAllocatorConfigTest, MultipleTier0Config) {
 }
 
 TEST_F(CacheAllocatorConfigTest, MultipleTier1Config) {
-  AllocatorT::Config config1, config2;
+  AllocatorT::Config config;
   // Accepts single-tier configuration
-  config1.setCacheSize(defaultTotalSize)
-      .configureMemoryTiers(
-          {MemoryTierCacheConfig::fromShm().setSize(defaultTotalSize)});
-  config2.setCacheSize(0).configureMemoryTiers(
-      {MemoryTierCacheConfig::fromShm().setSize(defaultTotalSize)});
+  config.setCacheSize(defaultTotalSize)
+      .configureMemoryTiers({MemoryTierCacheConfig::fromShm().setRatio(1)});
 }
 
-TEST_F(CacheAllocatorConfigTest, MultipleTier2Config) {
+TEST_F(CacheAllocatorConfigTest, InvalidTierRatios) {
   AllocatorT::Config config;
-  // Throws if vector of tier configs > 1
   EXPECT_THROW(config.configureMemoryTiers(generateTierConfigs(
-                   maxNumTiers + 1,
-                   MemoryTierCacheConfig::fromShm().setSize(defaultTotalSize))),
+                   config.maxCacheMemoryTiers + 1,
+                   MemoryTierCacheConfig::fromShm().setRatio(0))),
                std::invalid_argument);
 }
 
-TEST_F(CacheAllocatorConfigTest, TotalCacheSizeSet0) {
-  AllocatorT::Config config;
-  // If total cache size is set to zero and each individual tier specifies its
-  // size then total cache size is set to the sum of tier sizes
-  MemoryTierConfigs tierConfigs = {
-      MemoryTierCacheConfig::fromShm().setSize(defaultTotalSize)};
-  config.setCacheSize(0).configureMemoryTiers(tierConfigs);
-  ASSERT_EQ(config.getCacheSize(), sumTierSizes(config.getMemoryTierConfigs()));
-}
-
 TEST_F(CacheAllocatorConfigTest, TotalCacheSizeLessThanRatios) {
   AllocatorT::Config config;
   // Throws if total cache size is set to 0
-  EXPECT_THROW(
-      config.setCacheSize(defaultTotalSize)
-          .configureMemoryTiers({MemoryTierCacheConfig::fromShm().setRatio(
-              defaultTotalSize + 1)}),
-      std::invalid_argument);
-}
-
-TEST_F(CacheAllocatorConfigTest, SumTierSizes) {
-  AllocatorT::Config config1, config2;
-  MemoryTierConfigs sizeConfigs = generateTierConfigs(
-      maxNumTiers,
-      MemoryTierCacheConfig::fromShm().setSize(defaultTotalSize / maxNumTiers));
-  MemoryTierConfigs ratioConfigs = generateTierConfigs(
-      maxNumTiers, MemoryTierCacheConfig::fromShm().setRatio(1));
-
-  config1.setCacheSize(defaultTotalSize).configureMemoryTiers(sizeConfigs);
-  ASSERT_EQ(config1.getCacheSize(),
-            sumTierSizes(config1.getMemoryTierConfigs()));
-
-  config2.setCacheSize(defaultTotalSize).configureMemoryTiers(ratioConfigs);
-  ASSERT_EQ(config2.getCacheSize(),
-            sumTierSizes(config2.getMemoryTierConfigs()));
-}
-
-TEST_F(CacheAllocatorConfigTest, TotalCacheSizeReset1Tier) {
-  AllocatorT::Config config1, config2;
-  size_t numTiers = 1;
-  MemoryTierConfigs sizeConfigs = generateTierConfigs(
-      numTiers,
-      MemoryTierCacheConfig::fromShm().setSize(defaultTotalSize / numTiers));
-  MemoryTierConfigs ratioConfigs = generateTierConfigs(
-      numTiers, MemoryTierCacheConfig::fromShm().setRatio(1));
-
-  // Resetting total cache size when there's only one tier
-  // is considered harmless. TODO: when multiple tiers are
-  // enabled this test needs to be expanded to check that
-  // validate() throws if total cache size is reset to
-  // invalid number after configureMemoryTiers is called.
-  config1.setCacheSize(defaultTotalSize)
-      .configureMemoryTiers(sizeConfigs)
-      .setCacheSize(defaultTotalSize - 1);
-  config1.validate();
-
-  config2.setCacheSize(defaultTotalSize)
-      .configureMemoryTiers(ratioConfigs)
-      .setCacheSize(defaultTotalSize - 1);
-  config2.validate();
+  config.setCacheSize(defaultTotalSize)
+      .configureMemoryTiers(
+          {MemoryTierCacheConfig::fromShm().setRatio(defaultTotalSize + 1)});
+  EXPECT_THROW(config.validate(), std::invalid_argument);
 }
 
 } // namespace tests
