Add combined locking support for MMContainer #182

igchor · 2022-12-13T21:08:52Z

through withEvictionIterator function.

Also, expose the config option to enable and disable combined locking.
withEvictionIterator is implemented as an extra function, getEvictionIterator() is still there and it's behavior hasn't changed.

This is a subset of changes from: #172

haowu14 · 2022-12-13T22:42:33Z

I was reading your comment in #172 (comment). In single-tier version, without the further change, would using combined_locking for iterator cause reduction in eviction success rate and why?

haowu14

Overall this looks good. Once you address the qestions I will import it for internal review.

haowu14 · 2022-12-13T22:49:27Z

cachelib/allocator/MM2Q.h

@@ -68,6 +68,7 @@ class MM2Q {
  enum LruType { Warm, WarmTail, Hot, Cold, ColdTail, NumTypes };

  // Config class for MM2Q
+  // TODO: implement support for useCombinedLockForIterators


Given the code you have, implementing this for 2Q is just adding the if statement you have for MMLRU right?

Adding the if statement + extending the MM2Q config

Done. I just realized that TinyLFU uses a spin lock, which does not support combined locking, so there is no option to enable it.

haowu14 · 2022-12-13T22:52:55Z

cachelib/allocator/tests/MMTypeTest.h

@@ -343,5 +348,26 @@ size_t MMTypeTest<MMType>::getListSize(const Container& c,
  }
  throw std::invalid_argument("LruType not existing");
 }
+
+template <typename MMType>
+void MMTypeTest<MMType>::verifyIterationVariants(Container& c) {


Please add a comment on this function.

igchor · 2022-12-14T02:06:51Z

I was reading your comment in #172 (comment). In single-tier version, without the further change, would using combined_locking for iterator cause reduction in eviction success rate and why?

There are two issues with using withEvictionIterator with current design:

Current implementation of findEviction makes it pretty tricky to use withEvictionIterator - the iterator is passed from one function to another.
The length of the critical section (under MMContainer lock) inside findEviction is pretty big. Combined locking makes the p99 latency very high (especially for readers).

The second issue is actually what started our efforts to optimize findEviction and shorten the Critical Section. We observed a very big performance drop in our initial multi-tier implementation (that followed the current findEviction logic). Our first attempt to shorten the Critical Section was this PR: #132 where we just wanted to mark the item as exclusive under the lock and do all other operations outside. The problem there was that the eviction success rate suffered leading us to #172 where performance is much better (for both single and multi-tier).

Lower eviction rate in PR 132 was due to higher chance of collision: after we marked the item and exited the critical section, someone else could have increased the ref count on that item causing us to fail. In the new design, this cannot happen because after item is marked, it's inaccessible by others.

facebook-github-bot · 2022-12-14T17:50:27Z

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

haowu14 · 2022-12-14T22:54:25Z

cachelib/allocator/MMLru.h

@@ -195,6 +200,9 @@ class MMLru {
    //     lruInsertionPointSpec = 2, we insert at a point 1/4th from tail
    uint8_t lruInsertionPointSpec{0};

+    // Whether to use combined locking for withEvictionIterator.


Move this after mmReconfigureIntervalSecs. Meta internal build failed on the initialization order.

facebook-github-bot · 2022-12-15T17:38:45Z

@igchor has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-12-15T17:42:08Z

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

haowu14 · 2022-12-15T18:36:35Z

cachelib/allocator/MM2Q.h

@@ -213,7 +216,8 @@ class MM2Q {
           bool rebalanceOnRecordAccs,
           size_t hotPercent,
           size_t coldPercent,
-           uint32_t mmReconfigureInterval)
+           uint32_t mmReconfigureInterval,
+           bool useCombinedLockForIterators)


Please make this optional.
The way we typically do is create a new constructor with all the parameters while keeping the constructor of Config(uint32_t time, double ratio, bool updateOnW, bool updateOnR, bool tryLockU, bool rebalanceOnRecordAccs, size_t hotPercent, size_t coldPercent, uint32_t mmReconfigureInterval)
And have it call the constructor with all the parameters with default value.

Sure, done.

haowu14 · 2022-12-15T18:38:22Z

cachelib/allocator/tests/MMTypeTest.h

+  }
+
+  size_t i = 0;
+  c.withEvictionIterator([&c, &nodes, &i](auto&& iter) {


Don't have to capture c here since it is not used inside.

facebook-github-bot · 2022-12-15T19:22:50Z

@igchor has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-12-15T23:12:57Z

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

through withEvictionIterator function. Also, expose config option to enable and disable combined locking. withEvictionIterator is implemented as en extra function, getEvictionIterator() is still there and it's behavior hasn't changed.

therealgymmy · 2023-01-12T19:03:18Z

cachelib/allocator/MMTinyLFU.h

@@ -456,6 +444,40 @@ class MMTinyLFU {
      // Tiny and main cache iterators
      ListIterator tIter_;
      ListIterator mIter_;
+    };
+
+    class LockedIterator : public Iterator {


@igchor: is this necessary? TinyLFU doesn't support combined locking. It seems like we can just rename Iterator to LockIterator and avoid introducing this new code here

Yes, you are right, I forgot to revert these changes after realizing TinyLFU does not support combined locking. Done.

facebook-github-bot · 2023-01-12T20:24:42Z

@igchor has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-01-13T17:24:09Z

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

facebook-github-bot · 2023-02-02T18:49:46Z

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

facebook-github-bot · 2023-02-03T04:56:14Z

@haowu14 merged this pull request in d5b5bea.

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 Dec 13, 2022

igchor mentioned this pull request Dec 13, 2022

Critical section part 2: findEviction optimization and code unification #172

Open

igchor force-pushed the combined_locking branch from f507394 to 212094a Compare December 13, 2022 21:33

haowu14 requested changes Dec 13, 2022

View reviewed changes

igchor force-pushed the combined_locking branch from 212094a to 8b6acff Compare December 14, 2022 17:47

haowu14 requested changes Dec 14, 2022

View reviewed changes

igchor force-pushed the combined_locking branch from 8b6acff to 5ade8c0 Compare December 15, 2022 17:38

haowu14 requested changes Dec 15, 2022

View reviewed changes

igchor force-pushed the combined_locking branch from 5ade8c0 to 0e9cbdf Compare December 15, 2022 19:22

Add combined locking support for MMContainer

3ad50fa

through withEvictionIterator function. Also, expose config option to enable and disable combined locking. withEvictionIterator is implemented as en extra function, getEvictionIterator() is still there and it's behavior hasn't changed.

therealgymmy reviewed Jan 12, 2023

View reviewed changes

igchor force-pushed the combined_locking branch from 0e9cbdf to 3ad50fa Compare January 12, 2023 20:24

byrnedj mentioned this pull request Feb 2, 2023

Rebased develop with NUMA bindings and proposed upstream PRs intel/CacheLib#56

Closed

facebook-github-bot closed this in d5b5bea Feb 3, 2023

facebook-github-bot added the Merged label Feb 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add combined locking support for MMContainer #182

Add combined locking support for MMContainer #182

igchor commented Dec 13, 2022

haowu14 commented Dec 13, 2022

haowu14 left a comment

haowu14 Dec 13, 2022

igchor Dec 14, 2022

igchor Dec 14, 2022

haowu14 Dec 13, 2022

igchor Dec 14, 2022

igchor commented Dec 14, 2022

facebook-github-bot commented Dec 14, 2022

haowu14 Dec 14, 2022

igchor Dec 15, 2022

facebook-github-bot commented Dec 15, 2022

facebook-github-bot commented Dec 15, 2022

haowu14 Dec 15, 2022

igchor Dec 15, 2022

haowu14 Dec 15, 2022

igchor Dec 15, 2022

facebook-github-bot commented Dec 15, 2022

facebook-github-bot commented Dec 15, 2022

therealgymmy Jan 12, 2023

igchor Jan 12, 2023

facebook-github-bot commented Jan 12, 2023

facebook-github-bot commented Jan 13, 2023

facebook-github-bot commented Feb 2, 2023

facebook-github-bot commented Feb 3, 2023

Add combined locking support for MMContainer #182

Add combined locking support for MMContainer #182

Conversation

igchor commented Dec 13, 2022

haowu14 commented Dec 13, 2022

haowu14 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

igchor commented Dec 14, 2022

facebook-github-bot commented Dec 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 15, 2022

facebook-github-bot commented Dec 15, 2022

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

facebook-github-bot commented Dec 15, 2022

facebook-github-bot commented Dec 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 12, 2023

facebook-github-bot commented Jan 13, 2023

facebook-github-bot commented Feb 2, 2023

facebook-github-bot commented Feb 3, 2023