From 28c20b4ef95aa5f74b938681a6f78f6b92dec2a8 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Sun, 10 Nov 2019 09:04:39 +0200 Subject: [PATCH] rename RN_SetLRUOrLFU -> RM_SetLRU and RN_SetLFU - the API name was odd, separated to two apis one for LRU and one for LFU - the LRU idle time was in 1 second resolution, which might be ok for RDB and RESTORE, but i think modules may need higher resolution - adding tests for LFU and for handling maxmemory policy mismatch --- src/cluster.c | 2 +- src/module.c | 64 +++++++++++++++++++++------------ src/object.c | 4 +-- src/rdb.c | 2 +- src/redismodule.h | 12 ++++--- src/server.h | 2 +- tests/modules/misc.c | 66 +++++++++++++++++++++++++++++------ tests/unit/moduleapi/misc.tcl | 33 ++++++++++++++++-- 8 files changed, 141 insertions(+), 44 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index a7d8a02c3..9e6ddb2c4 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -4966,7 +4966,7 @@ void restoreCommand(client *c) { if (!absttl) ttl+=mstime(); setExpire(c,c->db,c->argv[1],ttl); } - objectSetLRUOrLFU(obj,lfu_freq,lru_idle,lru_clock); + objectSetLRUOrLFU(obj,lfu_freq,lru_idle,lru_clock,1000); signalModifiedKey(c->db,c->argv[1]); addReply(c,shared.ok); server.dirty++; diff --git a/src/module.c b/src/module.c index ad34e7b64..cd79a5908 100644 --- a/src/module.c +++ b/src/module.c @@ -6736,35 +6736,53 @@ size_t moduleCount(void) { return dictSize(modules); } -/* Set the key LRU/LFU depending on server.maxmemory_policy. - * The lru_idle arg is idle time in seconds, and is only relevant if the - * eviction policy is LRU based. - * The lfu_freq arg is a logarithmic counter that provides an indication of - * the access frequencyonly (must be <= 255) and is only relevant if the - * eviction policy is LFU based. - * Either or both of them may be <0, in that case, nothing is set. */ -/* return value is an indication if the lru field was updated or not. */ -int RM_SetLRUOrLFU(RedisModuleKey *key, long long lfu_freq, long long lru_idle) { +/* Set the key last access time for LRU based eviction. not relevent if the + * servers's maxmemory policy is LFU based. Value is idle time in milliseconds. + * returns REDISMODULE_OK if the LRU was updated, REDISMODULE_ERR otherwise. */ +int RM_SetLRU(RedisModuleKey *key, mstime_t lru_idle) { if (!key->value) return REDISMODULE_ERR; - if (objectSetLRUOrLFU(key->value, lfu_freq, lru_idle, lru_idle>=0 ? LRU_CLOCK() : 0)) + if (objectSetLRUOrLFU(key->value, -1, lru_idle, lru_idle>=0 ? LRU_CLOCK() : 0, 1)) return REDISMODULE_OK; return REDISMODULE_ERR; } -/* Gets the key LRU or LFU (depending on the current eviction policy). - * One will be set to the appropiate return value, and the other will be set to -1. - * see RedisModule_SetLRUOrLFU for units and ranges. - * return value is an indication of success. */ -int RM_GetLRUOrLFU(RedisModuleKey *key, long long *lfu_freq, long long *lru_idle) { - *lru_idle = *lfu_freq = -1; +/* Gets the key last access time. + * Value is idletime in milliseconds or -1 if the server's eviction policy is + * LFU based. + * returns REDISMODULE_OK if when key is valid. */ +int RM_GetLRU(RedisModuleKey *key, mstime_t *lru_idle) { + *lru_idle = -1; if (!key->value) return REDISMODULE_ERR; - if (server.maxmemory_policy & MAXMEMORY_FLAG_LFU) { + if (server.maxmemory_policy & MAXMEMORY_FLAG_LFU) + return REDISMODULE_OK; + *lru_idle = estimateObjectIdleTime(key->value); + return REDISMODULE_OK; +} + +/* Set the key access frequency. only relevant if the server's maxmemory policy + * is LFU based. + * The frequency is a logarithmic counter that provides an indication of + * the access frequencyonly (must be <= 255). + * returns REDISMODULE_OK if the LFU was updated, REDISMODULE_ERR otherwise. */ +int RM_SetLFU(RedisModuleKey *key, long long lfu_freq) { + if (!key->value) + return REDISMODULE_ERR; + if (objectSetLRUOrLFU(key->value, lfu_freq, -1, 0, 1)) + return REDISMODULE_OK; + return REDISMODULE_ERR; +} + +/* Gets the key access frequency or -1 if the server's eviction policy is not + * LFU based. + * returns REDISMODULE_OK if when key is valid. */ +int RM_GetLFU(RedisModuleKey *key, long long *lfu_freq) { + *lfu_freq = -1; + if (!key->value) + return REDISMODULE_ERR; + if (server.maxmemory_policy & MAXMEMORY_FLAG_LFU) *lfu_freq = LFUDecrAndReturn(key->value); - } else { - *lru_idle = estimateObjectIdleTime(key->value)/1000; - } return REDISMODULE_OK; } @@ -6966,8 +6984,10 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(GetClientInfoById); REGISTER_API(PublishMessage); REGISTER_API(SubscribeToServerEvent); - REGISTER_API(SetLRUOrLFU); - REGISTER_API(GetLRUOrLFU); + REGISTER_API(SetLRU); + REGISTER_API(GetLRU); + REGISTER_API(SetLFU); + REGISTER_API(GetLFU); REGISTER_API(BlockClientOnKeys); REGISTER_API(SignalKeyAsReady); REGISTER_API(GetBlockedClientReadyKey); diff --git a/src/object.c b/src/object.c index 5e9a99dec..c6d89bfa7 100644 --- a/src/object.c +++ b/src/object.c @@ -1210,7 +1210,7 @@ sds getMemoryDoctorReport(void) { * is MAXMEMORY_FLAG_LRU. * Either or both of them may be <0, in that case, nothing is set. */ int objectSetLRUOrLFU(robj *val, long long lfu_freq, long long lru_idle, - long long lru_clock) { + long long lru_clock, int lru_multiplier) { if (server.maxmemory_policy & MAXMEMORY_FLAG_LFU) { if (lfu_freq >= 0) { serverAssert(lfu_freq <= 255); @@ -1222,7 +1222,7 @@ int objectSetLRUOrLFU(robj *val, long long lfu_freq, long long lru_idle, * according to the LRU clock resolution this Redis * instance was compiled with (normally 1000 ms, so the * below statement will expand to lru_idle*1000/1000. */ - lru_idle = lru_idle*1000/LRU_CLOCK_RESOLUTION; + lru_idle = lru_idle*lru_multiplier/LRU_CLOCK_RESOLUTION; long lru_abs = lru_clock - lru_idle; /* Absolute access time. */ /* If the LRU field underflows (since LRU it is a wrapping * clock), the best we can do is to provide a large enough LRU diff --git a/src/rdb.c b/src/rdb.c index b569edfea..301a33642 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2239,7 +2239,7 @@ int rdbLoadRio(rio *rdb, int rdbflags, rdbSaveInfo *rsi) { if (expiretime != -1) setExpire(NULL,db,key,expiretime); /* Set usage information (for eviction). */ - objectSetLRUOrLFU(val,lfu_freq,lru_idle,lru_clock); + objectSetLRUOrLFU(val,lfu_freq,lru_idle,lru_clock,1000); /* Decrement the key refcount since dbAdd() will take its * own reference. */ diff --git a/src/redismodule.h b/src/redismodule.h index 728c7f584..e388d7439 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -583,8 +583,10 @@ int REDISMODULE_API_FUNC(RedisModule_InfoAddFieldDouble)(RedisModuleInfoCtx *ctx int REDISMODULE_API_FUNC(RedisModule_InfoAddFieldLongLong)(RedisModuleInfoCtx *ctx, char *field, long long value); int REDISMODULE_API_FUNC(RedisModule_InfoAddFieldULongLong)(RedisModuleInfoCtx *ctx, char *field, unsigned long long value); int REDISMODULE_API_FUNC(RedisModule_SubscribeToServerEvent)(RedisModuleCtx *ctx, RedisModuleEvent event, RedisModuleEventCallback callback); -int REDISMODULE_API_FUNC(RedisModule_SetLRUOrLFU)(RedisModuleKey *key, long long lfu_freq, long long lru_idle); -int REDISMODULE_API_FUNC(RedisModule_GetLRUOrLFU)(RedisModuleKey *key, long long *lfu_freq, long long *lru_idle); +int REDISMODULE_API_FUNC(RedisModule_SetLRU)(RedisModuleKey *key, mstime_t lru_idle); +int REDISMODULE_API_FUNC(RedisModule_GetLRU)(RedisModuleKey *key, mstime_t *lru_idle); +int REDISMODULE_API_FUNC(RedisModule_SetLFU)(RedisModuleKey *key, long long lfu_freq); +int REDISMODULE_API_FUNC(RedisModule_GetLFU)(RedisModuleKey *key, long long *lfu_freq); RedisModuleBlockedClient *REDISMODULE_API_FUNC(RedisModule_BlockClientOnKeys)(RedisModuleCtx *ctx, RedisModuleCmdFunc reply_callback, RedisModuleCmdFunc timeout_callback, void (*free_privdata)(RedisModuleCtx*,void*), long long timeout_ms, RedisModuleString **keys, int numkeys, void *privdata); void REDISMODULE_API_FUNC(RedisModule_SignalKeyAsReady)(RedisModuleCtx *ctx, RedisModuleString *key); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_GetBlockedClientReadyKey)(RedisModuleCtx *ctx); @@ -794,8 +796,10 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(GetClientInfoById); REDISMODULE_GET_API(PublishMessage); REDISMODULE_GET_API(SubscribeToServerEvent); - REDISMODULE_GET_API(SetLRUOrLFU); - REDISMODULE_GET_API(GetLRUOrLFU); + REDISMODULE_GET_API(SetLRU); + REDISMODULE_GET_API(GetLRU); + REDISMODULE_GET_API(SetLFU); + REDISMODULE_GET_API(GetLFU); REDISMODULE_GET_API(BlockClientOnKeys); REDISMODULE_GET_API(SignalKeyAsReady); REDISMODULE_GET_API(GetBlockedClientReadyKey); diff --git a/src/server.h b/src/server.h index 8063dc101..c9ac3003e 100644 --- a/src/server.h +++ b/src/server.h @@ -2089,7 +2089,7 @@ robj *lookupKeyWriteWithFlags(redisDb *db, robj *key, int flags); robj *objectCommandLookup(client *c, robj *key); robj *objectCommandLookupOrReply(client *c, robj *key, robj *reply); int objectSetLRUOrLFU(robj *val, long long lfu_freq, long long lru_idle, - long long lru_clock); + long long lru_clock, int lru_multiplier); #define LOOKUP_NONE 0 #define LOOKUP_NOTOUCH (1<<0) void dbAdd(redisDb *db, robj *key, robj *val); diff --git a/tests/modules/misc.c b/tests/modules/misc.c index 7701a9c7c..06b5af620 100644 --- a/tests/modules/misc.c +++ b/tests/modules/misc.c @@ -68,16 +68,24 @@ int test_randomkey(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) return REDISMODULE_OK; } +RedisModuleKey *open_key_or_reply(RedisModuleCtx *ctx, RedisModuleString *keyname, int mode) { + RedisModuleKey *key = RedisModule_OpenKey(ctx, keyname, mode); + if (!key) { + RedisModule_ReplyWithError(ctx, "key not found"); + return NULL; + } + return key; +} + int test_getlru(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { if (argc<2) { RedisModule_WrongArity(ctx); return REDISMODULE_OK; } - RedisModuleString *keyname = argv[1]; - RedisModuleKey *key = RedisModule_OpenKey(ctx, keyname, REDISMODULE_READ|REDISMODULE_OPEN_KEY_NOTOUCH); - long long lru, lfu; - RedisModule_GetLRUOrLFU(key, &lfu, &lru); + RedisModuleKey *key = open_key_or_reply(ctx, argv[1], REDISMODULE_READ|REDISMODULE_OPEN_KEY_NOTOUCH); + mstime_t lru; + RedisModule_GetLRU(key, &lru); RedisModule_ReplyWithLongLong(ctx, lru); RedisModule_CloseKey(key); return REDISMODULE_OK; @@ -89,12 +97,46 @@ int test_setlru(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) RedisModule_WrongArity(ctx); return REDISMODULE_OK; } - RedisModuleString *keyname = argv[1]; - RedisModuleKey *key = RedisModule_OpenKey(ctx, keyname, REDISMODULE_WRITE|REDISMODULE_OPEN_KEY_NOTOUCH); - long long lru; - RedisModule_StringToLongLong(argv[2], &lru); - RedisModule_SetLRUOrLFU(key, -1, lru); - RedisModule_ReplyWithCString(ctx, "Ok"); + RedisModuleKey *key = open_key_or_reply(ctx, argv[1], REDISMODULE_READ|REDISMODULE_OPEN_KEY_NOTOUCH); + mstime_t lru; + if (RedisModule_StringToLongLong(argv[2], &lru) != REDISMODULE_OK) { + RedisModule_ReplyWithError(ctx, "invalid idle time"); + return REDISMODULE_OK; + } + int was_set = RedisModule_SetLRU(key, lru)==REDISMODULE_OK; + RedisModule_ReplyWithLongLong(ctx, was_set); + RedisModule_CloseKey(key); + return REDISMODULE_OK; +} + +int test_getlfu(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +{ + if (argc<2) { + RedisModule_WrongArity(ctx); + return REDISMODULE_OK; + } + RedisModuleKey *key = open_key_or_reply(ctx, argv[1], REDISMODULE_READ|REDISMODULE_OPEN_KEY_NOTOUCH); + mstime_t lfu; + RedisModule_GetLFU(key, &lfu); + RedisModule_ReplyWithLongLong(ctx, lfu); + RedisModule_CloseKey(key); + return REDISMODULE_OK; +} + +int test_setlfu(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +{ + if (argc<3) { + RedisModule_WrongArity(ctx); + return REDISMODULE_OK; + } + RedisModuleKey *key = open_key_or_reply(ctx, argv[1], REDISMODULE_READ|REDISMODULE_OPEN_KEY_NOTOUCH); + mstime_t lfu; + if (RedisModule_StringToLongLong(argv[2], &lfu) != REDISMODULE_OK) { + RedisModule_ReplyWithError(ctx, "invalid freq"); + return REDISMODULE_OK; + } + int was_set = RedisModule_SetLFU(key, lfu)==REDISMODULE_OK; + RedisModule_ReplyWithLongLong(ctx, was_set); RedisModule_CloseKey(key); return REDISMODULE_OK; } @@ -119,6 +161,10 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) return REDISMODULE_ERR; if (RedisModule_CreateCommand(ctx,"test.getlru", test_getlru,"",0,0,0) == REDISMODULE_ERR) return REDISMODULE_ERR; + if (RedisModule_CreateCommand(ctx,"test.setlfu", test_setlfu,"",0,0,0) == REDISMODULE_ERR) + return REDISMODULE_ERR; + if (RedisModule_CreateCommand(ctx,"test.getlfu", test_getlfu,"",0,0,0) == REDISMODULE_ERR) + return REDISMODULE_ERR; return REDISMODULE_OK; } diff --git a/tests/unit/moduleapi/misc.tcl b/tests/unit/moduleapi/misc.tcl index ebfa9631f..376bc2eed 100644 --- a/tests/unit/moduleapi/misc.tcl +++ b/tests/unit/moduleapi/misc.tcl @@ -26,13 +26,40 @@ start_server {tags {"modules"}} { } test {test modle lru api} { + r config set maxmemory-policy allkeys-lru r set x foo set lru [r test.getlru x] - assert { $lru <= 1 } - r test.setlru x 100 + assert { $lru <= 1000 } + set was_set [r test.setlru x 100000] + assert { $was_set == 1 } set idle [r object idletime x] assert { $idle >= 100 } set lru [r test.getlru x] - assert { $lru >= 100 } + assert { $lru >= 100000 } + r config set maxmemory-policy allkeys-lfu + set lru [r test.getlru x] + assert { $lru == -1 } + set was_set [r test.setlru x 100000] + assert { $was_set == 0 } } + r config set maxmemory-policy allkeys-lru + + test {test modle lfu api} { + r config set maxmemory-policy allkeys-lfu + r set x foo + set lfu [r test.getlfu x] + assert { $lfu >= 1 } + set was_set [r test.setlfu x 100] + assert { $was_set == 1 } + set freq [r object freq x] + assert { $freq <= 100 } + set lfu [r test.getlfu x] + assert { $lfu <= 100 } + r config set maxmemory-policy allkeys-lru + set lfu [r test.getlfu x] + assert { $lfu == -1 } + set was_set [r test.setlfu x 100] + assert { $was_set == 0 } + } + }