From b7e6ba3464d46fa0a0382d66aff490351cebc0c2 Mon Sep 17 00:00:00 2001 From: Moti Cohen Date: Sun, 2 Jun 2024 11:37:21 +0300 Subject: [PATCH] Fix the logic of nextExpireTime at ebExpire() --- src/ebuckets.c | 30 ++++++++++++++++++++++++------ src/ebuckets.h | 6 ++---- src/t_hash.c | 11 +++++------ 3 files changed, 31 insertions(+), 16 deletions(-) diff --git a/src/ebuckets.c b/src/ebuckets.c index 387aef88c93..f4f88fadee4 100644 --- a/src/ebuckets.c +++ b/src/ebuckets.c @@ -385,6 +385,8 @@ int ebSingleSegExpire(FirstSegHdr *firstSegHdr, break; } + /* If indicated to re-insert the item, then chain it to updateList. + * it will be ebAdd() back to ebuckets at the end of ebExpire() */ if (act == ACT_UPDATE_EXP_ITEM) { mIter->next = *updateList; *updateList = iter; @@ -457,6 +459,8 @@ static int ebSegExpire(FirstSegHdr *firstSegHdr, break; } + /* If indicated to re-insert the item, then chain it to updateList. + * it will be ebAdd() back to ebuckets at the end of ebExpire() */ if (act == ACT_UPDATE_EXP_ITEM) { mIter->next = *updateList; *updateList = iter; @@ -695,6 +699,8 @@ static int ebListExpire(ebuckets *eb, break; } + /* If indicated to re-insert the item, then chain it to updateList. + * it will be ebAdd() back to ebuckets at the end of ebExpire() */ if (act == ACT_UPDATE_EXP_ITEM) { metaItem->next = *updateList; *updateList = item; @@ -707,7 +713,7 @@ static int ebListExpire(ebuckets *eb, if (expired == numItems) { *eb = NULL; - info->nextExpireTime = 0; + info->nextExpireTime = EB_EXPIRE_TIME_INVALID; return 1; } @@ -1458,11 +1464,14 @@ int ebAdd(ebuckets *eb, EbucketsType *type, eItem item, uint64_t expireTime) { void ebExpire(ebuckets *eb, EbucketsType *type, ExpireInfo *info) { /* updateList - maintain a list of expired items that the callback `onExpireItem` * indicated to update their expiration time rather than removing them. - * At the end of this function, `updateList` will be `ebAdd()` back. */ + * At the end of this function, the items will be `ebAdd()` back. + * + * Note, this list of items does not allocate any memory, but temporary reuses + * the `next` pointer of the `ExpireMeta` structure of the expired items. */ eItem updateList = NULL; /* reset info outputs */ - info->nextExpireTime = 0; + info->nextExpireTime = EB_EXPIRE_TIME_INVALID; info->itemsExpired = 0; /* if empty ebuckets */ @@ -1523,7 +1532,14 @@ void ebExpire(ebuckets *eb, EbucketsType *type, ExpireInfo *info) { while (updateList) { ExpireMeta *mItem = type->getExpireMeta(updateList); eItem next = mItem->next; - ebAdd(eb, type, updateList, ebGetMetaExpTime(mItem)); + uint64_t expireAt = ebGetMetaExpTime(mItem); + + /* Update next minimum expire time if needed. + * Condition is valid also if nextExpireTime is EB_EXPIRE_TIME_INVALID */ + if (expireAt < info->nextExpireTime) + info->nextExpireTime = expireAt; + + ebAdd(eb, type, updateList, expireAt); updateList = next; } @@ -2179,7 +2195,7 @@ int ebucketsTest(int argc, char **argv, int flags) { assert(info.itemsExpired == 1); if (i == numItems) { /* if last item */ assert(eb == NULL); - assert(info.nextExpireTime == 0); + assert(info.nextExpireTime == EB_EXPIRE_TIME_INVALID); } else { assert(info.nextExpireTime == EB_BUCKET_EXP_TIME(i)); } @@ -2209,7 +2225,7 @@ int ebucketsTest(int argc, char **argv, int flags) { assert(info.itemsExpired == expirePerIter); if (i == numIterations) { /* if last item */ assert(eb == NULL); - assert(info.nextExpireTime == 0); + assert(info.nextExpireTime == EB_EXPIRE_TIME_INVALID); } else { assert(info.nextExpireTime == expireTime); } @@ -2372,6 +2388,7 @@ int ebucketsTest(int argc, char **argv, int flags) { .itemsExpired = 0}; ebExpire(&eb, &myEbucketsType2, &info); assert(info.itemsExpired == (uint64_t) numItems); + assert(info.nextExpireTime == (uint64_t)updateItemTo << EB_BUCKET_KEY_PRECISION); assert(ebGetTotalItems(eb, &myEbucketsType2) == 1); /* active-expire. Expected that all will be expired */ @@ -2383,6 +2400,7 @@ int ebucketsTest(int argc, char **argv, int flags) { .itemsExpired = 0}; ebExpire(&eb, &myEbucketsType2, &info2); assert(info2.itemsExpired == (uint64_t) 1); + assert(info2.nextExpireTime == EB_EXPIRE_TIME_INVALID); assert(ebGetTotalItems(eb, &myEbucketsType2) == 0); ebDestroy(&eb, &myEbucketsType2, NULL); diff --git a/src/ebuckets.h b/src/ebuckets.h index 66954131a02..9f66589e517 100644 --- a/src/ebuckets.h +++ b/src/ebuckets.h @@ -250,11 +250,9 @@ typedef struct ExpireInfo { uint64_t maxToExpire; /* [INPUT ] Limit of number expired items to scan */ void *ctx; /* [INPUT ] context to pass to onExpireItem */ uint64_t now; /* [INPUT ] Current time in msec. */ - uint64_t nextExpireTime; /* [OUTPUT] Next expiration time. Return 0, if none left. */ - - /* TODO: Distinct between expired & updated */ uint64_t itemsExpired; /* [OUTPUT] Returns the number of expired or updated items. */ - + uint64_t nextExpireTime; /* [OUTPUT] Next expiration time. Returns + EB_EXPIRE_TIME_INVALID if none left. */ } ExpireInfo; /* ebuckets API */ diff --git a/src/t_hash.c b/src/t_hash.c index a4b182d91b2..282628d52b2 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -446,7 +446,7 @@ void listpackExExpire(redisDb *db, robj *o, ExpireInfo *info) { lpt->lp = lpDeleteRange(lpt->lp, 0, expired * 3); min = hashTypeGetNextTimeToExpire(o); - info->nextExpireTime = (min != EB_EXPIRE_TIME_INVALID) ? min : 0; + info->nextExpireTime = min; } static void listpackExAddInternal(robj *o, listpackEntry ent[3]) { @@ -1885,7 +1885,7 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) { activeExpireCtx->fieldsToExpireQuota -= info.itemsExpired; /* If hash has no more fields to expire, remove it from HFE DB */ - if (info.nextExpireTime == 0) { + if (info.nextExpireTime == EB_EXPIRE_TIME_INVALID) { if (hashTypeLength(hashObj, 0) == 0) { robj *key = createStringObject(keystr, sdslen(keystr)); dbDelete(activeExpireCtx->db, key); @@ -1896,10 +1896,9 @@ static ExpireAction hashTypeActiveExpire(eItem _hashObj, void *ctx) { } return ACT_REMOVE_EXP_ITEM; } else { - /* Hash has more fields to expire. Keep hash to pending items that will - * be added back to global HFE DS at the end of ebExpire() */ - ExpireMeta *expireMeta = hashGetExpireMeta(hashObj); - ebSetMetaExpTime(expireMeta, info.nextExpireTime); + /* Hash has more fields to expire. Update next expiration time of the hash + * and indicate to add it back to global HFE DS */ + ebSetMetaExpTime(hashGetExpireMeta(hashObj), info.nextExpireTime); return ACT_UPDATE_EXP_ITEM; } }