-
Notifications
You must be signed in to change notification settings - Fork 4
correct handling for expired items in eviction #86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @byrnedj)
cachelib/allocator/CacheAllocator-inl.h
line 1811 at r1 (raw file):
TierId tid, PoolId pid, Item& item, bool fromBgThread) { XDCHECK(item.isMoving()); XDCHECK(!item.isExpired());
I think the item can still become expired after we exit the MMContainer critical section - we should remove this assert. From a correctness perspective I think that's fine (it can become expired anytime I think), we can just evict the item and find() will handle it properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @igchor)
cachelib/allocator/CacheAllocator-inl.h
line 1811 at r1 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
I think the item can still become expired after we exit the MMContainer critical section - we should remove this assert. From a correctness perspective I think that's fine (it can become expired anytime I think), we can just evict the item and find() will handle it properly.
Done.
- we first check if an item is expired under mmContainer lock and if so mark it for eviction so it is recycled back up to allocateInternalTier.
- we first check if an item is expired under mmContainer lock and if so mark it for eviction so it is recycled back up to allocateInternalTier.
- we first check if an item is expired under mmContainer lock and if so mark it for eviction so it is recycled back up to allocateInternalTier.
- we first check if an item is expired under mmContainer lock and if so mark it for eviction so it is recycled back up to allocateInternalTier.
- we first check if an item is expired under mmContainer lock and if so mark it for eviction so it is recycled back up to allocateInternalTier.
Part 2. ---------------------------- This patch introduces tryEvictToNextMemoryTier and some additional multi-tier tests. We can consider merging tryEvictToNextMemoryTier with the initial implementation and seperating the tests into a seperate patch. Per tier pool stats (multi-tier patch part 3.) -------------------- This introduces per tier stats this can go with multi-tier patch part 2. Fix token creation and stats (#79) (multi-tier patch 4.) --------------------------------- This patch can go after we implement tryEvictToNextMemoryTier (or multi-tier part 2) and should be combined as such. * Fix issue with token creation * Do not increment evictFail* stats if evictFailConcurrentFill were incremented correct handling for expired items in eviction (#86) (multi-tier patch 5.) ----------------------------------------------------- This can be merged with patches that fix token creation and probably squashed into multi-tier patch 2. - we first check if an item is expired under mmContainer lock and if so mark it for eviction so it is recycled back up to allocateInternalTier.
Part 2. ---------------------------- This patch introduces tryEvictToNextMemoryTier and some additional multi-tier tests. We can consider merging tryEvictToNextMemoryTier with the initial implementation and seperating the tests into a seperate patch. Per tier pool stats (multi-tier patch part 3.) -------------------- This introduces per tier stats this can go with multi-tier patch part 2. Fix token creation and stats (#79) (multi-tier patch 4.) --------------------------------- This patch can go after we implement tryEvictToNextMemoryTier (or multi-tier part 2) and should be combined as such. * Fix issue with token creation * Do not increment evictFail* stats if evictFailConcurrentFill were incremented correct handling for expired items in eviction (#86) (multi-tier patch 5.) ----------------------------------------------------- This can be merged with patches that fix token creation and probably squashed into multi-tier patch 2. - we first check if an item is expired under mmContainer lock and if so mark it for eviction so it is recycled back up to allocateInternalTier.
Part 2. ---------------------------- This patch introduces tryEvictToNextMemoryTier and some additional multi-tier tests. We can consider merging tryEvictToNextMemoryTier with the initial implementation and seperating the tests into a seperate patch. Per tier pool stats (multi-tier patch part 3.) -------------------- This introduces per tier stats this can go with multi-tier patch part 2. Fix token creation and stats (#79) (multi-tier patch 4.) --------------------------------- This patch can go after we implement tryEvictToNextMemoryTier (or multi-tier part 2) and should be combined as such. * Fix issue with token creation * Do not increment evictFail* stats if evictFailConcurrentFill were incremented correct handling for expired items in eviction (#86) (multi-tier patch 5.) ----------------------------------------------------- This can be merged with patches that fix token creation and probably squashed into multi-tier patch 2. - we first check if an item is expired under mmContainer lock and if so mark it for eviction so it is recycled back up to allocateInternalTier.
We first check if an item is expired under mmContainer lock and if so mark it for eviction so it is recycled back up to allocateInternalTier.
This change is