Fix ReaperSkippingSlabTraversalWhileSlabReleasing test

[vinser52]

This change is 

[igchor]

Please add some description to the commit. What was the problem and how does this fix it?

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

---

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

    TierId tid, PoolId pid, Item& item) {
  if(item.isChainedItem()) return {}; // TODO: We do not support ChainedItem yet
  if(item.isExpired()) {

nit: you could use removeIfExpired (if you can get a handle first or change the predicate).

[igchor]

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

[vinser52]

Done

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

---

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

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

nit: you could use removeIfExpired (if you can get a handle first or change the predicate).

I saw removeIfExpired() function. But I do not want to use it because tryEvictToNextMemoryTier() function can be called on the hot path. And removeIfExpired() function calls accessContainer_->removeIf() with predicate to check if item is expired. Internally it grabs the bucket lock. I think it is too expensive on the hot path.
Furthermore, to call removeIfExpired() function we need to acquire a handle in that particular case. And think it is also might have some overhead because of atomic operation internally. All this action only need if the item is really expired - I believe, more frequently, item.isExpired() is false on this path.

But I have implemented another refactoring. When I did rebase of develop branch I blamed such refactoring... But now doing it by myself.
Less code we touch - less merge conflicts we have during rebase

[igchor]

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @byrnedj, @igchor, and @vinser52)

---

run_tests.sh line 4 at r3 (raw file):

# Newline separated list of tests to ignore
BLACKLIST="allocator-test-AllocatorTypeTest

why did you remove that?

[vinser52]

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

---

run_tests.sh line 4 at r3 (raw file):

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

why did you remove that?

I would ask why we have it in the black list. As I remember we created this blacklist to disable tests that are failed even on mainline. But bow this test are passed on mainline.
The issue fixed by this PR was introduced by me when I implemented transparent async Item movement. And if that test was enabled we would catch the issue during initial PR review.

[byrnedj]

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @byrnedj, @igchor, and @vinser52)

run_tests.sh line 4 at r3 (raw file):

# Newline separated list of tests to ignore
BLACKLIST="allocator-test-AllocatorTypeTest

why did you remove that?

Because this is the test suite that was failing.

I agree with the logic behind refactoring removeIf - and it does effect item reaper code path so we may have merge conflicts later with upstream.

I can compile and pass all tests.

[vinser52]

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

---

run_tests.sh line 4 at r3 (raw file):

Because this is the test suite that was failing.

Yeah, I remember it. My point was that we should remove this from blacklist earlier (when test suite was fixed by Meta)

[vinser52]

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

---

run_tests.sh line 4 at r3 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Because this is the test suite that was failing.

Yeah, I remember it. My point was that we should remove this from blacklist earlier (when test suite was fixed by Meta)

Which merge conflicts did you mean?