From db5349d30aa86b215f0957a202a65d2b54c15066 Mon Sep 17 00:00:00 2001 From: Binbin Date: Fri, 2 Aug 2024 15:00:03 +0800 Subject: [PATCH 1/7] Fast path in SET if the expiration time is expired If the expiration time passed in SET is expired, for example, it has expired due to the machine time (DTS) or the expiration time passed in (wrong arg). In this case, we don't need to set the key and wait for the active expire scan before deleting the key. Compared with previous changes: 1. If the key does not exist, previously we would set the key and wait for the active expire to delete it, so it is a set + del from the perspective of propaganda. Now we will no set the key and return, so it a NOP. 2. If the key exists, previously we woule set the key and wait for the active expire to delete it, so it is a set + del From the perspective of propaganda. Now we will delete it and return, so it is a del. A simple benchmark test: ``` ./src/valkey-benchmark -P 32 -c 100 -n 10000000 -r 1000000000 set __rand_int__ value exat 100 old: 252474.23, new: 829256.12 ``` Signed-off-by: Binbin --- src/t_string.c | 22 ++++++++++++++++++++++ tests/unit/type/string.tcl | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/src/t_string.c b/src/t_string.c index 19e19cf5d5..f9a770ce89 100644 --- a/src/t_string.c +++ b/src/t_string.c @@ -109,6 +109,28 @@ void setGenericCommand(client *c, return; } + /* If the milliseconds have expired, then we don't need to set it into the + * database, and then wait for the active expire to delete it, it is wasteful. + * If the key already exists, delete it. */ + if (expire && checkAlreadyExpired(milliseconds)) { + if (found) { + int deleted = dbGenericDelete(c->db, key, server.lazyfree_lazy_expire, DB_FLAG_KEY_EXPIRED); + serverAssertWithInfo(c, key, deleted); + server.dirty++; + + /* Replicate/AOF this as an explicit DEL or UNLINK. */ + robj *aux = server.lazyfree_lazy_expire ? shared.unlink : shared.del; + rewriteClientCommandVector(c, 2, aux, key); + signalModifiedKey(c, c->db, key); + notifyKeyspaceEvent(NOTIFY_GENERIC, "del", key, c->db->id); + } + + if (!(flags & OBJ_SET_GET)) { + addReply(c, ok_reply ? ok_reply : shared.ok); + } + return; + } + /* When expire is not NULL, we avoid deleting the TTL so it can be updated later instead of being deleted and then * created again. */ setkey_flags |= ((flags & OBJ_KEEPTTL) || expire) ? SETKEY_KEEPTTL : 0; diff --git a/tests/unit/type/string.tcl b/tests/unit/type/string.tcl index 381cc4a693..6d273498aa 100644 --- a/tests/unit/type/string.tcl +++ b/tests/unit/type/string.tcl @@ -607,6 +607,40 @@ if {[string match {*jemalloc*} [s mem_allocator]]} { r set foo bar pxat [expr [clock milliseconds] + 10000] assert_range [r ttl foo] 5 10 } + + test "SET EXAT / PXAT Expiration time is expired" { + r debug set-active-expire 0 + set repl [attach_to_replication_stream] + + # Key exists. + r set foo bar + r set foo bar exat [expr [clock seconds] - 100] + assert_error {ERR no such key} {r debug object foo} + r set foo bar + r set foo bar pxat [expr [clock milliseconds] - 10000] + assert_error {ERR no such key} {r debug object foo} + + # Key does not exist. + r del foo + r set foo bar exat [expr [clock seconds] - 100] + assert_error {ERR no such key} {r debug object foo} + r set foo bar pxat [expr [clock milliseconds] - 10000] + assert_error {ERR no such key} {r debug object foo} + + r incr foo + assert_replication_stream $repl { + {select *} + {set foo bar} + {del foo} + {set foo bar} + {del foo} + {incr foo} + } + + r debug set-active-expire 1 + close_replication_stream $repl + } {} {needs:debug needs:repl} + test {Extended SET using multiple options at once} { r set foo val assert {[r set foo bar xx px 10000] eq {OK}} From 4a742cb93b4b48a4cafa1ee011f032e630ae3830 Mon Sep 17 00:00:00 2001 From: Binbin Date: Thu, 8 Aug 2024 11:51:34 +0800 Subject: [PATCH 2/7] adding deleteExpiredKeyFromOverwriteAndPropagate Signed-off-by: Binbin --- src/cluster.c | 5 +---- src/db.c | 15 +++++++++++++++ src/expire.c | 12 +----------- src/server.h | 1 + src/t_string.c | 25 +++---------------------- 5 files changed, 21 insertions(+), 37 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index 162004d703..bfed1de0a9 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -262,10 +262,7 @@ void restoreCommand(client *c) { if (ttl && !absttl) ttl += commandTimeSnapshot(); if (ttl && checkAlreadyExpired(ttl)) { if (deleted) { - robj *aux = server.lazyfree_lazy_server_del ? shared.unlink : shared.del; - rewriteClientCommandVector(c, 2, aux, key); - signalModifiedKey(c, c->db, key); - notifyKeyspaceEvent(NOTIFY_GENERIC, "del", key, c->db->id); + deleteExpiredKeyFromOverwriteAndPropagate(c, key, 0); server.dirty++; } decrRefCount(obj); diff --git a/src/db.c b/src/db.c index d0a6640f57..ab0af1df69 100644 --- a/src/db.c +++ b/src/db.c @@ -1712,6 +1712,21 @@ void deleteExpiredKeyAndPropagate(serverDb *db, robj *keyobj) { server.stat_expiredkeys++; } +/* Delete the specified expired key from overwriting and propagate the DEL or UNLINK. */ +void deleteExpiredKeyFromOverwriteAndPropagate(client *c, robj *keyobj, int do_delete) { + if (do_delete) { + int deleted = dbGenericDelete(c->db, keyobj, server.lazyfree_lazy_expire, DB_FLAG_KEY_EXPIRED); + serverAssertWithInfo(c, keyobj, deleted); + server.dirty++; + } + + /* Replicate/AOF this as an explicit DEL or UNLINK. */ + robj *aux = server.lazyfree_lazy_expire ? shared.unlink : shared.del; + rewriteClientCommandVector(c, 2, aux, keyobj); + signalModifiedKey(c, c->db, keyobj); + notifyKeyspaceEvent(NOTIFY_GENERIC, "del", keyobj, c->db->id); +} + /* Propagate an implicit key deletion into replicas and the AOF file. * When a key was deleted in the primary by eviction, expiration or a similar * mechanism a DEL/UNLINK operation for this key is sent diff --git a/src/expire.c b/src/expire.c index 05abb9580a..f6a711b7db 100644 --- a/src/expire.c +++ b/src/expire.c @@ -668,17 +668,7 @@ void expireGenericCommand(client *c, long long basetime, int unit) { } if (checkAlreadyExpired(when)) { - robj *aux; - - int deleted = dbGenericDelete(c->db, key, server.lazyfree_lazy_expire, DB_FLAG_KEY_EXPIRED); - serverAssertWithInfo(c, key, deleted); - server.dirty++; - - /* Replicate/AOF this as an explicit DEL or UNLINK. */ - aux = server.lazyfree_lazy_expire ? shared.unlink : shared.del; - rewriteClientCommandVector(c, 2, aux, key); - signalModifiedKey(c, c->db, key); - notifyKeyspaceEvent(NOTIFY_GENERIC, "del", key, c->db->id); + deleteExpiredKeyFromOverwriteAndPropagate(c, key, 1); addReply(c, shared.cone); return; } else { diff --git a/src/server.h b/src/server.h index ccdece20dd..996f6e4364 100644 --- a/src/server.h +++ b/src/server.h @@ -3481,6 +3481,7 @@ int setModuleNumericConfig(ModuleConfig *config, long long val, const char **err /* db.c -- Keyspace access API */ int removeExpire(serverDb *db, robj *key); void deleteExpiredKeyAndPropagate(serverDb *db, robj *keyobj); +void deleteExpiredKeyFromOverwriteAndPropagate(client *c, robj *keyobj, int do_delete); void propagateDeletion(serverDb *db, robj *key, int lazy); int keyIsExpired(serverDb *db, robj *key); long long getExpire(serverDb *db, robj *key); diff --git a/src/t_string.c b/src/t_string.c index f9a770ce89..c6e3ffc1c0 100644 --- a/src/t_string.c +++ b/src/t_string.c @@ -113,21 +113,8 @@ void setGenericCommand(client *c, * database, and then wait for the active expire to delete it, it is wasteful. * If the key already exists, delete it. */ if (expire && checkAlreadyExpired(milliseconds)) { - if (found) { - int deleted = dbGenericDelete(c->db, key, server.lazyfree_lazy_expire, DB_FLAG_KEY_EXPIRED); - serverAssertWithInfo(c, key, deleted); - server.dirty++; - - /* Replicate/AOF this as an explicit DEL or UNLINK. */ - robj *aux = server.lazyfree_lazy_expire ? shared.unlink : shared.del; - rewriteClientCommandVector(c, 2, aux, key); - signalModifiedKey(c, c->db, key); - notifyKeyspaceEvent(NOTIFY_GENERIC, "del", key, c->db->id); - } - - if (!(flags & OBJ_SET_GET)) { - addReply(c, ok_reply ? ok_reply : shared.ok); - } + if (found) deleteExpiredKeyFromOverwriteAndPropagate(c, key, 1); + if (!(flags & OBJ_SET_GET)) addReply(c, shared.ok); return; } @@ -417,13 +404,7 @@ void getexCommand(client *c) { if (((flags & OBJ_PXAT) || (flags & OBJ_EXAT)) && checkAlreadyExpired(milliseconds)) { /* When PXAT/EXAT absolute timestamp is specified, there can be a chance that timestamp * has already elapsed so delete the key in that case. */ - int deleted = dbGenericDelete(c->db, c->argv[1], server.lazyfree_lazy_expire, DB_FLAG_KEY_EXPIRED); - serverAssert(deleted); - robj *aux = server.lazyfree_lazy_expire ? shared.unlink : shared.del; - rewriteClientCommandVector(c,2,aux,c->argv[1]); - signalModifiedKey(c, c->db, c->argv[1]); - notifyKeyspaceEvent(NOTIFY_GENERIC, "del", c->argv[1], c->db->id); - server.dirty++; + deleteExpiredKeyFromOverwriteAndPropagate(c, c->argv[1], 1); } else if (expire) { setExpire(c,c->db,c->argv[1],milliseconds); /* Propagate as PXEXPIREAT millisecond-timestamp if there is From 33eb3b5a37ac8ee1d9462e537ed214b7e3bdea6f Mon Sep 17 00:00:00 2001 From: Binbin Date: Thu, 8 Aug 2024 12:12:36 +0800 Subject: [PATCH 3/7] add back tick to milliseconds comment Signed-off-by: Binbin --- src/t_string.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/t_string.c b/src/t_string.c index c6e3ffc1c0..3a2db43e7f 100644 --- a/src/t_string.c +++ b/src/t_string.c @@ -109,7 +109,7 @@ void setGenericCommand(client *c, return; } - /* If the milliseconds have expired, then we don't need to set it into the + /* If the `milliseconds` have expired, then we don't need to set it into the * database, and then wait for the active expire to delete it, it is wasteful. * If the key already exists, delete it. */ if (expire && checkAlreadyExpired(milliseconds)) { From 73cde7ad451466a898ac01ac5397040167943de2 Mon Sep 17 00:00:00 2001 From: Binbin Date: Thu, 8 Aug 2024 13:34:04 +0800 Subject: [PATCH 4/7] fix test Signed-off-by: Binbin --- tests/unit/dump.tcl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/dump.tcl b/tests/unit/dump.tcl index e4c0f9d312..baa13bab0e 100644 --- a/tests/unit/dump.tcl +++ b/tests/unit/dump.tcl @@ -118,7 +118,7 @@ start_server {tags {"dump"}} { assert_replication_stream $repl { {select *} {del key1{t}} - {unlink key2{t}} + {del key2{t}} } close_replication_stream $repl From e9e089bcf4134b4add98616a01c726d3de943782 Mon Sep 17 00:00:00 2001 From: Binbin Date: Thu, 8 Aug 2024 13:41:54 +0800 Subject: [PATCH 5/7] leave restore alone Signed-off-by: Binbin --- src/cluster.c | 7 ++++++- tests/unit/dump.tcl | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index bfed1de0a9..e49cdba655 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -262,7 +262,12 @@ void restoreCommand(client *c) { if (ttl && !absttl) ttl += commandTimeSnapshot(); if (ttl && checkAlreadyExpired(ttl)) { if (deleted) { - deleteExpiredKeyFromOverwriteAndPropagate(c, key, 0); + /* Here we don't use deleteExpiredKeyFromOverwriteAndPropagate because + * strictly speaking, the `deleted` is triggered by `replace`. */ + robj *aux = server.lazyfree_lazy_server_del ? shared.unlink : shared.del; + rewriteClientCommandVector(c, 2, aux, key); + signalModifiedKey(c, c->db, key); + notifyKeyspaceEvent(NOTIFY_GENERIC, "del", key, c->db->id); server.dirty++; } decrRefCount(obj); diff --git a/tests/unit/dump.tcl b/tests/unit/dump.tcl index baa13bab0e..e4c0f9d312 100644 --- a/tests/unit/dump.tcl +++ b/tests/unit/dump.tcl @@ -118,7 +118,7 @@ start_server {tags {"dump"}} { assert_replication_stream $repl { {select *} {del key1{t}} - {del key2{t}} + {unlink key2{t}} } close_replication_stream $repl From c7f74af86719f8e3f3dfd46a4a2b309c9e57244d Mon Sep 17 00:00:00 2001 From: Binbin Date: Fri, 23 Aug 2024 10:43:24 +0800 Subject: [PATCH 6/7] remove do_delete arg Signed-off-by: Binbin --- src/db.c | 10 ++++------ src/expire.c | 2 +- src/server.h | 2 +- src/t_string.c | 4 ++-- 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/db.c b/src/db.c index 5a0d5458e1..0e8661f02b 100644 --- a/src/db.c +++ b/src/db.c @@ -1713,12 +1713,10 @@ void deleteExpiredKeyAndPropagate(serverDb *db, robj *keyobj) { } /* Delete the specified expired key from overwriting and propagate the DEL or UNLINK. */ -void deleteExpiredKeyFromOverwriteAndPropagate(client *c, robj *keyobj, int do_delete) { - if (do_delete) { - int deleted = dbGenericDelete(c->db, keyobj, server.lazyfree_lazy_expire, DB_FLAG_KEY_EXPIRED); - serverAssertWithInfo(c, keyobj, deleted); - server.dirty++; - } +void deleteExpiredKeyFromOverwriteAndPropagate(client *c, robj *keyobj) { + int deleted = dbGenericDelete(c->db, keyobj, server.lazyfree_lazy_expire, DB_FLAG_KEY_EXPIRED); + serverAssertWithInfo(c, keyobj, deleted); + server.dirty++; /* Replicate/AOF this as an explicit DEL or UNLINK. */ robj *aux = server.lazyfree_lazy_expire ? shared.unlink : shared.del; diff --git a/src/expire.c b/src/expire.c index 18fda97d5c..97426e9e1a 100644 --- a/src/expire.c +++ b/src/expire.c @@ -668,7 +668,7 @@ void expireGenericCommand(client *c, long long basetime, int unit) { } if (checkAlreadyExpired(when)) { - deleteExpiredKeyFromOverwriteAndPropagate(c, key, 1); + deleteExpiredKeyFromOverwriteAndPropagate(c, key); addReply(c, shared.cone); return; } else { diff --git a/src/server.h b/src/server.h index 28d9ca6277..ba77b8b3a9 100644 --- a/src/server.h +++ b/src/server.h @@ -3490,7 +3490,7 @@ int setModuleNumericConfig(ModuleConfig *config, long long val, const char **err /* db.c -- Keyspace access API */ int removeExpire(serverDb *db, robj *key); void deleteExpiredKeyAndPropagate(serverDb *db, robj *keyobj); -void deleteExpiredKeyFromOverwriteAndPropagate(client *c, robj *keyobj, int do_delete); +void deleteExpiredKeyFromOverwriteAndPropagate(client *c, robj *keyobj); void propagateDeletion(serverDb *db, robj *key, int lazy); int keyIsExpired(serverDb *db, robj *key); long long getExpire(serverDb *db, robj *key); diff --git a/src/t_string.c b/src/t_string.c index 021427c22e..429fdfac49 100644 --- a/src/t_string.c +++ b/src/t_string.c @@ -113,7 +113,7 @@ void setGenericCommand(client *c, * database, and then wait for the active expire to delete it, it is wasteful. * If the key already exists, delete it. */ if (expire && checkAlreadyExpired(milliseconds)) { - if (found) deleteExpiredKeyFromOverwriteAndPropagate(c, key, 1); + if (found) deleteExpiredKeyFromOverwriteAndPropagate(c, key); if (!(flags & OBJ_SET_GET)) addReply(c, shared.ok); return; } @@ -404,7 +404,7 @@ void getexCommand(client *c) { if (((flags & OBJ_PXAT) || (flags & OBJ_EXAT)) && checkAlreadyExpired(milliseconds)) { /* When PXAT/EXAT absolute timestamp is specified, there can be a chance that timestamp * has already elapsed so delete the key in that case. */ - deleteExpiredKeyFromOverwriteAndPropagate(c, c->argv[1], 1); + deleteExpiredKeyFromOverwriteAndPropagate(c, c->argv[1]); } else if (expire) { setExpire(c,c->db,c->argv[1],milliseconds); /* Propagate as PXEXPIREAT millisecond-timestamp if there is From 58c086c9c9bda8ea68c6e5af12927d472edfe317 Mon Sep 17 00:00:00 2001 From: Binbin Date: Fri, 23 Aug 2024 12:27:21 +0800 Subject: [PATCH 7/7] Update src/cluster.c Co-authored-by: Madelyn Olson Signed-off-by: Binbin --- src/cluster.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cluster.c b/src/cluster.c index 7bda6d81db..0434bad16e 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -263,7 +263,7 @@ void restoreCommand(client *c) { if (ttl && checkAlreadyExpired(ttl)) { if (deleted) { /* Here we don't use deleteExpiredKeyFromOverwriteAndPropagate because - * strictly speaking, the `deleted` is triggered by `replace`. */ + * strictly speaking, the `delete` is triggered by the `replace`. */ robj *aux = server.lazyfree_lazy_server_del ? shared.unlink : shared.del; rewriteClientCommandVector(c, 2, aux, key); signalModifiedKey(c, c->db, key);