Lock-free ClockCache #10390

guidotag · 2022-07-19T23:05:02Z

Summary: ClockCache completely free of locks. As part of this PR we have also pushed clock algorithm functionality out of ClockCacheShard into ClockHandleTable, so that ClockCacheShard acts more as an interface and less as an actual data structure.

Test plan:

make -j24 check
make -j24 CRASH_TEST_EXT_ARGS="--duration=960 --cache_type=clock_cache --cache_size=1073741824 --block_size=16384" blackbox_crash_test_with_atomic_flush

pdillinger · 2022-07-20T03:24:47Z

cache/clock_cache.cc

-    new_entry->InternalToExclusiveRef();
-    Assign(new_entry, h);
-    return new_entry;
+    RemoveAll(h->key(), h->hash, probe, deleted);


Can't we do this outside of holding the exclusive ref?

You may be right. I had a bad case in mind: two or more concurrent inserts (of the same key) annihilating each other. But perhaps this is not possible: the smallest insert in the probe sequence (the one whose call to FindAvailableSlot lands in the smallest probe) that is not deleted during the other concurrent FindAvailableSlot calls will survive, since RemoveAll only deletes larger items in the probe sequence.

pdillinger · 2022-07-21T17:24:37Z

cache/clock_cache.h

@@ -257,7 +246,15 @@ struct ClockHandle {
    key_data.fill(0);
  }

-  ClockHandle(const ClockHandle& other) { *this = other; }
+  // The copy constructor is only used to copy a handle for immediate


I find this confusing / troublesome, especially because now the copy ctor and assignment operator disagree. This is unusual if not undefined behavior depending on which the compiler chooses (e.g. optional copy elision).

How about a base class of ClockHandle that includes only these fields?

This was a mistake---they should agree. Apparently the push_back function in autovector requires both of them, but I don't understand why is the copy ctor needed. I'll investigate.

So, the copy ctor is required to push_back const elements. We're not using this feature, though.

anand1976

The PR looks pretty complicated. At this point, I have aa few small comments and some questions. Will try to review in more detail tomorrow.

anand1976 · 2022-07-22T00:35:39Z

cache/clock_cache.cc

  dst->SetCachePriority(src->GetCachePriority());
+  // dst->SetClockPriority(ClockHandle::ClockPriority::NONE);


Not needed?

No, I will delete it.

anand1976 · 2022-07-22T00:51:40Z

cache/clock_cache.h

-    // allows us to save many atomic operations by packing data more carefully.
-    // In particular:
+    // references are functionally equivalent to RW locks (external and internal
+    // references are read locks, and exclusive references are write locks).


Just curious, how is the distinction between internal and external refs useful? Are there any operations that are not allowed when there are external refs, but allowed if there are only internal refs?

For 2 reasons:

Internal references are short lived, but external references may not. This is helpful when acquiring an exclusive ref: if there is an external ref to the item, it's probably not worth spinning, so we move on.

In Release, when deciding whether the reference was the last one, transient references to the handle (i.e., other threads probing the slot) don't get mixed up in the ref count. (This is unlikely to happen, though, unless there are too many concurrent operations.)

I will make this a comment.

anand1976 · 2022-07-22T00:53:33Z

cache/clock_cache.h

 constexpr double kStrictLoadFactor = 0.7;

+// Maximum number of spins when trying to acquire a ref.
+constexpr uint32_t kSpinsPerTry = 100000;


What's the rationale behind this value?

This choice was arbitrary. I was tested a few values and this one performed okay. I'm adding a TODO to investigate more.

anand1976 · 2022-07-22T01:01:35Z

cache/clock_cache.cc

+  bool is_high_priority =
+      h->HasHit() || h->GetCachePriority() == Cache::Priority::HIGH;
+  h->SetClockPriority(static_cast<ClockHandle::ClockPriority>(
+      is_high_priority * ClockHandle::ClockPriority::HIGH +


Why not is_high_priority ? ClockHandle::ClockPriority::HIGH : ClockHandle::ClockPriority::MEDIUM? Might be a little easier to reason about.

I did it this way to avoid a conditional. But I guess I should trust the compiler more?

anand1976 · 2022-07-22T01:08:48Z

cache/clock_cache.h

-                                         EXCLUSIVE_REF | will_be_deleted)) {
-      if (expected & EXTERNAL_REFS) {
+                                         EXCLUSIVE_REF | will_be_deleted) &&
+           spins--) {


Maybe pause after each attempt?

I can try this.

anand1976

Overall LGTM. Some questions and comments inline. One larger question - to verify the correctness of the lock free implementation, is db_stress sufficient or do you think we need a dedicated clock_cache stress test? I'm wondering if db_stress can flush out all possible race conditions since each operation would do a lot more than cache access.

anand1976 · 2022-07-22T21:11:08Z

cache/clock_cache.cc

+      key,
+      [&](ClockHandle* h) {
+        if (h->TryInternalRef()) {
+          if (h->IsElement() && h->Matches(key, hash)) {


I know it wasn't introduced in this PR, but IsElement() is not very clear. Maybe something like IsValid() would have been more appropriate.

A handle can be 3 different things: a KV element, a tombstone or an empty slot. Tombstones and empty slots are not really invalid.

Maybe IsOccupied()? I don't love this term either because I think of tombstones as occupying slots too.

anand1976 · 2022-07-22T21:12:53Z

cache/clock_cache.cc

+        }
+        return false;
+      },
+      [&](ClockHandle* h) { return h->displacements == 0; },


Is it possible to come up with a intuitively named function, instead of h->displacements == 0? I don't have any good suggestions though.

NothingAfterThisProbe()?

anand1976 · 2022-07-22T21:25:59Z

cache/clock_cache.h

 // These properties induce 4 different states, with transitions defined as
 // follows:
 // - Not M --> M: When a handle is deleted or replaced by a new version, but
 //    not immediately evicted.
 // - M --> not M: This cannot happen. Once a handle is marked for deletion,
-//    there is no can't go back.
+//    there is no way back.


If possible, it'd be good to document the transitions in more detail, including the triggers for transition. For example, what would cause a handle to be marked for deletion but not immediately evicted, transition from internal to external ref or vice versa.

I will work on improving this part of the documentation.

anand1976 · 2022-07-22T21:33:11Z

cache/clock_cache.h

+  // ------------------------------------
+  //
+  // We separate data members that are updated frequently from the ones that
+  // are not frequently updated so that they don't share the same cache line


How is cacheline separation being ensured? Also, shouldn't each frequently modified data member be on a separate cacheline? And array_ is not frequently modified I believe?

To be honest, I ported this from LRUCache without giving it much thought, so thank you for asking this question. I think we want to avoid reads on some field X to trigger the cache coherence mechanism only because it shares the cache line with other modified field Y, not because the operation reading X actually needs to use Y. In our case: Lookup uses array_; Release and Erase use array_, occupancy_ and usage_; and Insert uses array_, occupancy_, usage_ and clock_pointer_. So I think we want 3 different cache lines: array_; occupancy_ and usage_; and clock_pointer_.

As to how is cache line separation ensured: I think we should be aligning the fields.

anand1976 · 2022-07-22T21:39:46Z

cache/clock_cache.cc

+
+    if (h->TryExclusiveRef()) {
+      if (h->WillBeDeleted()) {
+        Remove(h, deleted);


Remove() will update charge_ and occupancy_. If multiple insertions are happening in parallel, it could result in those cache lines thrashing. Would it be better to evict as many as needed and update in one shot?

Yes. I'll implement this. clock_pointer_ is also contended among all inserts doing eviction, but it's trickier to fix, because with variable-size blocks we don't know right off the bat how many elements we will need to evict.

An alternative approach is to have a single thread on every shard periodically run the eviction algorithm. This could be future optimization work.

I fixed all of this, including the clock_pointer_ thing.

guidotag · 2022-07-23T07:56:45Z

Overall LGTM. Some questions and comments inline. One larger question - to verify the correctness of the lock free implementation, is db_stress sufficient or do you think we need a dedicated clock_cache stress test? I'm wondering if db_stress can flush out all possible race conditions since each operation would do a lot more than cache access.

I don't know the details of db_stress, but I believe it only exercises steady state. This particular steady state may not be really risky for our lock-free algorithm. For example, I would like to stress it with multiple concurrent lookup/insert/delete on the same key. I don't know if this is tested in db_stress.

Also, it'd be good to have more unit tests for adversarial cases that are not exercised in the current battery of tests, which are mostly single thread.

anand1976

SGTM. I'll leave it up to you to implement further optimizations either in this PR or a follow-on PR.

facebook-github-bot · 2022-07-25T00:46:40Z

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

facebook-github-bot · 2022-07-25T00:53:13Z

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

facebook-github-bot · 2022-07-25T00:54:06Z

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

Guido Tagliavini Ponce added 2 commits July 18, 2022 23:55

First commit.

8835baf

Update.

36b37d1

guidotag requested a review from pdillinger July 19, 2022 23:05

facebook-github-bot added the CLA Signed label Jul 19, 2022

pdillinger reviewed Jul 20, 2022

View reviewed changes

Guido Tagliavini Ponce added 3 commits July 19, 2022 20:37

Fix a couple of bugs. Fix comments.

dac7485

Format.

99a2751

Comments.

a0771d1

guidotag requested a review from anand1976 July 20, 2022 15:58

Guido Tagliavini Ponce added 5 commits July 20, 2022 09:08

Update comment.

a61ce02

Bug fix.

6f98e89

Copy constructor only copies relevant fields.

22ae64b

Comments.

b22d077

Format.

2869533

pdillinger reviewed Jul 21, 2022

View reviewed changes

Guido Tagliavini Ponce added 2 commits July 21, 2022 11:34

Fix copy ctor and assignment.

66cb016

Format.

2cf7fda

anand1976 reviewed Jul 22, 2022

View reviewed changes

Guido Tagliavini Ponce added 4 commits July 22, 2022 10:02

PR comments.

a314942

Comments.

c477cbb

Cosmetic improvements.

08de250

Format.

3576f36

guidotag mentioned this pull request Jul 22, 2022

Lock-Free Clock Cache #10306

Open

17 tasks

anand1976 reviewed Jul 22, 2022

View reviewed changes

anand1976 approved these changes Jul 23, 2022

View reviewed changes

Guido Tagliavini Ponce added 4 commits July 23, 2022 18:09

Reduce cache line contention.

4f21ee9

Format.

742d1b1

Cast.

6674586

Bug fixes. Improve documentation.

f6d5805

Guido Tagliavini Ponce added 4 commits July 24, 2022 15:29

Documentation.

542fb12

Documentation.

bf719b6

Merge remote-tracking branch 'upstream/main' into clock-cache-6

8ad437a

Function comment.

e702cdb

Revert changes to cache_test.cc.

4795625

facebook-github-bot closed this in 6a160e1 Jul 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lock-free ClockCache #10390

Lock-free ClockCache #10390

guidotag commented Jul 19, 2022 •

edited

Loading

pdillinger Jul 20, 2022

guidotag Jul 20, 2022 •

edited

Loading

pdillinger Jul 21, 2022

guidotag Jul 21, 2022

guidotag Jul 21, 2022

anand1976 left a comment

anand1976 Jul 22, 2022

guidotag Jul 22, 2022

anand1976 Jul 22, 2022

guidotag Jul 22, 2022

guidotag Jul 22, 2022

anand1976 Jul 22, 2022

guidotag Jul 22, 2022

anand1976 Jul 22, 2022

guidotag Jul 22, 2022

anand1976 Jul 22, 2022

guidotag Jul 22, 2022

anand1976 left a comment

anand1976 Jul 22, 2022

guidotag Jul 22, 2022

anand1976 Jul 22, 2022

guidotag Jul 22, 2022

anand1976 Jul 22, 2022

guidotag Jul 22, 2022

anand1976 Jul 22, 2022

guidotag Jul 23, 2022 •

edited

Loading

anand1976 Jul 22, 2022

guidotag Jul 23, 2022 •

edited

Loading

guidotag Jul 24, 2022

guidotag commented Jul 23, 2022 •

edited

Loading

anand1976 left a comment

facebook-github-bot commented Jul 25, 2022

facebook-github-bot commented Jul 25, 2022

facebook-github-bot commented Jul 25, 2022

		dst->SetCachePriority(src->GetCachePriority());
		// dst->SetClockPriority(ClockHandle::ClockPriority::NONE);

Lock-free ClockCache #10390

Lock-free ClockCache #10390

Conversation

guidotag commented Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

guidotag Jul 20, 2022 • 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

anand1976 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

anand1976 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

guidotag Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guidotag Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guidotag commented Jul 23, 2022 • edited Loading

anand1976 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 25, 2022

facebook-github-bot commented Jul 25, 2022

facebook-github-bot commented Jul 25, 2022

guidotag commented Jul 19, 2022 •

edited

Loading

guidotag Jul 20, 2022 •

edited

Loading

guidotag Jul 23, 2022 •

edited

Loading

guidotag Jul 23, 2022 •

edited

Loading

guidotag commented Jul 23, 2022 •

edited

Loading