fix: deduplicate mget response #4175
Conversation
romange
changed the title
romange
force-pushed
the
DeduplicateMgt
branch
from
35fd883 to
0dbbee2
Compare
src/server/transaction.cc
Outdated
| @@ -1104,10 +1104,13 @@ bool Transaction::ScheduleInShard(EngineShard* shard, bool execute_optimistic) { | |||
|
|
|||
| // We need to acquire the fp locks because the executing callback | |||
| // within RunCallback below might preempt. | |||
| bool keys_unlocked = GetDbSlice(shard->shard_id()).Acquire(mode, lock_args); | |||
| auto [keys_unlocked, has_duplicates] = GetDbSlice(shard->shard_id()).Acquire(mode, lock_args); | |||
There was a problem hiding this comment.
when running dragonfly with lock on hash tag will this always be true for multi keys?
if so do you see any downside?
There was a problem hiding this comment.
I think we are missing some tests that check the response for mget for the same key
lets add this test before merging this pr
There was a problem hiding this comment.
good catch, i did not think of it. yes, there is a downside
There was a problem hiding this comment.
I think I am gonna drop the transaction optimization for now, too bad it does not work.
In case of duplicate mget keys, skips fetching the same key twice. We add transactional flag DUPLICATE_KEYS_HINT that tells us for free whether shard has duplicate keys. The optimization is straighforward - we just copy the response for the original key, since the response is a shallow object, we potentially save lots of memory with this deduplication. Signed-off-by: Roman Gershman <[email protected]>
Always deduplicate inside OpMGet. Signed-off-by: Roman Gershman <[email protected]>
romange
force-pushed
the
DeduplicateMgt
branch
from
0dbbee2 to
4bfe929
Compare
|
@romange please add unit test for checking the output of mget with same key |
|
@adiholden MGetCachingModeBug2465 that you added already covers it. |
As I wrote above we do not check the mget response, we just run the mget command and check the stats |
src/server/string_family.cc
Outdated
| absl::InlinedVector<Item, 32> items(keys.Size()); | ||
| static thread_local absl::flat_hash_map<string_view, unsigned> key_index; | ||
|
|
||
| FiberAtomicGuard guard; |
There was a problem hiding this comment.
I dont think this is not preemptive
If you run in cache mode and do bumpups for example you will need to serialize the bucket
There was a problem hiding this comment.
then we can not use thread_local.
| @@ -596,6 +623,7 @@ MGetResponse OpMGet(util::fb2::BlockingCounter wait_bc, uint8_t fetch_mask, cons | |||
| } | |||
| } | |||
| } | |||
| key_index.clear(); | |||
There was a problem hiding this comment.
ok this is redundant now
In case of duplicate mget keys, skips fetching the same key twice.
We add transactional flag DUPLICATE_KEYS_HINT that tells us for free
whether shard has duplicate keys.
The optimization is straighforward - we just copy the response for the original key,
since the response is a shallow object, we potentially save lots of memory with this
deduplication.