Fix moveRegularItemWithSync and add tests

[vinser52]

This change is 

[igchor]

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

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 128 at r1 (raw file):

  }

  void testMultiTiersRemoveDuringEviction() {

nit: you could probably make a generic function which would accept std::function F as a param and call F inside moveCB instead of copy-pasting the initialization phase.

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 150 at r1 (raw file):

      t = std::make_unique<std::thread>([&](){ alloc->remove(key); });
      // sleep to make sure async thread calls remove(key)
      std::this_thread::sleep_for(std::chrono::milliseconds(100));

nit: instead of this sleep you could probably use latches quite easily to make this deterministic: https://github.com/facebook/folly/blob/main/folly/synchronization/Latch.h

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 200 at r1 (raw file):

            });
        // sleep to make sure async thread calls remove(key)
        std::this_thread::sleep_for(std::chrono::milliseconds(100));

nit: similarly as above + comment is wrong (should be insertOrReplace)

[vinser52]

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

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 128 at r1 (raw file):

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

nit: you could probably make a generic function which would accept std::function F as a param and call F inside moveCB instead of copy-pasting the initialization phase.

Done

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 150 at r1 (raw file):

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

nit: instead of this sleep you could probably use latches quite easily to make this deterministic: https://github.com/facebook/folly/blob/main/folly/synchronization/Latch.h

Done

---

cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 200 at r1 (raw file):

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

nit: similarly as above + comment is wrong (should be insertOrReplace)

Done

[igchor]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @byrnedj and @vinser52)

---

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

      auto key = oldItem.getKey();
      t = std::make_unique<std::thread>([&](){
            latch.count_down();

shouldnt this be after the remove? or are you trying to interleave the execution of remove with memcpy?
if this you just want to execute the remove and wait for completion then you could probably even just call join immediately, right?

[vinser52]

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

---

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

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

shouldnt this be after the remove? or are you trying to interleave the execution of remove with memcpy?
if this you just want to execute the remove and wait for completion then you could probably even just call join immediately, right?

Remove call is blocked (on wait context) till async move is completed. So that we cannot call latch.count_down() after remove because it will cause deadlock.

[vinser52]

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

---

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

Previously, vinser52 (Sergei Vinogradov) wrote…

Remove call is blocked (on wait context) till async move is completed. So that we cannot call latch.count_down() after remove because it will cause deadlock.

I understand that ideally latch.count_down() should be called when the thread hanged on the wait context, but it is not possible to implement in the test.

[vinser52]

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

---

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

Previously, vinser52 (Sergei Vinogradov) wrote…

I understand that ideally latch.count_down() should be called when the thread hanged on the wait context, but it is not possible to implement in the test.

I have added comments as we discussed in the chat