updates per comments (still outstanding on the todos)

Author: byrnedj

Diff for: cachelib/allocator/Cache.h

@@ -90,6 +90,7 @@ class CacheBase {
   virtual const std::string getCacheName() const = 0;
 
   // Get the reference  to a memory pool, for stats purposes
+  // uses the top most memory tier by default
   //
   // @param poolId    The pool id to query
   virtual const MemoryPool& getPool(PoolId poolId) const = 0;

Diff for: cachelib/allocator/CacheAllocator-inl.h

@@ -372,8 +372,7 @@ CacheAllocator<CacheTrait>::allocateInternalTier(TierId tid,
                                              typename Item::Key key,
                                              uint32_t size,
                                              uint32_t creationTime,
-                                             uint32_t expiryTime,
-                                             bool fromEvictorThread) {
+                                             uint32_t expiryTime) {
   util::LatencyTracker tracker{stats().allocateLatency_};
 
   SCOPE_FAIL { stats_.invalidAllocs.inc(); };
@@ -403,7 +402,7 @@ CacheAllocator<CacheTrait>::allocateInternalTier(TierId tid,
   }
 
   // TODO: Today disableEviction means do not evict from memory (DRAM).
-  //       Should we support eviction between memory tiers (e.g. from DRAM to PMEM)?
+  //       Should we support eviction between memory tiers (e.g. from DRAM to next tier)?
   if (memory == nullptr && !config_.disableEviction) {
     memory = findEviction(tid, pid, cid);
   }
@@ -475,7 +474,7 @@ CacheAllocator<CacheTrait>::getTargetTierForItem(PoolId pid,
   if (freePercentage <= config_.minAcAllocationWatermark)
     return defaultTargetTier + 1;
 
-  // TODO: we can even think about creating different allocation classes for PMEM
+  // TODO: we can even think about creating different allocation classes for different tiers
   // and we could look at possible fragmentation when deciding where to put the item
   if (config_.sizeThresholdPolicy)
     return requiredSize < config_.sizeThresholdPolicy ? defaultTargetTier : defaultTargetTier + 1;
@@ -506,10 +505,9 @@ CacheAllocator<CacheTrait>::allocateInternal(PoolId pid,
                                              typename Item::Key key,
                                              uint32_t size,
                                              uint32_t creationTime,
-                                             uint32_t expiryTime,
-                                             bool fromEvictorThread) {
+                                             uint32_t expiryTime) {
   auto tid = getTargetTierForItem(pid, key, size, creationTime, expiryTime);
-  return allocateInternalTier(tid, pid, key, size, creationTime, expiryTime, fromEvictorThread);
+  return allocateInternalTier(tid, pid, key, size, creationTime, expiryTime);
 }
 
 template <typename CacheTrait>
@@ -1680,17 +1678,13 @@ template <typename CacheTrait>
 bool CacheAllocator<CacheTrait>::shouldEvictToNextMemoryTier(
     TierId sourceTierId, TierId targetTierId, PoolId pid, Item& item)
 {
-  if (config_.disableEvictionToMemory)
-    return false;
-
-  // TODO: implement more advanced admission policies for memory tiers
-  return true;
+  return !(config_.disableEvictionToMemory);
 }
 
 template <typename CacheTrait>
 typename CacheAllocator<CacheTrait>::WriteHandle
 CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier(
-    TierId tid, PoolId pid, Item& item, bool fromEvictorThread) {
+    TierId tid, PoolId pid, Item& item) {
   if(item.isExpired()) return acquire(&item);
 
   TierId nextTier = tid;
@@ -1703,8 +1697,7 @@ CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier(
                      item.getKey(),
                      item.getSize(),
                      item.getCreationTime(),
-                     item.getExpiryTime(),
-                     fromEvictorThread);
+                     item.getExpiryTime());
 
     if (newItemHdl) {
       XDCHECK_EQ(newItemHdl->getSize(), item.getSize());
@@ -1717,10 +1710,10 @@ CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier(
 
 template <typename CacheTrait>
 typename CacheAllocator<CacheTrait>::WriteHandle
-CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier(Item& item, bool fromEvictorThread) {
+CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier(Item& item) {
     auto tid = getTierId(item);
     auto pid = allocator_[tid]->getAllocInfo(item.getMemory()).poolId;
-    return tryEvictToNextMemoryTier(tid, pid, item, fromEvictorThread);
+    return tryEvictToNextMemoryTier(tid, pid, item);
 }
 
 template <typename CacheTrait>
@@ -3047,14 +3040,14 @@ void CacheAllocator<CacheTrait>::evictForSlabRelease(
 template <typename CacheTrait>
 typename CacheAllocator<CacheTrait>::ItemHandle
 CacheAllocator<CacheTrait>::evictNormalItem(Item& item,
-                                            bool skipIfTokenInvalid, bool fromEvictorThread) {
+                                            bool skipIfTokenInvalid) {
   XDCHECK(item.isMoving());
 
   if (item.isOnlyMoving()) {
     return ItemHandle{};
   }
 
-  auto evictHandle = tryEvictToNextMemoryTier(item, fromEvictorThread);
+  auto evictHandle = tryEvictToNextMemoryTier(item);
   if(evictHandle) return evictHandle;
 
   auto predicate = [](const Item& it) { return it.getRefCount() == 0; };

Diff for: cachelib/allocator/CacheAllocator.h

@@ -36,6 +36,7 @@
 #include <folly/Format.h>
 #include <folly/Range.h>
 #pragma GCC diagnostic pop
+
 #include "cachelib/allocator/CCacheManager.h"
 #include "cachelib/allocator/Cache.h"
 #include "cachelib/allocator/CacheAllocatorConfig.h"
@@ -1334,8 +1335,7 @@ class CacheAllocator : public CacheBase {
                               Key key,
                               uint32_t size,
                               uint32_t creationTime,
-                              uint32_t expiryTime,
-                              bool fromEvictorThread);
+                              uint32_t expiryTime);
 
   // create a new cache allocation on specific memory tier.
   // For description see allocateInternal.
@@ -1346,8 +1346,7 @@ class CacheAllocator : public CacheBase {
                               Key key,
                               uint32_t size,
                               uint32_t creationTime,
-                              uint32_t expiryTime,
-                              bool fromEvictorThread);
+                              uint32_t expiryTime);
 
   // Allocate a chained item
   //
@@ -1588,15 +1587,15 @@ class CacheAllocator : public CacheBase {
   //
   // @return valid handle to the item. This will be the last
   //         handle to the item. On failure an empty handle.
-  WriteHandle tryEvictToNextMemoryTier(TierId tid, PoolId pid, Item& item, bool fromEvictorThread);
+  WriteHandle tryEvictToNextMemoryTier(TierId tid, PoolId pid, Item& item);
 
   // Try to move the item down to the next memory tier
   //
   // @param item the item to evict
   //
   // @return valid handle to the item. This will be the last
   //         handle to the item. On failure an empty handle. 
-  WriteHandle tryEvictToNextMemoryTier(Item& item, bool fromEvictorThread);
+  WriteHandle tryEvictToNextMemoryTier(Item& item);
 
   bool shouldEvictToNextMemoryTier(TierId sourceTierId,
         TierId targetTierId, PoolId pid, Item& item);
@@ -1728,7 +1727,7 @@ class CacheAllocator : public CacheBase {
   //
   // @return last handle for corresponding to item on success. empty handle on
   // failure. caller can retry if needed.
-  ItemHandle evictNormalItem(Item& item, bool skipIfTokenInvalid = false, bool fromEvictorThread = false);
+  ItemHandle evictNormalItem(Item& item, bool skipIfTokenInvalid = false);
 
   // Helper function to evict a child item for slab release
   // As a side effect, the parent item is also evicted

Diff for: cachelib/allocator/CacheAllocatorConfig.h

@@ -587,12 +587,12 @@ class CacheAllocatorConfig {
 
   bool disableEvictionToMemory{false};
 
-  double minAcAllocationWatermark{0.0};
-  double maxAcAllocationWatermark{0.0};
-  double acTopTierEvictionWatermark{0.0}; // TODO: make it per TIER?
-  uint64_t sizeThresholdPolicy{0};   
-  double defaultTierChancePercentage{50.0};
-  uint64_t forceAllocationTier{UINT64_MAX};
+  double minAcAllocationWatermark{0.0}; //if % free in AC is <= then try to allocate in next tier 
+  double maxAcAllocationWatermark{0.0}; //if % free in AC is >= then allocate in default target tier
+  double acTopTierEvictionWatermark{0.0}; // evict from 1st tier if % free is < watermark
+  uint64_t sizeThresholdPolicy{0}; // only allow items < threshold in top tier   
+  double defaultTierChancePercentage{50.0}; //randomly allocate items among top and bottom tiers
+  uint64_t forceAllocationTier{UINT64_MAX}; //force allocations to happen in this tier
 
   friend CacheT;