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
Add defragment support for HFE #13229
Conversation
src/ebuckets.c
Outdated
@@ -1780,6 +1780,59 @@ void ebValidate(ebuckets eb, EbucketsType *type) { | |||
ebValidateRax(ebGetRaxPtr(eb), type); | |||
} | |||
|
|||
eItem ebDefragItem(ebuckets *eb, EbucketsType *type, eItem item, ebDefragFunction *fn) { |
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.
Add assert and verify all the calls to this function that the ExpireMeta of the item is not trash.
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.
hfieldGetExpireTime() has ensured that the trash is not 1.
the caller is responsible for ensuring the item has TTL via hfieldGetExpireTime()
, otherwise, all the calls to this function will be dangerous.
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.
I added comment (include note) for this function.
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.
after struggling, i decide to add the assertion.
src/defrag.c
Outdated
if (unlikely(ob->type == OBJ_HASH && hashTypeHasExpireField(ob))) { | ||
/* Update its reference in the ebucket while defragging it. */ | ||
newob = ebDefragItem(&db->hexpires, &hashExpireBucketsType, ob, (ebDefragFunction *)activeDefragStringOb); |
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.
The function should renamed to hashTypeHasExpireMeta
or hashTypeExpireSupport
. It doesn't necessarily gurantee that it has fields with expiry. Only tells that it support expiry.
We need to verify either inside ebDefragItem
or before the call that ExpireMeta of the hash is not Trash
.
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.
I think we can discard entire function hashTypeHasExpireMeta()
and use instead existing hashTypeGetMinExpire()
against EB_EXPIRE_TIME_INVALID
.
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.
good idea, i choose 2
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.
done with 5bf9ddb
(#13229).
Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
Co-authored-by: Moti Cohen <moti.cohen@redis.com>
@@ -204,7 +204,7 @@ start_server {tags {"external:skip needs:debug"}} { | |||
} | |||
|
|||
# Wait for active expire | |||
wait_for_condition 50 20 { [r EXISTS same40] == 0 } else { fail "hash `same40` should be expired" } | |||
wait_for_condition 500 20 { [r EXISTS same40] == 0 } else { fail "hash `same40` should be expired" } |
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.
src/ebuckets.c
Outdated
@@ -1780,6 +1780,59 @@ void ebValidate(ebuckets eb, EbucketsType *type) { | |||
ebValidateRax(ebGetRaxPtr(eb), type); | |||
} | |||
|
|||
eItem ebDefragItem(ebuckets *eb, EbucketsType *type, eItem item, ebDefragFunction *fn) { |
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.
I added comment (include note) for this function.
src/defrag.c
Outdated
if (unlikely(ob->type == OBJ_HASH && hashTypeHasExpireField(ob))) { | ||
/* Update its reference in the ebucket while defragging it. */ | ||
newob = ebDefragItem(&db->hexpires, &hashExpireBucketsType, ob, (ebDefragFunction *)activeDefragStringOb); |
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.
done with 5bf9ddb
(#13229).
src/ebuckets.c
Outdated
@@ -1780,6 +1780,59 @@ void ebValidate(ebuckets eb, EbucketsType *type) { | |||
ebValidateRax(ebGetRaxPtr(eb), type); | |||
} | |||
|
|||
eItem ebDefragItem(ebuckets *eb, EbucketsType *type, eItem item, ebDefragFunction *fn) { |
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.
after struggling, i decide to add the assertion.
I see the comment that you'll add test cases for eblist and listpack later. I didn't do deep bug hunting but overall, PR seems good to me. |
@tezc please have a look, make the test cover the listpackex and eblist. |
@tezc because the defragment for ebucket doesn't complete, i increase the threhold to make the test stable, now the test is just for ensuring the correctly updating of references. |
Background
So when we defrag the hash object or the field in a dict with HFE, we also need to update the references in them.
Interface
ebDefragItem
, which can accept a defrag callback to defrag items in ebuckets, and simultaneously update their references in the ebucket.Mainly changes
activeDefragHfieldDict()
to defrag the dict instead ofactiveDefragSdsDict()
.dictScanDefrag()
, we always set the defrag callbackdefragKey
ofdictDefragFunctions
to NULL, because we can't reallocate a field with out updating it's reference in ebuckets.Instead, we will defrag the field of the dict and update its reference in the callback
dictScanDefrag
of dictScanFunction().ebDefragItem
to defrag the robj and update the reference in db->hexpires.TODO:
Defrag ebucket structure incremently, which will be handler in a future PR.