[upstream] extend the memory tier config to work with NUMA bindings and add tests

[byrnedj]

• 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

---

This change is 

[igchor]

Reviewed 10 of 11 files at r1, all commit messages.
Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @byrnedj and @vinser52)

---

cachelib/allocator/CacheAllocator-inl.h line 116 at r1 (raw file):

XDCHECK_EQ(memoryTierConfigs.size(), 1ul);
opts.memBindNumaNodes = memoryTierConfigs[0].getMemBind();
if (memoryTierConfigs.size() > 1) {

There is already assert 2 lines above so we'll never reach this line if we have 2 tiers. Perhaps it might be better to throw this exception from Config (congiureMemoryTiers) for now?

---

cachelib/allocator/CacheAllocator-inl.h line 2184 at r1 (raw file):

template <typename CacheTrait>
size_t CacheAllocator<CacheTrait>::getPoolSize(PoolId poolId) const {

Do you actually need this function? I think you could call getPoolStats and get the size from there.

---

cachelib/cachebench/util/CacheConfig.cpp line 104 at r1 (raw file):

JSONSetVal(configJson, customConfigJson);
JSONSetVal(configJson, usePosixShm);

why it's duplicated?

[vinser52]

Reviewed all commit messages.
Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @byrnedj and @igchor)

---

cachelib/cachebench/util/CacheConfig.cpp line 104 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

why it's duplicated?

Yeah, I think I already upstreamed these changes to Meta's main branch. Have you updated our main?

[igchor]

clang-format should be fixed

Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @byrnedj and @vinser52)

[byrnedj]

done

Reviewable status: 3 of 11 files reviewed, 3 unresolved discussions (waiting on @igchor and @vinser52)

---

cachelib/allocator/CacheAllocator-inl.h line 116 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

There is already assert 2 lines above so we'll never reach this line if we have 2 tiers. Perhaps it might be better to throw this exception from Config (congiureMemoryTiers) for now?

configureMemoryTiers will throw if greater than kMaxTiers, so I removed this line.

---

cachelib/allocator/CacheAllocator-inl.h line 2184 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Do you actually need this function? I think you could call getPoolStats and get the size from there.

Done.

---

cachelib/cachebench/util/CacheConfig.cpp line 104 at r1 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Yeah, I think I already upstreamed these changes to Meta's main branch. Have you updated our main?

Done.

[vinser52]

Reviewable status: 3 of 11 files reviewed, 4 unresolved discussions (waiting on @byrnedj and @igchor)

---

cachelib/cachebench/util/CacheConfig.cpp line 102 at r2 (raw file):

JSONSetVal(configJson, nvmAdmissionRetentionTimeThreshold);
JSONSetVal(configJson, customConfigJson);

Why this line is removed?

[vinser52]

Reviewable status: 3 of 11 files reviewed, 5 unresolved discussions (waiting on @byrnedj and @igchor)

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 36 at r2 (raw file):

 {MemoryTierCacheConfig::fromShm().setRatio(1).setMemBind(
std::string("0")),
MemoryTierCacheConfig::fromShm().setRatio(1).setMemBind(

Will this test fail today (because only a single tier is allowed)?

[byrnedj]

Reviewable status: 3 of 11 files reviewed, 5 unresolved discussions (waiting on @igchor and @vinser52)

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 36 at r2 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Will this test fail today (because only a single tier is allowed)?

No because I increase kMaxTiers to 2 in CacheAllocatorConfig.h

---

cachelib/cachebench/util/CacheConfig.cpp line 102 at r2 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Why this line is removed?

I just noticed this when removing the duplicate - I fixed now. There should be no change to this file.

Code quote:

customConfigJson

[vinser52]

Reviewed 1 of 1 files at r3.
Reviewable status: 4 of 11 files reviewed, 4 unresolved discussions (waiting on @byrnedj and @igchor)

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 36 at r2 (raw file):

Previously, byrnedj (Daniel Byrne) wrote…

No because I increase kMaxTiers to 2 in CacheAllocatorConfig.h

In that case, I think we should throw an exception in the CacheAllocator constructor if (number of tiers > 1) because CacheAllocator is not enabled yet to support more then 1 tier.

[byrnedj]

Reviewable status: 4 of 11 files reviewed, 4 unresolved discussions (waiting on @igchor and @vinser52)

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 36 at r2 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

In that case, I think we should throw an exception in the CacheAllocator constructor if (number of tiers > 1) because CacheAllocator is not enabled yet to support more then 1 tier.

We currently have assert in CacheAllocator constructor to check number of tiers == 1, if you think exception is better I can change.

[vinser52]

Reviewable status: 4 of 11 files reviewed, 4 unresolved discussions (waiting on @byrnedj and @igchor)

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 36 at r2 (raw file):

Previously, byrnedj (Daniel Byrne) wrote…

We currently have assert in CacheAllocator constructor to check number of tiers == 1, if you think exception is better I can change.

Yeah, I think an exception is better because assert is usually used to check for an unexpected condition caused by a mistake in the code logic. In our case, config allows to set 2 tiers but CacheAllocator does not support it yet. In other words cache config is the external input and we cannot control it - it is better to throw an exception in that case.

[byrnedj]

Reviewable status: 4 of 11 files reviewed, 4 unresolved discussions (waiting on @igchor and @vinser52)

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 36 at r2 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Yeah, I think an exception is better because assert is usually used to check for an unexpected condition caused by a mistake in the code logic. In our case, config allows to set 2 tiers but CacheAllocator does not support it yet. In other words cache config is the external input and we cannot control it - it is better to throw an exception in that case.

okay - done.

[vinser52]

Reviewable status: 4 of 11 files reviewed, 3 unresolved discussions (waiting on @igchor)

[vinser52]

Reviewable status: 4 of 11 files reviewed, 3 unresolved discussions (waiting on @igchor)