CacheLib icon indicating copy to clipboard operation
CacheLib copied to clipboard

Admission policies + some extra stats for cachebench

Open byrnedj opened this issue 3 years ago • 2 comments

This is the first part of what was formerly the background data movement + tier admission PR


This change is Reviewable

byrnedj avatar Aug 25 '22 01:08 byrnedj

I rebased the PR, the outstanding issues from the review to address are:

cachelib/allocator/MemoryTierCacheConfig.h line 72 at r1 (raw file):

}

// TODO: move it to MMContainer config

Looks like we need to address this TODO comment. Not sure why it should be per tier.

cachelib/allocator/memory/MemoryAllocator.h line 652 at r1 (raw file):

}

// TODO: what is it?

cachelib/allocator/memory/MemoryAllocatorStats.h line 58 at r1 (raw file):

}

constexpr size_t getTotalMemory() const noexcept { should it be renamed to getTotalAllocMemory?

cachelib/allocator/memory/MemoryPool.h line 311 at r1 (raw file):

void setNumSlabsAdvised(uint64_t value) { curSlabsAdvised_ = value; }

// TODO: Again, we broken incapsulation. Not sure that we have to merge some hacks to develop branch.

byrnedj avatar Sep 20 '22 17:09 byrnedj

cachelib/allocator/CacheAllocator-inl.h line 2508 at r2 (raw file):

            : 0);
    for (TierId tid = 0; tid < getNumTiers(); tid++) {
      if constexpr (std::is_same_v<MMConfig, MMLru::Config> || std::is_same_v<MMConfig, MM2Q::Config>) {

This should probably be fixed (this if was just a temp workaround). The best way would probably be to move those 2 params to specific MMContainer config. Do we have support for setting MMCOntainer configs from JSON currently? And will it work for multi tier?

For this it is not trivial to add per tier mmContainer configs. Partly because in cachebench/Cache-inl.h, we have the following initialization:

    const size_t numBytes = cache_->getCacheMemoryStats().cacheSize;
    for (uint64_t i = 0; i < config_.numPools; ++i) {
      const double& ratio = config_.poolSizes[i];
      const size_t poolSize = static_cast<size_t>(numBytes * ratio);
      typename Allocator::MMConfig mmConfig =
          makeMMConfig<typename Allocator::MMConfig>(config_);
      const PoolId pid = cache_->addPool(
          folly::sformat("pool_{}", i), poolSize, {} /* allocSizes */, mmConfig,
          nullptr /* rebalanceStrategy */, nullptr /* resizeStrategy */,
          true /* ensureSufficientMem */);
      pools_.push_back(pid);
    }

The call to addPool initializes a pool that is split among tiers, so mmConfig is done per pool rather than per tier. For now I would suggest that we either:

  1. Accept the current implementation or
  2. Drop it from this PR and create a new PR that fully supports per-tier mmContainer creation (allowing us to have different allocators per tier too).

I have addressed the rest of the comments in the latest commit.

byrnedj avatar Sep 21 '22 21:09 byrnedj