Rebased develop with NUMA bindings and proposed upstream PRs #56

byrnedj · 2023-02-02T12:19:16Z

This contains the following proposed upstream patches:

Introduce 'markedForEviction' state for the Item, Introduce 'markedForEviction' state for the Item. facebook/CacheLib#183
Add combined locking support for MMContainer, Add combined locking support for MMContainer facebook/CacheLib#182
Expose 'eviction' and 'moving' states for an item, facebook@6aa4078
Change the cache size calculation to use getCacheSize(), [upstream] use getCacheSize() in shm construction #48
Add memory tier configs to cache allocator based on NUMA bindings, [upstream] extend the memory tier config to work with NUMA bindings and add tests #49
CompressedPtr, Compressed pointer addressing in single and 2-tier mode facebook/CacheLib#188
Add memory usage statistics for allocation classes, [upstream] Add memory usage statistics for allocation classes #42
- RollingStats per class, [Upstream] Rolling Stats rebased against the upstream AC Stats branch #47

The rest of develop features have been rebased:

AC stats - approx slab percentage per class
background workers
transparent item sync - with waiters
configs for multi-tier now rely on NUMA bindings
all CI related commits have been squashed together (first commit 5130a65)

I did not include file backed shared memory in this version.

All tests compile and pass and there is no performance difference in the hit_ratio configs. Navy consistency runs and benchmarks work.

I did my best to separate out the multi-tier additional patches for the upstream PRs (i.e. rolling stats upstream commit is followed by the multi-tier implementation for that feature - such as adding TierId and outputting)

This change is

guptask · 2023-02-02T18:47:13Z

What's the overall goal with this rebase:
Is it to bring the latest changes from Meta into develop ?
OR
Did you refactor the feature commits so that we can just cherry-pick one or more commit from here and raise upstream PRs ? If this is the primary goal, then I think it would help if you modify the PR description to group the dependent features together (grouped based on the staggered upstreaming plan) e.g. rolling stats cannot exist independently. It needs AC stats to work.

byrnedj · 2023-02-02T20:32:12Z

Is it to bring the latest changes from Meta into develop ?

Yes

Did you refactor the feature commits so that we can just cherry-pick one or more commit from here and raise upstream PRs ? If this is the primary goal, then I think it would help if you modify the PR description to group the dependent features together (grouped based on the staggered upstreaming plan) e.g. rolling stats cannot exist independently. It needs AC stats to work.

This is also the goal of the rebase - I put rolling stats as a sub bullet.

igchor · 2023-02-02T20:53:15Z

I'm not sure if we should have this commit (Enable touchValue by default): 08916e3 I think it's fine to remove it and just set it in the config every time.

igchor · 2023-02-02T20:54:48Z

Why does this commit (added per tier pool class rolling average latency (based on upstream PR)) 6b41f3b introduce kMaxTiers? Isn't it added in some earlier commit?

guptask · 2023-02-02T21:03:35Z

Why does this commit (added per tier pool class rolling average latency (based on upstream PR)) 6b41f3b introduce kMaxTiers? Isn't it added in some earlier commit?

kMaxTiers change should be part of compressed ptr commit since by design those changes support <= 2 tiers. It is currently not part of facebook#188

igchor · 2023-02-02T22:08:05Z

Why does this commit (added per tier pool class rolling average latency (based on upstream PR)) 6b41f3b introduce kMaxTiers? Isn't it added in some earlier commit?

kMaxTiers change should be part of compressed ptr commit since by design those changes support <= 2 tiers. It is currently not part of facebook#188

Ok, I just thought we introduced this constant in some earlier commit (on develop).

igchor · 2023-02-02T22:12:46Z

cachelib/allocator/Refcount.h

+     if (atomicUpdateValue(predicate, newValue)) {
+         return incOk;
+     } 
+     Value val = __atomic_load_n(&refCount_, __ATOMIC_RELAXED);


Hm, this part of the code looks incorrect. There is a race possible. We should probably return the incResult from predicate (through reference perhaps).

Is this a rebase conflict?

igchor · 2023-02-02T22:16:00Z

cachelib/allocator/tests/MMTypeTest.h

-    ASSERT_FALSE(node.isInMMContainer());
-    if (iter) {
-      ASSERT_NE((*iter).getId(), node.getId());
+  c.withEvictionIterator([&foundNodes, &c](auto&& iter) {


I think this is based on old version of CS-patch PR. In the newest one I have rolled-back those changes to tests to limit number of lines changed. You can see here (https://github.com/facebook/CacheLib/pull/182/files#diff-5298053236abd39332070ce33eda4db9571ef064728bfe48bbf1747306c32997) that I just extended the existing tests with verifyIterationVariants

igchor · 2023-02-02T22:19:35Z

Where is the fix in (Fix moveRegularItemWithSync and add tests): 2d80fb1 ? I only see tests in that commit

igchor · 2023-02-02T22:22:15Z

I'm not sure if this commit (Support for approx free percentage in each allocation class based): e4ce2e7 is still needed after: e756b24

I think it would be better to just leave the commit from https://github.com/intel/CacheLib/pull/42/commits and use that in background workers (functionality is basically the same). What do you think?

igchor · 2023-02-02T22:22:56Z

7d105a7 (Create token before marking item as exclusive) and 9283cc8 (wakeUpWaiters in SlabRelease code (based on upstream) and d2cbd5b (Fix moving for slab release) and 752971c (Insert the child to MMContainer after incrementing) can be squashed with he CS-patch commit.

233be02 (Do not block reader if a child item is moving) potentially could be squashed as well but I'm not sure about this - it looks more like a temporary solution until we have working chained items.

igchor · 2023-02-02T22:27:56Z

I'm not sure if we need to keep 2d80fb1 (Implemented async Item movement between tier) as a separate commit. The final solution is to use the moving bit so perhaps we should squash with critical section patch?

Summary: GitHub commits: facebook/wangle@43e5620 facebookexperimental/edencommon@3934bec facebookincubator/katran@a4c94aa Reviewed By: jurajh-fb fbshipit-source-id: 720310662068e4aa245198d817666700c9dde1d5

Summary: GitHub commits: facebook/fb303@c51a290 facebook/fbthrift@5e97023 facebook/proxygen@3710a32 facebook/wangle@fd29800 facebook/watchman@fca53d2 facebookexperimental/edencommon@3d8a6d9 facebookexperimental/rust-shed@75c5d75 facebookincubator/fizz@bae2d7e facebookincubator/katran@139c75d facebook/mvfst@001332d facebookincubator/velox@ec0ea1f Reviewed By: jurajh-fb fbshipit-source-id: a8a8ff6a7fddeaa7bbccb3faa6a737c50d22cfe2

Summary: As mentioned in the tasks, these stats make more sense in rates. Reviewed By: jiayuebao Differential Revision: D43243498 fbshipit-source-id: 556b0b4a062235bb3b34dd076e5f7a5a88316f8c

Summary: GitHub commits: facebook@ba9dd68 facebook/fbthrift@23208ff facebookincubator/velox@68b8a45 Reviewed By: jurajh-fb fbshipit-source-id: f8581e13cd1c0a2510d87f42c813ca2c65eaef1d

guptask · 2023-02-17T20:09:16Z

cachelib/allocator/memory/CompressedPtr.h line 151 at r1 (raw file):

  uint32_t getAllocIdx(bool isMultiTiered) const noexcept {
    XDCHECK(!isNull());
    auto noTierIdPtr = isMultiTiered ? ptr_ & ~kTierIdxMask : ptr_;

This section has changed based on latest review comments from Meta. Please refer to facebook@969d5c9

guptask · 2023-02-17T20:09:52Z

cachelib/allocator/memory/CompressedPtr.h line 46 at r1 (raw file):

// the  slab  index. Hence we can index 128 GiB of memory in slabs per tier and
// index anything more than 64 byte allocations inside the slab using a 32 bit
// representation.

The summary has changed based on review comments from Meta. Please refer to facebook@969d5c9

Summary: GitHub commits: facebook/fbthrift@3b00d58 Reviewed By: jurajh-fb fbshipit-source-id: 23ecb07e7e9ebd4dfbf3980de141ea1ebe36bb12

Summary: GitHub commits: facebook/fbthrift@df0eb61 facebook/proxygen@7fc3fea Reviewed By: jurajh-fb fbshipit-source-id: fa982baf35fd98edc52be391f03d15a9c2c333bf

Summary: GitHub commits: facebook/fbthrift@1d01d76 Reviewed By: jurajh-fb fbshipit-source-id: bdefba88e8d002c65d8b7c11e86c8aa772996d9b

Summary: GitHub commits: facebook/fbthrift@1b848d8 facebook/rocksdb@cfe50f7 pytorch/FBGEMM@c55d800 Reviewed By: bigfootjon fbshipit-source-id: e840d1116311ac52336e598442cf66cdc50d7725

Summary: GitHub commits: facebook/fbthrift@90fddab Reviewed By: bigfootjon fbshipit-source-id: cf7ac48c00c548805fb74663fe3e9bd9f5906cc2

Summary: Currently `replacedPtr` is passed as an argument and such usage isn't easy to understand (we kept receiving user feedback on that). Now we want to return it. Reviewed By: therealgymmy, antonf, jaesoo-fb Differential Revision: D43143989 fbshipit-source-id: eaae055107050cd67a7ca8b26b716fffc0c002f2

Summary: The Samsung PM9A3 does not report samsung in the model number so I added the specific model number to the vendorMap. Reviewed By: jaesoo-fb Differential Revision: D43676582 fbshipit-source-id: 6df19c40dd9da9563b75aa5847a1d1f9eb6a9aef

Summary: XDCHECK (or XCHECK) fails here for gcc-8.x in a lambda, so we move it outside. This occurs in CacheBench's AsyncCacheStressor.h. ``` /__w/CacheLib/CacheLib/cachelib/../cachelib/cachebench/runner/AsyncCacheStressor.h:306:7: internal compiler error: in cp_build_addr_expr_1, at cp/typeck.c:5965 ``` This line compiled fine with 8.5 before Jan 2023 so I suspect a regression in folly or other external library. It still compiles fine with gcc-7.5, 9.4, and 11.3.1. Possibly related commits: facebook/folly@1aafad4 and facebook/folly@e6d09f6 Line of gcc-8.5 that it fails on: https://github.com/gcc-mirror/gcc/blob/releases/gcc-8.5.0/gcc/cp/typeck.c#L5965 The version that isn't in a lambda compiles just fine: https://github.com/facebook/CacheLib/blob/df5b9f6ef35c55e432b6713c52397a03dd19c34c/cachelib/cachebench/runner/CacheStressor.h#L399 Pull Request resolved: facebook#201 Test Plan: GitHub actions. Built fine on my fork with CentOS 8.5/gcc-8.5. This issue currently causes 3 builds that use gcc-8.5 to fail (2 CentOS and RockyLinux-8.6) and 1 build using gcc-8.3 (Debian). Reviewed By: therealgymmy Differential Revision: D43681854 Pulled By: jaesoo-fb fbshipit-source-id: f3a65aefedcd98a26a80bb6ad009ad0d64e2395b

Summary: Add the following TTL-related APIs: - getConfiguredTtlSec - getExpiryTimeSec - extendTtlSec - updateExpiryTimeSec Usage: ``` auto ptr = objcache->find<T>("key"); auto configuredTtl = objcache->getConfiguredTtl(ptr); auto expiryTime = objcache->getExpiryTimeSec(ptr); objcache->extendTtl(ptr, std::chrono::seconds(3)); objcache->updateExpiryTimeSec(ptr, newExpiryTimeSecs); ``` Reviewed By: therealgymmy, jaesoo-fb Differential Revision: D43167879 fbshipit-source-id: 3b11fb0a2b9a3b5c38fcfd856ade100e6ae27470

Summary: Based on our discussion, this API would be confusing if a reference of ReadHandle is obtained from a WriteHandle. We also don't want to make it virtual because this will increase `sizeof(ReadHandle)` / `sizeof(WriteHandle)` by 8 bytes. Reviewed By: therealgymmy Differential Revision: D43667308 fbshipit-source-id: a77a17113f1a23f84332ebfa7ea6772d7647339c

Summary: This diff has been automatically generated by the inpage editor. NOTE: If you want to update this diff, go via the preview link inside the static docs section below. Ensure you are editing the same page that was used to create this diff. Reviewed By: therealgymmy Differential Revision: D43667238 fbshipit-source-id: 0be4c1ef376a5a1a2de92afc311af24f66d10afd

Summary: 1. Workaround for Debian Docker image bug that is breaking Debian build on GitHub (Explicitly mark Git repo as safe). 2. Pin zstd to a commit that resolves problems with older CMakes (note: affects all OSes, not just Debian) Context for 1: In latest Debian Docker image , there is a regression that affects the checkout action. From actions/checkout#1169: > - Checkout runs, and runs /usr/bin/git config --global --add safe.directory <path> > - The global .gitconfig does not exist > - Any calls to git remain unsafe/dubious The suggested workaround was to use --system instead of --global. Pull Request resolved: facebook#200 Test Plan: See if GitHub Action Debian build is fixed. Reviewed By: therealgymmy Differential Revision: D43720363 Pulled By: jaesoo-fb fbshipit-source-id: 54f3586cc7f8e72045e60d8dd454c7a77725e6b2

Summary: An user in github reported an issue that the installation link is broken. Somehow, docs/installation/installation.md cannot be referenced by `/docs/installation/installation`, but only by `/docs/installation`. The root cause could not figured out yet, but this change fixes it as such for now. Reviewed By: jiayuebao Differential Revision: D43757739 fbshipit-source-id: 4abd3208800c0b68e9162d381f6395897f047b24

Run tests on CI Run long tests (navy/bench) every day on CI Run CI on prebuild docker image Run only centos build on CI Update docker file used in CI Centos8 is EOL Disable failing clang-format-check Add extra param to build-package.sh Add scripts for rebuilding/pushing docker images Taken from: pmem/dev-utils-kit@30794c3 Extend CI to rebuild docker automatically Update build-cachelib-docker.yml Do not use shallow clone to make sure Docker rebuild logic works correctly. Added required packages to install Intel ittapi Update CI to use intel/CacheLib repo (pmem#17) Add multi-tier navy benchmark and run it on CI - fix navy multi-tier config for NUMA bindings added code coverage support in CacheLib Adding libdml to CentOS docker image (pmem#53) only exclude allocator-test-NavySetupTestm, shm-test-test_page_size tests added perf and numactl to docker packages --------------------------------------------- one large commit for all CI and code coverage see above for the change history.

It is similar to 'moving' but requires ref count to be 0. An item which is marked for eviction causes all incRef() calls to that item to fail. This will be used to ensure that once item is selected for eviction, no one can interfere and prevent the eviction from suceeding. 'markedForEviction' relies on the same 'exlusive' bit as the 'moving' state. To distinguish between those two states, 'moving' add 1 to the refCount. This is hidden from the user, so getRefCount() will not return that extra ref.

to utilize combined locking.

- Switch to isUsingPosixShm() method

- Add in MemoryTier tests - Increase max number of tiers to 2 for tiers tests

…ng in single tier mode and use 31 bits for addressing in multi-tier mode

This includes printing: - allocSize - allocated memory size - memory usage fraction

…art 2) fix for compressed ptr (upstream) -> compress from false to true

for different pool sizes. We also use getPoolSize(pid), to get total size from all pools across allocators. It also fixes the tiering sizes (pulls changes from what was issue75 rebased commit that did not make it into upstream commits). Rebased to use ramCacheSize.

for the compressed ptr changes that were introduced upstream. - Includes later cosmetic changes added by sounak 9cb5c29

fix for rolling stats (on multi-tier to be followed by multi-tier rolling stats implementation in the following commit)

Hot queue iterator for 2Q. Will start at Hot queue and move to Warm queue if hot queue is exhausted. Useful for promotion semantics if using 2Q replacement. rebased on to develop and added some tests.

- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement

-updated configs for numa bindings

This would lead to deadlock (.e.g in forEachChainedItem) if the child is moving (e.g. marked by Slab Release thread). Instead treat moving bit only to prevent freeing the item and do all synchronization on parent.

Background data movement using periodic workers. Attempts to evict/promote items per given thresholds for each class. These reduce p99 latency since there is a higher chance that an allocation slot is free in the tier we are allocating in.

…move to fail - updated slab release logic for move failure, but there is still an issue with slab movement. currently investigating.

byrnedj requested review from igchor and guptask February 2, 2023 15:35

igchor reviewed Feb 2, 2023

View reviewed changes

byrnedj force-pushed the rebase_numa branch from cb82748 to 8a4c947 Compare February 10, 2023 15:49

Updating submodules

160bb77

Summary: GitHub commits: facebook/wangle@43e5620 facebookexperimental/edencommon@3934bec facebookincubator/katran@a4c94aa Reviewed By: jurajh-fb fbshipit-source-id: 720310662068e4aa245198d817666700c9dde1d5

guptask mentioned this pull request Feb 16, 2023

Compressed pointer addressing in single and 2-tier mode facebook/CacheLib#188

Closed

byrnedj force-pushed the rebase_numa branch 2 times, most recently from f9f7e5d to ef893e7 Compare February 16, 2023 22:54

Hao Wu and others added 2 commits February 17, 2023 10:09

Change some of the stats to rate

ba9dd68

Summary: As mentioned in the tasks, these stats make more sense in rates. Reviewed By: jiayuebao Differential Revision: D43243498 fbshipit-source-id: 556b0b4a062235bb3b34dd076e5f7a5a88316f8c

Updating submodules

ad8bfd6

Summary: GitHub commits: facebook@ba9dd68 facebook/fbthrift@23208ff facebookincubator/velox@68b8a45 Reviewed By: jurajh-fb fbshipit-source-id: f8581e13cd1c0a2510d87f42c813ca2c65eaef1d

Open Source Bot and others added 6 commits February 18, 2023 11:28

Updating submodules

bdad762

Summary: GitHub commits: facebook/fbthrift@3b00d58 Reviewed By: jurajh-fb fbshipit-source-id: 23ecb07e7e9ebd4dfbf3980de141ea1ebe36bb12

Updating submodules

b42327a

Summary: GitHub commits: facebook/fbthrift@df0eb61 facebook/proxygen@7fc3fea Reviewed By: jurajh-fb fbshipit-source-id: fa982baf35fd98edc52be391f03d15a9c2c333bf

Updating submodules

0d49b53

Summary: GitHub commits: facebook/fbthrift@1d01d76 Reviewed By: jurajh-fb fbshipit-source-id: bdefba88e8d002c65d8b7c11e86c8aa772996d9b

Updating submodules

bb2d1c6

Summary: GitHub commits: facebook/fbthrift@1b848d8 facebook/rocksdb@cfe50f7 pytorch/FBGEMM@c55d800 Reviewed By: bigfootjon fbshipit-source-id: e840d1116311ac52336e598442cf66cdc50d7725

Updating submodules

c167a83

Summary: GitHub commits: facebook/fbthrift@90fddab Reviewed By: bigfootjon fbshipit-source-id: cf7ac48c00c548805fb74663fe3e9bd9f5906cc2

Darryl Gardner and others added 7 commits February 28, 2023 17:07

byrnedj force-pushed the rebase_numa branch 2 times, most recently from aa311f6 to 08bf0b4 Compare March 3, 2023 21:37

igchor and others added 20 commits March 3, 2023 13:38

Adds createPutToken and switches findEviction

c1020df

to utilize combined locking.

- Change the cache size calculation to use getCacheSize()

9bb6775

- Switch to isUsingPosixShm() method

- Add memory tier configs to cache allocator based on NUMA bindings

1759e83

- Add in MemoryTier tests - Increase max number of tiers to 2 for tiers tests

added ability for compressed pointer to use full 32 bits for addressi…

62b0c41

…ng in single tier mode and use 31 bits for addressing in multi-tier mode

Add memory usage statistics for allocation classes

b7459ee

This includes printing: - allocSize - allocated memory size - memory usage fraction

Initial multi-tier support implementation (rebased with NUMA and cs p…

fafface

…art 2) fix for compressed ptr (upstream) -> compress from false to true

AC stats multi-tier

369e55b

This is the additional multi-tier support needed

d209d78

for the compressed ptr changes that were introduced upstream. - Includes later cosmetic changes added by sounak 9cb5c29

added per pool class rolling average latency (upstream PR version)

e74fa40

fix for rolling stats (on multi-tier to be followed by multi-tier rolling stats implementation in the following commit)

added per tier pool class rolling average latency (based on upstream PR)

bab780a

MM2Q promotion iterators (#1)

f0baeb1

Hot queue iterator for 2Q. Will start at Hot queue and move to Warm queue if hot queue is exhausted. Useful for promotion semantics if using 2Q replacement. rebased on to develop and added some tests.

basic multi-tier test based on numa bindings

2152639

Aadding new configs to hit_ratio/graph_cache_leader_fobj

e303104

-updated configs for numa bindings

Do not block reader if a child item is moving

1e40a00

This would lead to deadlock (.e.g in forEachChainedItem) if the child is moving (e.g. marked by Slab Release thread). Instead treat moving bit only to prevent freeing the item and do all synchronization on parent.

Background data movement (pmem#20)

b99bb9d

Background data movement using periodic workers. Attempts to evict/promote items per given thresholds for each class. These reduce p99 latency since there is a higher chance that an allocation slot is free in the tier we are allocating in.

fix race in moveRegularItemWith sync where insertOrReplace can cause …

08bf0b4

…move to fail - updated slab release logic for move failure, but there is still an issue with slab movement. currently investigating.

byrnedj closed this Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebased develop with NUMA bindings and proposed upstream PRs #56

Rebased develop with NUMA bindings and proposed upstream PRs #56

byrnedj commented Feb 2, 2023 •

edited

Loading

guptask commented Feb 2, 2023 •

edited

Loading

byrnedj commented Feb 2, 2023 •

edited

Loading

igchor commented Feb 2, 2023

igchor commented Feb 2, 2023

guptask commented Feb 2, 2023

igchor commented Feb 2, 2023

igchor Feb 2, 2023

igchor Feb 2, 2023

igchor commented Feb 2, 2023

igchor commented Feb 2, 2023

igchor commented Feb 2, 2023 •

edited

Loading

igchor commented Feb 2, 2023

guptask commented Feb 17, 2023

guptask commented Feb 17, 2023

Rebased develop with NUMA bindings and proposed upstream PRs #56

Rebased develop with NUMA bindings and proposed upstream PRs #56

Conversation

byrnedj commented Feb 2, 2023 • edited Loading

guptask commented Feb 2, 2023 • edited Loading

byrnedj commented Feb 2, 2023 • edited Loading

igchor commented Feb 2, 2023

igchor commented Feb 2, 2023

guptask commented Feb 2, 2023

igchor commented Feb 2, 2023

igchor Feb 2, 2023

Choose a reason for hiding this comment

igchor Feb 2, 2023

Choose a reason for hiding this comment

igchor commented Feb 2, 2023

igchor commented Feb 2, 2023

igchor commented Feb 2, 2023 • edited Loading

igchor commented Feb 2, 2023

guptask commented Feb 17, 2023

guptask commented Feb 17, 2023

byrnedj commented Feb 2, 2023 •

edited

Loading

guptask commented Feb 2, 2023 •

edited

Loading

byrnedj commented Feb 2, 2023 •

edited

Loading

igchor commented Feb 2, 2023 •

edited

Loading