[upstream] transparent Item movement #92
Conversation
vinser52
force-pushed
the
upstream_transparent_move
branch
from
e33e0fb to
14b6829
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @byrnedj and @igchor)
cachelib/common/Mutex.h line 344 at r1 (raw file):
using ReadLockHolder = ReadLockHolderType; using WriteLockHolder = WriteLockHolderType; using LockHolder = std::unique_lock<Lock>;
It looks like a workaround because WriteLockHolderType does not support try_lock.
| // we rely on moving flag being set (it should block all readers) | ||
| XDCHECK_EQ(item.getRefCount(),0); | ||
| XDCHECK(item.isMoving()); | ||
| return item.isMoving(); |
There was a problem hiding this comment.
we can probably just return true here, right? no need to check the isMoving in Release
There was a problem hiding this comment.
Yeah. My strategy was to extract code from develop as is and use this PR to discuss excessive XDCHECK. I believe Daniel added them when investigated data race issues.
In that particular case there are two possible simplifications:
- remove checks and just
return item.isMoving(); - hardcode to always return
trueand keep checks.
|
|
||
| size_t wakeUpWaitersLocked(folly::StringPiece key, WriteHandle&& handle); | ||
|
|
||
| class MoveCtx { |
There was a problem hiding this comment.
nit: This class can probably be implemented in CacheALlocator-inl.h
| XDCHECK(item->isChainedItem() | ||
| ? item->asChainedItem().getParentItem(compressor_).isMoving() | ||
| : item->isMoving()) << item->toString() << "\n" << syncItem->toString(); | ||
| if ( ( item->isChainedItem() && |
There was a problem hiding this comment.
I think just XDCHECK(syncItem->isMoving()); is enough. All other checks are just duplications.
| XDCHECK(oldItem.isChainedItem() && oldItem.getRefCount() == 1); | ||
| XDCHECK_EQ(0, parentItem->getRefCount()); | ||
| newItemHdl = | ||
| allocateChainedItemInternal(*parentItem, oldItem.getSize()); |
There was a problem hiding this comment.
I think we need to call allocateNewItemForOldItem function for chained items as well. And modify allocateNewItemForOldItem function to work correctly with chained items.
vinser52
force-pushed
the
upstream_transparent_move
branch
2 times, most recently
from
dcce4be to
955fbe7
Compare
vinser52
force-pushed
the
upstream_transparent_move
branch
from
3824795 to
ba3f5b5
Compare
| @@ -4841,7 +4841,7 @@ class BaseAllocatorTest : public AllocatorTest<AllocatorT> { | |||
|
|
|||
| std::memcpy(newItem.getMemory(), oldItem.getMemory(), oldItem.getSize()); | |||
| ++numMoves; | |||
| }, {}, 1000000 /* lots of moving tries */); | |||
There was a problem hiding this comment.
This change just returns the test to the initial state (like in main branch).
@byrnedj increased the number of tries, but after various fixes looks like it is not required anymore.
vinser52
force-pushed
the
upstream_transparent_move
branch
5 times, most recently
from
c0ecd6d to
ab70b7d
Compare
vinser52
force-pushed
the
upstream_transparent_move
branch
3 times, most recently
from
635eb22 to
bada673
Compare
The new algorithm relies on the moving bit and does not require external synchronization. Data movement happens transparently for the client: if the client thread attempts to get a handle for the item being moved it will get a handle with wait context to wait till the movement is completed.
vinser52
force-pushed
the
upstream_transparent_move
branch
from
bada673 to
807dbda
Compare
vinser52
force-pushed
the
upstream_transparent_move
branch
from
807dbda to
3d73cfe
Compare
Use findInternal instead of acquire
I was able to extract code changes related to transparent data movements.
No testing was done yet. I just make sure that the code is compiled.
I extracted code almost as-is from develop branch. I will use this PR to discuss possible refactoring - I will put comments to the appropriate code lines.
This change is