Config api integration #2

igchor · 2021-10-29T12:59:33Z

Right now, it only works when Allocator is constructed using shared memory (constructor taking SharedMemNew or SharedMemAttach)

TODO:

enable more tests

This change is

guptask

Please review my comments and reply back. Most of my comments are regarding coding standards compliance and cosmetic changes.

guptask · 2021-11-02T16:08:27Z

cachelib/allocator/CacheAllocator-inl.h

@@ -49,12 +50,16 @@ CacheAllocator<CacheTrait>::CacheAllocator(Config config)
      cacheCreationTime_{util::getCurrentTimeSec()},
      nvmCacheState_{config_.cacheDir, config_.isNvmCacheEncryptionEnabled(),
                     config_.isNvmCacheTruncateAllocSizeEnabled()} {
+  // TODO(MEMORY_TIER)
+  if (memoryTierConfigs.size())
+    throw std::runtime_error("Using custom memory tier is only supported for Shared Memory.");


for coding standards compliance, I would prefer {} for the if condition statement.

guptask · 2021-11-02T16:09:30Z

cachelib/allocator/CacheAllocator-inl.h

@@ -97,7 +102,8 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)

 template <typename CacheTrait>
 CacheAllocator<CacheTrait>::CacheAllocator(SharedMemAttachT, Config config)
-    : isOnShm_{true},
+    : memoryTierConfigs(config.getMemoryTierConfigs()),
+    isOnShm_{true},


cosmetic change: shift isOnShm_ line couple of spaces to the right

guptask · 2021-11-02T16:10:20Z

cachelib/allocator/CacheAllocator-inl.h

-CacheAllocator<CacheTrait>::createNewMemoryAllocator() {
+ShmSegmentOpts CacheAllocator<CacheTrait>::createShmCacheOpts() {
+  if (memoryTierConfigs.size() > 1)
+    throw std::invalid_argument("CacheLib only supports a single memory tier");


{} coding standard compliance needed

guptask · 2021-11-02T16:12:52Z

cachelib/allocator/CacheAllocator-inl.h

  ShmSegmentOpts opts;
  opts.alignment = sizeof(Slab);
-  opts.typeOpts = PosixSysVSegmentOpts(config_.usePosixShm);
+
+  if (memoryTierConfigs.size()) {


please explain this size check - I didn't understand why size > 0 means there'll be a file shm segment

Is this for backward compatibility reasons ?

victoria-mcgrath · 2021-11-02T16:32:18Z

cachelib/allocator/CacheAllocator-inl.h

@@ -68,7 +73,8 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)
                          AccessContainer::getRequiredSize(
                              config_.accessConfig.getNumBuckets()),
                          nullptr,
-                          ShmSegmentOpts(config_.accessConfig.getPageSize()))
+                          ShmSegmentOpts(config_.accessConfig.getPageSize(),
+                              false, config_.usePosixShm))


Please use isUsingPosixShm() method instead of direct access to the member.

victoria-mcgrath · 2021-11-02T16:32:45Z

cachelib/allocator/CacheAllocator-inl.h

@@ -79,7 +85,8 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)
                          AccessContainer::getRequiredSize(
                              config_.chainedItemAccessConfig.getNumBuckets()),
                          nullptr,
-                          ShmSegmentOpts(config_.accessConfig.getPageSize()))
+                          ShmSegmentOpts(config_.accessConfig.getPageSize(),
+                              false, config_.usePosixShm))


isUsingPosixShm()

victoria-mcgrath · 2021-11-02T16:33:43Z

cachelib/allocator/CacheAllocator-inl.h

  initCommon(false);
 }

 template <typename CacheTrait>
 CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)
-    : isOnShm_{true},
+    : memoryTierConfigs(config.getMemoryTierConfigs()),
+      isOnShm_{true},
      config_(config.validate()),
      shmManager_(
          std::make_unique<ShmManager>(config_.cacheDir, config_.usePosixShm)),


Please change to isUsingPosixShm() method instead of direct access to the member.

victoria-mcgrath · 2021-11-02T16:34:07Z

cachelib/allocator/CacheAllocator-inl.h

@@ -89,12 +96,14 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)
      nvmCacheState_{config_.cacheDir, config_.isNvmCacheEncryptionEnabled(),
                     config_.isNvmCacheTruncateAllocSizeEnabled()} {
  initCommon(false);
-  shmManager_->removeShm(detail::kShmInfoName);
+  shmManager_->removeShm(detail::kShmInfoName,
+    PosixSysVSegmentOpts(config_.usePosixShm));


isUsingPosixShm()

victoria-mcgrath · 2021-11-02T16:34:23Z

cachelib/allocator/CacheAllocator-inl.h

@@ -107,13 +116,15 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemAttachT, Config config)
      accessContainer_(std::make_unique<AccessContainer>(
          deserializer_->deserialize<AccessSerializationType>(),
          config_.accessConfig,
-          shmManager_->attachShm(detail::kShmHashTableName),
+          shmManager_->attachShm(detail::kShmHashTableName, nullptr,
+            ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)),


isUsingPosixShm()

victoria-mcgrath · 2021-11-02T16:34:52Z

cachelib/allocator/CacheAllocator-inl.h

          compressor_,
          [this](Item* it) -> ItemHandle { return acquire(it); })),
      chainedItemAccessContainer_(std::make_unique<AccessContainer>(
          deserializer_->deserialize<AccessSerializationType>(),
          config_.chainedItemAccessConfig,
-          shmManager_->attachShm(detail::kShmChainedItemHashTableName),
+          shmManager_->attachShm(detail::kShmChainedItemHashTableName, nullptr,
+            ShmSegmentOpts(PageSizeT::NORMAL, false, config_.usePosixShm)),


isUsingPosixShm() and the rest of the occurrances below...

victoria-mcgrath · 2021-11-02T16:50:54Z

cachelib/shm/FileShmSegment.cpp

+  const int fd = open(name, flags);
+
+  if (fd != -1) {
+    return fd;


I believe the preference should be to have a single return statement for each function. It improves readability and simplifies maintainability.

victoria-mcgrath · 2021-11-02T16:52:33Z

cachelib/shm/FileShmSegment.cpp

+  if (ret == 0) {
+    return;
+  }
+  switch (errno) {


This switch statement is repeated several times throughout this file. It will be better to write a function to do this error analysis and call it instead of copy/pasting the code.

victoria-mcgrath · 2021-11-02T16:53:57Z

cachelib/shm/FileShmSegment.cpp

+    return buf.st_size;
+  } else {
+    throw std::runtime_error(folly::sformat(
+        "Trying to get size of  segment with name {} in an invalid state",


""Trying to get size of segment" -> ""Trying to get size of segment" (delete extra space after 'of').

victoria-mcgrath · 2021-11-02T16:59:55Z

cachelib/shm/ShmManager.cpp

-  DCHECK(nameToKey_.find(shmName) == nameToKey_.end());
+  DCHECK(nameToOpts_.find(shmName) == nameToOpts_.end());
+
+  const auto* v = std::get_if<PosixSysVSegmentOpts>(&opts.typeOpts);


Could 'v' be changed to a more descriptive name?

victoria-mcgrath · 2021-11-02T17:01:17Z

cachelib/shm/SysVShmSegment.h

  // returns the key identifier for the given name.
  static KeyType createKeyForName(const std::string& name) noexcept;

+private:
+


Is this empty line needed here?

victoria-mcgrath · 2021-11-02T17:04:39Z

cachelib/allocator/tests/AllocatorMemoryTiersTest.h

+
+    // More than one tier is not supported
+    ASSERT_THROW(std::make_unique<AllocatorT>(AllocatorT::SharedMemNew, config),
+                 std::invalid_argument);


Will it be possible to add more checks here that would test allocator changes made to support tiers?

victoria-mcgrath · 2021-11-02T17:06:43Z

cachelib/allocator/tests/BaseAllocatorTest.h

@@ -1150,6 +1150,8 @@ class BaseAllocatorTest : public AllocatorTest<AllocatorT> {
    config.setAccessConfig({8, 8});
    config.enableCachePersistence(this->cacheDir_);
    config.enableItemReaperInBackground(std::chrono::seconds(1), {});
+    if (cfgs.size())


Let's move this check to configMemoryTiers()?

igchor

Reviewable status: 0 of 28 files reviewed, 17 unresolved discussions (waiting on @guptask and @victoria-mcgrath)

cachelib/allocator/CacheAllocator-inl.h, line 55 at r4 (raw file):

Previously, guptask (Sounak Gupta) wrote…

for coding standards compliance, I would prefer {} for the if condition statement.

Done.

cachelib/allocator/CacheAllocator-inl.h, line 65 at r4 (raw file):

Previously, victoria-mcgrath wrote…

Please change to isUsingPosixShm() method instead of direct access to the member.

Done.

cachelib/allocator/CacheAllocator-inl.h, line 77 at r4 (raw file):

Previously, victoria-mcgrath wrote…

Please use isUsingPosixShm() method instead of direct access to the member.

Done.

cachelib/allocator/CacheAllocator-inl.h, line 89 at r4 (raw file):

Previously, victoria-mcgrath wrote…

isUsingPosixShm()

Done.

cachelib/allocator/CacheAllocator-inl.h, line 100 at r4 (raw file):

Previously, victoria-mcgrath wrote…

isUsingPosixShm()

Done.

cachelib/allocator/CacheAllocator-inl.h, line 106 at r4 (raw file):

Previously, guptask (Sounak Gupta) wrote…

cosmetic change: shift isOnShm_ line couple of spaces to the right

Done.

cachelib/allocator/CacheAllocator-inl.h, line 120 at r4 (raw file):

Previously, victoria-mcgrath wrote…

isUsingPosixShm()

Done.

cachelib/allocator/CacheAllocator-inl.h, line 127 at r4 (raw file):

Previously, victoria-mcgrath wrote…

isUsingPosixShm() and the rest of the occurrances below...

Done.

cachelib/allocator/CacheAllocator-inl.h, line 161 at r4 (raw file):

Previously, guptask (Sounak Gupta) wrote…

{} coding standard compliance needed

Done.

cachelib/allocator/CacheAllocator-inl.h, line 166 at r4 (raw file):

Previously, guptask (Sounak Gupta) wrote…

Is this for backward compatibility reasons ?

Done.

Actually I tihnk we should implement it in this way(in later PR perhaps):

introduce MemoryTierCacheConfig::fromPosixSysV(bool posix)
initialize memoryTierConfigs in AllocatorConfig to contain this posix/sysv MemoryTierCacheConfig

This would allow us to remove this check for size and hide everything inside config.

@victoria-mcgrath what do you think?

cachelib/allocator/tests/AllocatorMemoryTiersTest.h, line 42 at r4 (raw file):

Previously, victoria-mcgrath wrote…

Will it be possible to add more checks here that would test allocator changes made to support tiers?

This test is specifically for testing multiple tiers - which is not supported for now.

For tests which would just verify if everything works fine with our new segment types, I wanted to modify existing tests (see BaseAllocatorTest - there is only one test enabled currently)

cachelib/allocator/tests/BaseAllocatorTest.h, line 1153 at r4 (raw file):

Previously, victoria-mcgrath wrote…

Let's move this check to configMemoryTiers()?

I'm not 100% sure, do we want to allow calling configureMemoryTiers({})?

cachelib/shm/FileShmSegment.cpp, line 41 at r4 (raw file):

Previously, victoria-mcgrath wrote…

I believe the preference should be to have a single return statement for each function. It improves readability and simplifies maintainability.

This file is not a part of this PR, but anyway, this was copied from existing PosixShm implementation. We can have a discussion about this on this PR: https://reviewable.io/reviews/pmem/cachelib/5

cachelib/shm/FileShmSegment.cpp, line 99 at r4 (raw file):

Previously, victoria-mcgrath wrote…

This switch statement is repeated several times throughout this file. It will be better to write a function to do this error analysis and call it instead of copy/pasting the code.

https://reviewable.io/reviews/pmem/cachelib/5

cachelib/shm/FileShmSegment.cpp, line 266 at r4 (raw file):

Previously, victoria-mcgrath wrote…

""Trying to get size of segment" -> ""Trying to get size of segment" (delete extra space after 'of').

As above.

cachelib/shm/ShmManager.cpp, line 314 at r4 (raw file):

Previously, victoria-mcgrath wrote…

Could 'v' be changed to a more descriptive name?

It's just used in the line belowe so I think it's OK - btw, it's not part of this PR.

cachelib/shm/SysVShmSegment.h, line 95 at r4 (raw file):

Previously, victoria-mcgrath wrote…

Is this empty line needed here?

I'm not sure if it's necessary but I think it's OK

victoria-mcgrath

Reviewed 5 of 9 files at r4, 1 of 24 files at r5.
Reviewable status: 6 of 32 files reviewed, 16 unresolved discussions (waiting on @guptask, @igchor, and @victoria-mcgrath)

cachelib/allocator/tests/BaseAllocatorTest.h, line 1153 at r4 (raw file):

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

I'm not 100% sure, do we want to allow calling configureMemoryTiers({})?

If we don't move this check, then all code users would have to repeat it in their client code. I'd rather have this check done in one place. Besides, the current configure... wouldn't work for the empty vector and it's probably not great.

victoria-mcgrath

Reviewable status: 6 of 32 files reviewed, 16 unresolved discussions (waiting on @guptask, @igchor, and @victoria-mcgrath)

cachelib/shm/FileShmSegment.cpp, line 41 at r4 (raw file):

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

This file is not a part of this PR, but anyway, this was copied from existing PosixShm implementation. We can have a discussion about this on this PR: https://reviewable.io/reviews/pmem/cachelib/5

Moved to 5.

cachelib/shm/FileShmSegment.cpp, line 99 at r4 (raw file):

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

https://reviewable.io/reviews/pmem/cachelib/5

Moved to 5.

cachelib/shm/FileShmSegment.cpp, line 266 at r4 (raw file):

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

As above.

Moved to 5.

victoria-mcgrath

Reviewed 6 of 24 files at r5.
Reviewable status: 12 of 32 files reviewed, 12 unresolved discussions (waiting on @guptask, @igchor, and @victoria-mcgrath)

cachelib/shm/SysVShmSegment.h, line 95 at r4 (raw file):

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

I'm not sure if it's necessary but I think it's OK

Just for consistency with the rest of the code?

igchor

Reviewable status: 12 of 32 files reviewed, 12 unresolved discussions (waiting on @guptask and @victoria-mcgrath)

cachelib/allocator/tests/BaseAllocatorTest.h, line 1153 at r4 (raw file):

Previously, victoria-mcgrath wrote…

If we don't move this check, then all code users would have to repeat it in their client code. I'd rather have this check done in one place. Besides, the current configure... wouldn't work for the empty vector and it's probably not great.

Right, this is the question - do we want to allow passing empty vector to configure? I think not. Ideally we would have one additional MemoryTierCacheConfig which would correspond to legacy behavior (posix/sysv) and we could use that as a default parameter. What do you think?

cachelib/shm/SysVShmSegment.h, line 95 at r4 (raw file):

Previously, victoria-mcgrath wrote…

Just for consistency with the rest of the code?

Ok, but usually we try do such cleanup changes in separate PRs - it makes it easier to revert/navigate through such PRs and commits.

victoria-mcgrath · 2021-11-04T15:40:08Z

cachelib/allocator/tests/BaseAllocatorTest.h, line 1153 at r4 (raw file):

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

Right, this is the question - do we want to allow passing empty vector to configure? I think not. Ideally we would have one additional MemoryTierCacheConfig which would correspond to legacy behavior (posix/sysv) and we could use that as a default parameter. What do you think?

Yes, I like the idea of default parameter to support legacy mode. As for the check, originally I was thinking that we could quietly return from configure on an empty vector, but after thinking more, I suggest throwing an exception in this case because an empty vector may be passed by mistake. What do you think?

victoria-mcgrath · 2021-11-04T18:02:55Z

cachelib/allocator/CacheAllocator-inl.h, line 166 at r4 (raw file):

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

Done.

Actually I tihnk we should implement it in this way(in later PR perhaps):

introduce MemoryTierCacheConfig::fromPosixSysV(bool posix)

initialize memoryTierConfigs in AllocatorConfig to contain this posix/sysv MemoryTierCacheConfig

This would allow us to remove this check for size and hide everything inside config.

@victoria-mcgrath what do you think?

Yes, let's do this.

victoria-mcgrath

Reviewed 3 of 9 files at r4, 17 of 24 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @guptask, @igchor, and @victoria-mcgrath)

igchor

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @guptask and @victoria-mcgrath)

cachelib/allocator/CacheAllocator-inl.h, line 166 at r4 (raw file):

Previously, victoria-mcgrath wrote…

Yes, let's do this.

Done.

cachelib/allocator/tests/BaseAllocatorTest.h, line 1153 at r4 (raw file):

Previously, victoria-mcgrath wrote…

Yes, I like the idea of default parameter to support legacy mode. As for the check, originally I was thinking that we could quietly return from configure on an empty vector, but after thinking more, I suggest throwing an exception in this case because an empty vector may be passed by mistake. What do you think?

Yes, that makes sense. Done.

igchor

@guptask done, please take a look and resolve conversations if you're satisfied.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @guptask and @victoria-mcgrath)

igchor · 2021-11-09T10:13:39Z

All tests (which we run on CI) have passed: #8

vinser52

Reviewed 3 of 9 files at r4, 1 of 24 files at r5, 2 of 10 files at r6, all commit messages.
Reviewable status: 30 of 32 files reviewed, 21 unresolved discussions (waiting on @guptask, @igchor, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 584 at r6 (raw file):

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

I'm not sure if we should allow this. This could work differently depending on when you call that. Consider the following:
cfg.configureMemoryTiers(single_tier.setSize(10));
cfg.setCacheSize(11); // would change memory tier size from 10 to 11
and
cfg.setCacheSize(11);
cfg.configureMemoryTiers(single_tier.setSize(10)); // what should happen here? error? overwrite?
Right now, it will fail in both cases, which I think is OK.

Agree with @igchor

cachelib/allocator/CacheAllocatorConfig.h, line 837 at r7 (raw file):

    auto tier_ratio = tier_config.getRatio();
    if ((!tier_size and !tier_ratio) || (tier_size and tier_ratio)) {
      throw std::invalid_argument(

As I understand this loop just validates passed parameters for memory tiers. Should we move it to a separate function? And possibly call it before calling configureMemoryTiers?

cachelib/allocator/CacheAllocatorConfig.h, line 855 at r7 (raw file):

  for (auto &tier_config: config) {
    if (auto *v = std::get_if<PosixSysVSegmentOpts>(&tier_config.shmOpts)) {
      v->usePosix = usePosixShm;

Why do we have such logic in the getter method?

cachelib/allocator/CacheAllocatorConfig.h, line 864 at r7 (raw file):

    return config;

  if (!size) {

Should we validate tiers config in a separate function as I mentioned above?

cachelib/allocator/CacheAllocatorConfig.h, line 1047 at r7 (raw file):

  if (sum_ratios && sum_sizes) {
    throw  std::invalid_argument("Cannot mix ratios and sizes.");

This just proves my assumption that we need a dedicated validation function for tiers config.

cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp, line 23 at r7 (raw file):

namespace tests {

using LruAllocatorMemoryTiersTest = AllocatorMemoryTiersTest<LruAllocator>;

Should we test it with all possible eviction policies? You can refer to TestBase.h - they use typed test cases.

cachelib/allocator/tests/MemoryTiersTest.cpp, line 147 at r7 (raw file):

TEST_F(LruMemoryTiersTest, TestInvalid2TierConfigNumberOfPartitionsTooLarge) {
  EXPECT_THROW(createTestCacheConfig({defaultDaxPath, defaultPmemPath},
                                     {std::make_tuple(defaultTotalCacheSize, 0), std::make_tuple(1, 0)}).validate(),

Right now validation is done only explicitly? Did we break legacy behavior?

igchor

Reviewable status: 30 of 32 files reviewed, 21 unresolved discussions (waiting on @guptask, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 837 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

As I understand this loop just validates passed parameters for memory tiers. Should we move it to a separate function? And possibly call it before calling configureMemoryTiers?

You mean a user should call this function before configureMemoryTiers or just we should call it internally from within configureMemoryTiers?

cachelib/allocator/CacheAllocatorConfig.h, line 855 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Why do we have such logic in the getter method?

Right now, (this is because of implementations details) we keep usePosix as variable in each PosixSysV tier. Here, we just set this variables to match the one from config. We could just keep this setting as global but this would require adding a few more abstractions.

We need this usePosix from MemoryTier for example in CacheAllocator-inl.h:171

But note, that I do not modify the existing tiers, I just create a copy.

cachelib/allocator/CacheAllocatorConfig.h, line 864 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Should we validate tiers config in a separate function as I mentioned above?

Actually this particular check is not necessary, if user calls validate() method on config. I just put it here to show that rest of the code assumes size is not zeo - I can remove it.

cachelib/allocator/CacheAllocatorConfig.h, line 1047 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

This just proves my assumption that we need a dedicated validation function for tiers config.

The one in configureMemoryTiers only needs to know about MemoryTier variables (ratios and size). Here we validate for example if user provided size for the cache (by using setCacheSize) if he's using ratios. And we cannot move this anywhere else because user can call setCacheSize() before or after configureMemoryTiers. So, we have enough information to do the validation only after config is fully created.

cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp, line 23 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Should we test it with all possible eviction policies? You can refer to TestBase.h - they use typed test cases.

Yeah, I think we can extend it later. Right now, this test just checks if cache allocator throws exception if there is more than 1 tier.

cachelib/allocator/tests/MemoryTiersTest.cpp, line 147 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Right now validation is done only explicitly? Did we break legacy behavior?

No, This test was created by us anyway, and there explicit validation is used by CacheLib tests/examples already.

igchor

Reviewable status: 30 of 32 files reviewed, 21 unresolved discussions (waiting on @guptask, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 855 at r7 (raw file):

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

Right now, (this is because of implementations details) we keep usePosix as variable in each PosixSysV tier. Here, we just set this variables to match the one from config. We could just keep this setting as global but this would require adding a few more abstractions.

We need this usePosix from MemoryTier for example in CacheAllocator-inl.h:171

But note, that I do not modify the existing tiers, I just create a copy.

As for setting the size below, we need to do this here because user could have called setCacheSize() before or after memoryConfigureTiers() (so it means, we cannot do this in configureMemoryTiers)

vinser52

Reviewable status: 30 of 32 files reviewed, 21 unresolved discussions (waiting on @guptask, @igchor, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 837 at r7 (raw file):

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

You mean a user should call this function before configureMemoryTiers or just we should call it internally from within configureMemoryTiers?

As I understand, today validation happens implicitly for users. Is it possible to make it implicitly? I understand your concern about total size and per-tier size: they are mutually exclusive.

cachelib/allocator/CacheAllocatorConfig.h, line 864 at r7 (raw file):

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

Actually this particular check is not necessary, if user calls validate() method on config. I just put it here to show that rest of the code assumes size is not zeo - I can remove it.

I am worried that our changes require explicitly call validate() method.

cachelib/allocator/CacheAllocatorConfig.h, line 1047 at r7 (raw file):

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

The one in configureMemoryTiers only needs to know about MemoryTier variables (ratios and size). Here we validate for example if user provided size for the cache (by using setCacheSize) if he's using ratios. And we cannot move this anywhere else because user can call setCacheSize() before or after configureMemoryTiers. So, we have enough information to do the validation only after config is fully created.

But we can implicitly check this when user calls setCacheSize() method, isn't it?

cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp, line 23 at r7 (raw file):

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

Yeah, I think we can extend it later. Right now, this test just checks if cache allocator throws exception if there is more than 1 tier.

Could you please add TODO comment

cachelib/allocator/tests/MemoryTiersTest.cpp, line 147 at r7 (raw file):

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

No, This test was created by us anyway, and there explicit validation is used by CacheLib tests/examples already.

Disregarding this test, does implicit validation still works for the cache config?

igchor

Reviewable status: 30 of 32 files reviewed, 21 unresolved discussions (waiting on @guptask, @igchor, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 837 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

As I understand, today validation happens implicitly for users. Is it possible to make it implicitly? I understand your concern about total size and per-tier size: they are mutually exclusive.

What do you mean by implicitly? I think currently, there is both implicit and explicit validation for config. Some of the methods throw exception if you call them without satisfying some conditions (e.b. usePosixForShm()) but there is also an explicit validate() method.

cachelib/allocator/CacheAllocatorConfig.h, line 864 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

I am worried that our changes require explicitly call validate() method.

It's already being called in CacheAllocator.h

cachelib/allocator/CacheAllocatorConfig.h, line 1047 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

But we can implicitly check this when user calls setCacheSize() method, isn't it?

We could, it would just need to be called both in setCacheSize() and in conifgureMemoryTiers() (since we don't know which is called first) or we would need to require user to always call one before the other. What would you prefer?

cachelib/allocator/tests/MemoryTiersTest.cpp, line 147 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Disregarding this test, does implicit validation still works for the cache config?

Again, not sure what you mean by implicit validation.

vinser52

Reviewable status: 30 of 32 files reviewed, 21 unresolved discussions (waiting on @guptask, @igchor, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 1047 at r7 (raw file):

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

We could, it would just need to be called both in setCacheSize() and in conifgureMemoryTiers() (since we don't know which is called first) or we would need to require user to always call one before the other. What would you prefer?

Yeah, I have the first option in mind. I think it is more user-friendly, but requires some additional validation logic in these two methods.

vinser52

Reviewable status: 30 of 32 files reviewed, 21 unresolved discussions (waiting on @guptask, @igchor, @victoria-mcgrath, and @vinser52)

cachelib/allocator/tests/MemoryTiersTest.cpp, line 147 at r7 (raw file):

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

Again, not sure what you mean by implicit validation.

I mean when the user is not required to call validate method explicitly, but we will still throw an exception when user sets cache size and per-tier sizes.

vinser52

Reviewable status: 30 of 32 files reviewed, 21 unresolved discussions (waiting on @guptask, @igchor, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 837 at r7 (raw file):

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

What do you mean by implicitly? I think currently, there is both implicit and explicit validation for config. Some of the methods throw exception if you call them without satisfying some conditions (e.b. usePosixForShm()) but there is also an explicit validate() method.

I think even validate() method is not called, we need to throw an exception if cache size and per-tier sizes are set simultaneously.

igchor

Reviewable status: 30 of 32 files reviewed, 20 unresolved discussions (waiting on @guptask, @igchor, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 1047 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Yeah, I have the first option in mind. I think it is more user-friendly, but requires some additional validation logic in these two methods.

But, I forgot about one other case. What should happen when user calls configureMemoryTiers with ratios and setCacheSize was NOT called before? We cannot return error because he can call it later (this is especially true for our default value for memoryTiers which has ratio set to 1), but without validate() we have no way to check it.

I've moved the check for sum_sizes to conifgureMemoryTiers and setCacheSize but left the check for sum_ratios in validate, here are the changes: igchor@962597a. Let me know which version you prefer - you can look at the MemoryTiersTest to see which checks require validate() Or maybe you have some other ideas?

igchor

Reviewable status: 29 of 32 files reviewed, 20 unresolved discussions (waiting on @guptask, @igchor, @victoria-mcgrath, and @vinser52)

cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp, line 23 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Could you please add TODO comment

Done.

cachelib/allocator/tests/MemoryTiersTest.cpp, line 147 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

I mean when the user is not required to call validate method explicitly, but we will still throw an exception when user sets cache size and per-tier sizes.

Done.

igchor

Reviewable status: 29 of 32 files reviewed, 20 unresolved discussions (waiting on @guptask, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 837 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

I think even validate() method is not called, we need to throw an exception if cache size and per-tier sizes are set simultaneously.

Done.

vinser52

Reviewed 1 of 1 files at r8.
Reviewable status: 30 of 32 files reviewed, 16 unresolved discussions (waiting on @guptask, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 1047 at r7 (raw file):

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

But, I forgot about one other case. What should happen when user calls configureMemoryTiers with ratios and setCacheSize was NOT called before? We cannot return error because he can call it later (this is especially true for our default value for memoryTiers which has ratio set to 1), but without validate() we have no way to check it.

I've moved the check for sum_sizes to conifgureMemoryTiers and setCacheSize but left the check for sum_ratios in validate, here are the changes: igchor@962597a. Let me know which version you prefer - you can look at the MemoryTiersTest to see which checks require validate() Or maybe you have some other ideas?

Ok, I got your point. My considerations are the following:

implicit and explicit validations are not mutually exclusive
Implicitly we can detect invalid arguments only when it is set.

igchor

Reviewable status: 24 of 32 files reviewed, 16 unresolved discussions (waiting on @guptask, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 1047 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Ok, I got your point. My considerations are the following:

implicit and explicit validations are not mutually exclusive

Implicitly we can detect invalid arguments only when it is set.

Ok, I used this hybrid approach.

vinser52

Reviewable status: 24 of 32 files reviewed, 16 unresolved discussions (waiting on @guptask, @igchor, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 855 at r7 (raw file):

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

As for setting the size below, we need to do this here because user could have called setCacheSize() before or after memoryConfigureTiers() (so it means, we cannot do this in configureMemoryTiers)

Is it because we support only posix SHM today and do not support sysV?

igchor

Reviewable status: 24 of 32 files reviewed, 16 unresolved discussions (waiting on @guptask, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 855 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Is it because we support only posix SHM today and do not support sysV?

No, we support sysV as well. This is because we decided to have usePosixShm as global setting. Alternative to this, would be to require user to set posix = true/false in MemoryTierConfig. For example change MemoryTierConfig::fromShm() to MemoryTierConfig::fromShm(bool posix).

vinser52

Reviewable status: 24 of 32 files reviewed, 16 unresolved discussions (waiting on @guptask, @igchor, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 855 at r7 (raw file):

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

No, we support sysV as well. This is because we decided to have usePosixShm as global setting. Alternative to this, would be to require user to set posix = true/false in MemoryTierConfig. For example change MemoryTierConfig::fromShm() to MemoryTierConfig::fromShm(bool posix).

Now I understand the case.
But isn't better to set usePosixShm value to tier config in CacheAllocator constructor when we pass the config? It would allow to avoid creating a copy in this method. Or do I miss something?

igchor

Reviewable status: 24 of 32 files reviewed, 16 unresolved discussions (waiting on @guptask, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 855 at r7 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Now I understand the case.
But isn't better to set usePosixShm value to tier config in CacheAllocator constructor when we pass the config? It would allow to avoid creating a copy in this method. Or do I miss something?

You mean to just return the vector of memory tiers as it is and let a user (in this case, CacheAllocator) set the appropriate value for each tier? I did this inside of this method to provide better encapsulation and I think that making a copy of this vector should not be a problem.

Also. what about setting the size (the code below)? Where would it go?

to allow using new configureMemoryTiers() API with legacy behavior. Move validation code for memory tiers to validate() method and convert ratios to sizes lazily (on get)..

It wrongly assumed that the only possible segment type is PosixSysV segment.

vinser52

Reviewable status: 24 of 32 files reviewed, 15 unresolved discussions (waiting on @guptask, @victoria-mcgrath, and @vinser52)

cachelib/allocator/CacheAllocatorConfig.h, line 855 at r7 (raw file):

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

You mean to just return the vector of memory tiers as it is and let a user (in this case, CacheAllocator) set the appropriate value for each tier? I did this inside of this method to provide better encapsulation and I think that making a copy of this vector should not be a problem.

Also. what about setting the size (the code below)? Where would it go?

Now I get it.

vinser52

Reviewed 1 of 9 files at r4.
Reviewable status: 24 of 32 files reviewed, 15 unresolved discussions (waiting on @guptask, @victoria-mcgrath, and @vinser52)

vinser52

Reviewed 1 of 9 files at r4, 1 of 10 files at r6, 1 of 2 files at r7, 3 of 8 files at r10.
Reviewable status: 27 of 32 files reviewed, 15 unresolved discussions (waiting on @guptask, @victoria-mcgrath, and @vinser52)

guptask

Reviewed 5 of 8 files at r10, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @guptask and @victoria-mcgrath)

guptask

Reviewed 4 of 9 files at r4, 1 of 24 files at r5, 3 of 8 files at r10.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @guptask and @victoria-mcgrath)

Simplify BG evictor code and add possibility to wakeup it

Summary: AdRanker ASAN canary flagged a possible UBSan violation. ## Error Failed Run: https://fburl.com/servicelab/apytosry ``` #0 0x562e3adb59bc in facebook::cachelib::objcache2::ObjectCacheSizeController<facebook::cachelib::CacheAllocator<facebook::cachelib::LruCacheTrait> >::work() buck-out/v2/gen/fbcode/47d914adeee3d982/cachelib/experimental/objcache2/__object-cache__/headers/cachelib/experimental/objcache2/ObjectCacheSizeController-inl.h #1 0x562de7610f78 in facebook::cachelib::PeriodicWorker::loop() fbcode/cachelib/common/PeriodicWorker.cpp:55 #2 0x7f7632c524e4 in execute_native_thread_routine /home/engshare/third-party2/libgcc/11.x/src/gcc-11.x/x86_64-facebook-linux/libstdc++-v3/src/c++11/../../../.././libstdc++-v3/src/c++11/thread.cc:82:18 #3 0x7f7632f6ec0e in start_thread /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/nptl/pthread_create.c:434:8 #4 0x7f76330011db in clone3 /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81 UndefinedBehaviorSanitizer: integer-divide-by-zero buck-out/v2/gen/fbcode/47d914adeee3d982/cachelib/experimental/objcache2/__object-cache__/headers/cachelib/experimental/objcache2/ObjectCacheSizeController-inl.h:33:40 in ``` Reviewed By: jiayuebao Differential Revision: D39024188 fbshipit-source-id: 64ad644c360565e638fa3ca74616a315038382ab

igchor marked this pull request as draft October 29, 2021 12:59

igchor force-pushed the config_api_integration branch 2 times, most recently from bdcb286 to cc5df38 Compare November 2, 2021 15:13

guptask requested changes Nov 2, 2021

View reviewed changes

victoria-mcgrath reviewed Nov 2, 2021

View reviewed changes

igchor force-pushed the config_api_integration branch 2 times, most recently from b3942e0 to 21766be Compare November 3, 2021 12:04

igchor commented Nov 3, 2021

View reviewed changes

victoria-mcgrath reviewed Nov 3, 2021

View reviewed changes

igchor commented Nov 4, 2021

View reviewed changes

igchor marked this pull request as ready for review November 4, 2021 14:32

victoria-mcgrath reviewed Nov 4, 2021

View reviewed changes

igchor commented Nov 5, 2021

View reviewed changes

vinser52 requested changes Nov 9, 2021

View reviewed changes

igchor commented Nov 9, 2021

View reviewed changes

vinser52 requested changes Nov 9, 2021

View reviewed changes

igchor commented Nov 9, 2021

View reviewed changes

vinser52 reviewed Nov 9, 2021

View reviewed changes

vinser52 requested changes Nov 9, 2021

View reviewed changes

igchor commented Nov 9, 2021

View reviewed changes

igchor force-pushed the config_api_integration branch from 7a7cc55 to 4626805 Compare November 9, 2021 14:18

igchor commented Nov 9, 2021

View reviewed changes

vinser52 reviewed Nov 10, 2021

View reviewed changes

igchor force-pushed the config_api_integration branch from 4626805 to f401f17 Compare November 10, 2021 09:47

igchor commented Nov 10, 2021

View reviewed changes

vinser52 requested changes Nov 10, 2021

View reviewed changes

igchor commented Nov 10, 2021

View reviewed changes

vinser52 requested changes Nov 10, 2021

View reviewed changes

igchor commented Nov 10, 2021

View reviewed changes

igchor added 2 commits November 10, 2021 16:17

Add MemoryTierCacheConfig::fromShm()

6b4f46b

to allow using new configureMemoryTiers() API with legacy behavior. Move validation code for memory tiers to validate() method and convert ratios to sizes lazily (on get)..

Fix test_shm_manager.cpp test

f401f17

It wrongly assumed that the only possible segment type is PosixSysV segment.

vinser52 reviewed Nov 15, 2021

View reviewed changes

vinser52 approved these changes Nov 15, 2021

View reviewed changes

guptask approved these changes Nov 16, 2021

View reviewed changes

igchor merged commit 35cb682 into pmem:develop Nov 16, 2021

igchor referenced this pull request in igchor/CacheLib-1 Apr 4, 2022

Merge pull request #2 from igchor/simplified_bg

4fa3901

Simplify BG evictor code and add possibility to wakeup it

Config api integration #2

Config api integration #2

Conversation

igchor commented Oct 29, 2021 • edited Loading

guptask left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victoria-mcgrath Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

victoria-mcgrath left a comment

Choose a reason for hiding this comment

victoria-mcgrath left a comment

Choose a reason for hiding this comment

victoria-mcgrath left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

victoria-mcgrath commented Nov 4, 2021

victoria-mcgrath commented Nov 4, 2021

victoria-mcgrath left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

igchor commented Nov 9, 2021

vinser52 left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

vinser52 left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

vinser52 left a comment

Choose a reason for hiding this comment

vinser52 left a comment

Choose a reason for hiding this comment

vinser52 left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

vinser52 left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

vinser52 left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

vinser52 left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

vinser52 left a comment

Choose a reason for hiding this comment

igchor commented Oct 29, 2021 •

edited

Loading

victoria-mcgrath Nov 2, 2021 •

edited

Loading