First set of changes to cache configuration API to enable multi-tier caches #138

victoria-mcgrath · 2022-05-02T17:48:53Z

These changes introduce per-tier cache configuration required to implement features discussed here: #102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced.

…ement features discussed here: pmem#102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced.

facebook-github-bot · 2022-05-06T01:23:16Z

@jiayuebao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jiayuebao

Could you fix the build failure by adding MemoryTierCacheConfig.h to "cachelib/allocator/TARGETS" and CacheAllocatorConfigTest.cpp to "cachelib/allocator/tests/TARGETS"?

jiayuebao · 2022-05-12T02:01:43Z

cachelib/allocator/MemoryTierCacheConfig.h

+  MemoryTierCacheConfig() = default;
+};
+} // namespace cachelib
+} // namespace facebook


Could you add a new line?

cachelib/allocator/MemoryTierCacheConfig.h

…nd of MemoryTierCacheConfig.h

facebook-github-bot · 2022-05-12T14:39:29Z

@victoria-mcgrath has updated the pull request. You must reimport the pull request before landing.

victoria-mcgrath · 2022-05-12T14:43:39Z

Could you fix the build failure by adding MemoryTierCacheConfig.h to "cachelib/allocator/TARGETS" and CacheAllocatorConfigTest.cpp to "cachelib/allocator/tests/TARGETS"?

@jiayuebao I don't seem to have access/visibility to TARGETS files. Could you give me more specific instructions how I can address your request?

sathyaphoenix · 2022-05-12T22:46:06Z

cachelib/allocator/CacheAllocatorConfig.h

+  // all tier sizes
+  CacheAllocatorConfig& configureMemoryTiers(const MemoryTierConfigs& configs);
+
+  // Sets total cache size and configures cache memory tiers. This method


does this override the previously set cache size ? And by cache size we mean the DRAM cache size ?

By cache size we mean "total cache size" which is the sum of all memory tier sizes. By default there's only one DRAM memory tier whose size matches total cache size.

I decided to delete this 2 argument configureMemoryTiers method.

sathyaphoenix · 2022-05-12T22:47:56Z

cachelib/allocator/CacheAllocatorConfig.h

  // cachePersistence()
  CacheAllocatorConfig& usePosixForShm();

+  // Configures cache memory tiers. Accepts vector of MemoryTierCacheConfig.


Is there a description of what Memory tiers refers to here ? There is already the notion of tiers through hybrid cache. It would help if we can point to some documentation about the memory tier here.

I updated this method's description to list possible memory types for memory tiers. In the future, we could convert this write-up #102 into a documentation section to refer to.

jiayuebao · 2022-05-13T00:25:55Z

@victoria-mcgrath I just realized the TARGETS files aren't synced to the Github. Let me check whether we should modify the file internally on our end.

… added validation funciton and a test for tier sizes

facebook-github-bot · 2022-05-17T18:52:53Z

@victoria-mcgrath has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-05-17T19:48:02Z

@victoria-mcgrath has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-05-17T20:10:49Z

@jiayuebao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jiayuebao · 2022-05-24T21:44:54Z

cachelib/allocator/CacheAllocatorConfig.h

        "It's not allowed to enable both RemoveCB and ItemDestructor.");
  }
+
+  validateMemoryTierSizes();


@victoria-mcgrath Adding this line will cause the existing tests to fail. Can we only introduce the multi-tier cache config in this PR without affecting the existing use cases?

@jiayuebao I fixed failing tests by updating default DRAM tier to have size that matches default total cache size. In general, changes in this PR shouldn't break current DRAM cache configuration. I ran a subset of unit tests locally and confirmed that they passed. Please let me know of remaining failing tests if any.

cachelib/allocator/CacheAllocatorConfig.h

jiayuebao · 2022-05-24T22:01:56Z

cachelib/allocator/CacheAllocatorConfig.h


 template <typename T>
-CacheAllocatorConfig<T>& CacheAllocatorConfig<T>::setCacheSize(size_t _size) {
+CacheAllocatorConfig<T>& CacheAllocatorConfig<T>::setCacheSizeImpl(


Shall we not change setCacheSize function directly as other users are using this function without setting the multi-tier cache config? Can we add a new setCacheSize function only for multi-tier caches?

This PR includes support for current DRAM only cache configuration which can be configured as a single tier cache, and it is a default configuration (line #608). Changing setCacheSize to support 1 or more tiers is one of the changes required to initiate multi-tier support in general. Could you elaborate more on what you meant and advantages of having 2 separate methods?

cachelib/allocator/CacheAllocatorConfig.h

facebook-github-bot · 2022-05-25T00:55:32Z

@victoria-mcgrath has updated the pull request. You must reimport the pull request before landing.

Co-authored-by: Jiayue Bao <40874685+jiayuebao@users.noreply.github.com>

facebook-github-bot · 2022-05-25T16:14:38Z

@victoria-mcgrath has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-05-25T20:10:39Z

@victoria-mcgrath has updated the pull request. You must reimport the pull request before landing.

…ze; deleted unused code.

facebook-github-bot · 2022-05-25T21:11:33Z

@victoria-mcgrath has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-05-26T15:23:06Z

@jiayuebao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jiayuebao · 2022-06-07T00:21:20Z

Hi @victoria-mcgrath: The change will still break tons of our internal tests. The reason is not all cachelib user cases are calling setCacheSize() to configure the cache size; some of them are using config.size = xxx. In this case, could you try not to directly change the existing cachelib APIs but introduce new ones for the change?

BTW, can I know whether the test failure is visible from your side?

jiayuebao · 2022-06-15T18:01:36Z

Hey @victoria-mcgrath any update on the fix?

victoria-mcgrath · 2022-06-16T13:29:50Z

Hey @victoria-mcgrath any update on the fix?

@jiayuebao The changes in the config API aren't supposed to be breaking the existing functionality and no additional API should be required: current DRAM cache config is going to be configured as a multi-tiered cache with a single tier.

However, setting the total cache size without calling setCacheSize is problematic for multi-tier enabling. I can fix the failing tests (I believe I only see failures from compact-cache-test-CCacheTests, but I never run the entire suite, just a large subset) by either making them call setCacheSize (after making "size" private) or I can change a check in validation method (see last if-else) to maintain compatibility with the existing code:

template <typename T>
void CacheAllocatorConfig<T>::validateMemoryTierSizes() {
  size_t sumSizes = 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 (sumSizes != size) {
    if (memoryTierConfigs.size() == 1) {
      // Allow setting cache size explicitly without calling setCacheSize method
      // when there's only one memory tier
      memoryTierConfigs[0].setSize(size);
    } else {
      throw std::invalid_argument(
          "Sum of sizes of all cache tiers must equal to total cache size.");
    }
  }
}

For the later option, I'd need to omit const-ness from validate method though.

EDIT: there's a 3rd option: delete validateMemoryTierSizes all together but it seems even more messy.

What do you think?

victoria-mcgrath · 2022-06-20T14:51:13Z

@jiayuebao I just discussed this problem with @igchor and he had a much better idea to work around this problem: we can only allow setting ratios of tiers and calculate their sizes only when we actually allocate memory. Then, the timing and method of setting the total cache size shouldn't seem to matter. In our internal experiments, we never needed to specify tier sizes explicitly, so this has been so far a somewhat redundant feature anyways. We'll work on the change and share it here.

victoria-mcgrath · 2022-06-20T20:24:26Z

@jiayuebao and @igchor: please review the latest changes that disable size property for tiers.

facebook-github-bot · 2022-06-20T20:53:04Z

@victoria-mcgrath has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-06-20T22:48:48Z

@victoria-mcgrath has updated the pull request. You must reimport the pull request before landing.

jiayuebao · 2022-06-21T00:07:40Z

@jiayuebao and @igchor: please review the latest changes that disable size property for tiers.

Will let @igchor review first. If things look good, I will re-import the change again.

cachelib/allocator/CacheAllocatorConfig.h

cachelib/allocator/tests/CacheAllocatorConfigTest.cpp

facebook-github-bot · 2022-06-21T14:08:15Z

@victoria-mcgrath has updated the pull request. You must reimport the pull request before landing.

igchor

Looks good to me

facebook-github-bot · 2022-06-21T17:29:57Z

@jiayuebao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

…caches (facebook#138) Summary: These changes introduce per-tier cache configuration required to implement features discussed here: facebook#102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced. Pull Request resolved: facebook#138 Reviewed By: therealgymmy Differential Revision: D36189766 Pulled By: jiayuebao fbshipit-source-id: 947aa0cd800ea6accffc1b7b6b0c9693aa7fc0a5

* #75: Use actual tier sizes (rounded down to slab size and decreased by header size) when creating new memory pools * Added getPoolSize method to calculate combined pool size for all tiers; added pool size validation to tests * Explicitly specified type for totalCacheSize to avoid overflow * Minor test change * Reworked tests * Minor change * Deleted redundant tests * Deleted unused constant * First set of changes to cache configuration API to enable multi-tier caches (facebook#138) Summary: These changes introduce per-tier cache configuration required to implement features discussed here: facebook#102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced. Pull Request resolved: facebook#138 Reviewed By: therealgymmy Differential Revision: D36189766 Pulled By: jiayuebao fbshipit-source-id: 947aa0cd800ea6accffc1b7b6b0c9693aa7fc0a5 Co-authored-by: Victoria McGrath <victoria.mcgrath@intel.com>

* pmem#75: Use actual tier sizes (rounded down to slab size and decreased by header size) when creating new memory pools * Added getPoolSize method to calculate combined pool size for all tiers; added pool size validation to tests * Explicitly specified type for totalCacheSize to avoid overflow * Minor test change * Reworked tests * Minor change * Deleted redundant tests * Deleted unused constant * First set of changes to cache configuration API to enable multi-tier caches (facebook#138) Summary: These changes introduce per-tier cache configuration required to implement features discussed here: facebook#102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced. Pull Request resolved: facebook#138 Reviewed By: therealgymmy Differential Revision: D36189766 Pulled By: jiayuebao fbshipit-source-id: 947aa0cd800ea6accffc1b7b6b0c9693aa7fc0a5 Co-authored-by: Victoria McGrath <victoria.mcgrath@intel.com>

* #75: Use actual tier sizes (rounded down to slab size and decreased by header size) when creating new memory pools * Added getPoolSize method to calculate combined pool size for all tiers; added pool size validation to tests * Explicitly specified type for totalCacheSize to avoid overflow * Minor test change * Reworked tests * Minor change * Deleted redundant tests * Deleted unused constant * First set of changes to cache configuration API to enable multi-tier caches (facebook#138) Summary: These changes introduce per-tier cache configuration required to implement features discussed here: facebook#102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced. Pull Request resolved: facebook#138 Reviewed By: therealgymmy Differential Revision: D36189766 Pulled By: jiayuebao fbshipit-source-id: 947aa0cd800ea6accffc1b7b6b0c9693aa7fc0a5 Co-authored-by: Victoria McGrath <victoria.mcgrath@intel.com>

* pmem#75: Use actual tier sizes (rounded down to slab size and decreased by header size) when creating new memory pools * Added getPoolSize method to calculate combined pool size for all tiers; added pool size validation to tests * Explicitly specified type for totalCacheSize to avoid overflow * Minor test change * Reworked tests * Minor change * Deleted redundant tests * Deleted unused constant * First set of changes to cache configuration API to enable multi-tier caches (facebook#138) Summary: These changes introduce per-tier cache configuration required to implement features discussed here: facebook#102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced. Pull Request resolved: facebook#138 Reviewed By: therealgymmy Differential Revision: D36189766 Pulled By: jiayuebao fbshipit-source-id: 947aa0cd800ea6accffc1b7b6b0c9693aa7fc0a5 Co-authored-by: Victoria McGrath <victoria.mcgrath@intel.com>

author Chorazewicz, Igor <igor.chorazewicz@intel.com> 1632834667 +0200 committer Daniel Byrne <byrnedj12@gmail.com> 1671070203 -0800 Initial multi-tier support implementation Extend CompressedPtr to work with multiple tiers Now it's size is 8 bytes intead of 4. Original CompressedPtr stored only some offset with a memory Allocator. For multi-tier implementation, this is not enough. We must also store tierId and when uncompressing, select a proper allocator. An alternative could be to just resign from CompressedPtr but they are leveraged to allow the cache to be mapped to different addresses on shared memory. Changing CompressedPtr impacted CacheItem size - it increased from 32 to 44 bytes. Implemented async Item movement between tiers (with corrected behaivour of tryEvictToNextMemoryTier). Fix ReaperSkippingSlabTraversalWhileSlabReleasing test The issue was caused by incorrect behaviour of the CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier method in case the evicted item is expired. We cannot simply return a handle to it, but we need to remove it from the access container and MM container. Enable workarounds in tests Add basic multi-tier test Set correct size for each memory tier Do not compensate for rounding error when calculating tier sizes (pmem#43) Compensation results in ratios being different than originially specified. Fixed total cache size in CacheMemoryStats (pmem#38) Return a sum of sizes of each tier instead of just 1st tier's size. Issue75 rebased (pmem#88) * pmem#75: Use actual tier sizes (rounded down to slab size and decreased by header size) when creating new memory pools * Added getPoolSize method to calculate combined pool size for all tiers; added pool size validation to tests * Explicitly specified type for totalCacheSize to avoid overflow * Minor test change * Reworked tests * Minor change * Deleted redundant tests * Deleted unused constant * First set of changes to cache configuration API to enable multi-tier caches (facebook#138) Summary: These changes introduce per-tier cache configuration required to implement features discussed here: facebook#102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced. Pull Request resolved: facebook#138 Reviewed By: therealgymmy Differential Revision: D36189766 Pulled By: jiayuebao fbshipit-source-id: 947aa0cd800ea6accffc1b7b6b0c9693aa7fc0a5 Co-authored-by: Victoria McGrath <victoria.mcgrath@intel.com> Fix eviction flow and removeCb calls Without this fix removeCb called even in case when Item is moved between tiers. Fix issue with "Destorying an unresolved handle" The issue happened when ReadHandleImpl ctor needs to destroy waitContext_ because addWaitContextForMovingItem() returns false. So before destroying waitContext_ we are calling discard method to notify ~ItemWaitContext() that Item is ready. Fix slab release code Get tier id of item before calling any function on allocator (which needs the tierID).

* New class MemoryTierCacheConfig allows to configure a memory tier. Setting tier size and location of a file for file-backed memory are supported in this initial implementation; * New member, vector of memory tiers, is added to class CacheAllocatorConfig. * New test suite, chelib/allocator/tests/MemoryTiersTest.cpp, demonstrates the usage of and tests extended config API. Integrate Memory Tier config API with CacheAllocator. Enabled memory tier config API for cachebench. Assumes NUMA bindings support - this commit was done before tiering support enabled. fix memBind call replaces Add MemoryTierCacheConfig::fromShm() (commit 1c7c15d) Move validation code for memory tiers to validate() method and convert ratios to sizes lazily Do not compensate for rounding error when calculating tier sizes (pmem#43) Compensation results in ratios being different than originially specified. Fixed total cache size in CacheMemoryStats (pmem#38) Return a sum of sizes of each tier instead of just 1st tier's size. Issue75 rebased (pmem#88) * pmem#75: Use actual tier sizes (rounded down to slab size and decreased by header size) when creating new memory pools * Added getPoolSize method to calculate combined pool size for all tiers; added pool size validation to tests * Explicitly specified type for totalCacheSize to avoid overflow * Minor test change * Reworked tests * Minor change * Deleted redundant tests * Deleted unused constant * First set of changes to cache configuration API to enable multi-tier caches (facebook#138) Summary: These changes introduce per-tier cache configuration required to implement features discussed here: facebook#102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced. Pull Request resolved: facebook#138 Reviewed By: therealgymmy Differential Revision: D36189766 Pulled By: jiayuebao fbshipit-source-id: 947aa0cd800ea6accffc1b7b6b0c9693aa7fc0a5 Co-authored-by: Victoria McGrath <victoria.mcgrath@intel.com>

…222) Summary: This PR is to add support for NUMA bindings to the memory tier config that was upstreamed in #138 A quick summary of the PR is: - Add memory tier configs to cache allocator based on NUMA bindings - Add in MemoryTier tests - Add getPoolSize() and inline the calculate tier size for tests - Increase max number of tiers to 2 for tiers tests Pull Request resolved: #222 Reviewed By: haowu14 Differential Revision: D47101857 Pulled By: therealgymmy fbshipit-source-id: 346b78a561e3badb5e2c403c0dcf3c215264c31c

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 2, 2022

victoria-mcgrath force-pushed the tier-cfg branch from 4a03bd2 to dcca025 Compare May 4, 2022 14:18

victoria-mcgrath mentioned this pull request May 5, 2022

Upstreaming plan (5/4/2022) pmem/CacheLib#68

Closed

jiayuebao requested changes May 12, 2022

View reviewed changes

Changed ratio parameter from double to size_t, added newline at the e…

8b9f1f3

…nd of MemoryTierCacheConfig.h

sathyaphoenix reviewed May 12, 2022

View reviewed changes

victoria-mcgrath added 2 commits May 17, 2022 12:45

Added examples of types of memory where a cache tier can be created

c83bff4

Clarified the meaning of total cache size as a sum of all tier sizes;…

808ab4e

… added validation funciton and a test for tier sizes

Deleted method that reset total cache size when configuring tiers

d86302e

jiayuebao requested changes May 24, 2022

View reviewed changes

jiayuebao reviewed May 25, 2022

View reviewed changes

cachelib/allocator/CacheAllocatorConfig.h Outdated Show resolved Hide resolved

A typo in setCacheSize?

8037f6d

Fixed typo

54fa491

Co-authored-by: Jiayue Bao <40874685+jiayuebao@users.noreply.github.com>

Corrected one of the new tests

49c72f2

Added setting default size for default tier to match default cache si…

43a44d6

…ze; deleted unused code.

guptask mentioned this pull request Jun 6, 2022

[Upstreaming] Added file-based SHM segment option in SHM module pmem/CacheLib#74

Closed

Disabled memory tier size setting

441c35a

victoria-mcgrath force-pushed the tier-cfg branch from 2572a26 to 441c35a Compare June 20, 2022 22:25

igchor reviewed Jun 21, 2022

View reviewed changes

cachelib/allocator/CacheAllocatorConfig.h Outdated Show resolved Hide resolved

cachelib/allocator/CacheAllocatorConfig.h Outdated Show resolved Hide resolved

cachelib/allocator/tests/CacheAllocatorConfigTest.cpp Show resolved Hide resolved

Addressed reviewer's comments

f942a49

igchor reviewed Jun 21, 2022

View reviewed changes

facebook-github-bot closed this in 0673525 Jun 22, 2022

guptask mentioned this pull request Aug 25, 2022

Rebase develop intel/CacheLib#4

Closed

byrnedj mentioned this pull request Apr 3, 2023

Extend memory tier configs to cache allocator based on NUMA bindings #222

Closed

First set of changes to cache configuration API to enable multi-tier caches #138

First set of changes to cache configuration API to enable multi-tier caches #138

Uh oh!

Conversation

victoria-mcgrath commented May 2, 2022

Uh oh!

facebook-github-bot commented May 6, 2022

Uh oh!

jiayuebao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot commented May 12, 2022

Uh oh!

victoria-mcgrath commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiayuebao commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented May 17, 2022

Uh oh!

facebook-github-bot commented May 17, 2022

Uh oh!

facebook-github-bot commented May 17, 2022

Uh oh!

jiayuebao May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jiayuebao May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot commented May 25, 2022

Uh oh!

facebook-github-bot commented May 25, 2022

Uh oh!

facebook-github-bot commented May 25, 2022

Uh oh!

facebook-github-bot commented May 25, 2022

Uh oh!

facebook-github-bot commented May 26, 2022

Uh oh!

jiayuebao commented Jun 7, 2022

Uh oh!

jiayuebao commented Jun 15, 2022

Uh oh!

victoria-mcgrath commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

victoria-mcgrath commented Jun 20, 2022

Uh oh!

victoria-mcgrath commented Jun 20, 2022

Uh oh!

facebook-github-bot commented Jun 20, 2022

Uh oh!

facebook-github-bot commented Jun 20, 2022

Uh oh!

jiayuebao commented Jun 21, 2022

Uh oh!

Uh oh!

victoria-mcgrath commented May 12, 2022 •

edited

Loading

jiayuebao commented May 13, 2022 •

edited

Loading

jiayuebao May 24, 2022 •

edited

Loading

jiayuebao May 24, 2022 •

edited

Loading

victoria-mcgrath commented Jun 16, 2022 •

edited

Loading