Optimize Expiry Check in `scanCallback()` Using `kvobj` (#14140)
CI / test-ubuntu-latest (push) Waiting to run Details
CI / test-sanitizer-address (push) Waiting to run Details
CI / build-debian-old (push) Waiting to run Details
CI / build-macos-latest (push) Waiting to run Details
CI / build-32bit (push) Waiting to run Details
CI / build-libc-malloc (push) Waiting to run Details
CI / build-centos-jemalloc (push) Waiting to run Details
CI / build-old-chain-jemalloc (push) Waiting to run Details
Codecov / code-coverage (push) Waiting to run Details
External Server Tests / test-external-standalone (push) Waiting to run Details
External Server Tests / test-external-cluster (push) Waiting to run Details
External Server Tests / test-external-nodebug (push) Waiting to run Details
Spellcheck / Spellcheck (push) Waiting to run Details

The current `scanCallback()` implementation performs expiry checks like
this:

```c
robj kobj;
sds keyname = kvobjGetKey(kv);
initStaticStringObject(kobj, keyname);
expireIfNeeded(db, &kobj, kv, 0);
```

This pattern introduces unnecessary temporary stack allocation for robj
and additional memory traffic, confirmed by topdown analysis + perf

```
sudo ./toplev.py --pid $(pgrep redis-server) --level 2 --run-sample -- sleep 30
# 5.01-full-perf on Intel(R) Xeon(R) Platinum 8488C [spr/sapphire_rapids]
BE               Backend_Bound               % Slots                       49.6  
BE/Mem           Backend_Bound.Memory_Bound  % Slots                       34.3  <==
	This metric represents fraction of slots the Memory
	subsystem within the Backend was a bottleneck...
```


From perf record -g sampling (hot path in scanCallback):

kvobjGetKey() + initStaticStringObject() account for ~12% frontend +
backend bound stalls


These cycles are avoidable when kvobj is already available.

This PR extends expireIfNeeded to support key is none (using kvobj) , in
order to avoid sds key copy + temporary robj on scanCallback when it's
not needed.

## Benchmarks

By running 
```
redis-benchmarks-spec-client-runner --tests-regexp ".*scan.*" --flushall_on_every_test_start --flushall_on_every_test_end  --cpuset_start_pos 2 --override-memtier-test-time 30 --benchmark_local_install --override-test-runs 3 --db_server_port <...> --db_server_password <...> --db_server_host <...>
```

We see 

Test Name | Metric | baseline redis Wed Jun 25
(4313d7ff23) | comparison redis
(3bb00b3a97) | Δ (%)
-- | -- | -- | -- | --
generic-scan-count-500 | Ops/sec | 25126 | 26726 | 6.4%
generic-scan-cursor-count-5000 | Ops/sec | 1416.18 | 1446 | 2.1%
generic-scan-count-500 | p99 | 55.039 | 50.687 | 8.6%
generic-scan-cursor-count-5000 | p99 | 1448.62 | 1064.959 | 36.0%

---------

Co-authored-by: debing.sun <debing.sun@redis.com>
Co-authored-by: Yuan Wang <yuan.wang@redis.com>
This commit is contained in:
Filipe Oliveira (Redis) 2025-06-25 13:32:23 +01:00 committed by GitHub
parent 4313d7ff23
commit a744411f27
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
1 changed files with 15 additions and 11 deletions

View File

@ -16,6 +16,7 @@
#include "latency.h" #include "latency.h"
#include "script.h" #include "script.h"
#include "functions.h" #include "functions.h"
#include "redisassert.h"
#include <signal.h> #include <signal.h>
#include <ctype.h> #include <ctype.h>
@ -1303,11 +1304,9 @@ void scanCallback(void *privdata, const dictEntry *de, dictEntryLink plink) {
if (!o) { /* If scanning keyspace */ if (!o) { /* If scanning keyspace */
kvobj *kv = dictGetKV(de); kvobj *kv = dictGetKV(de);
/* Expiration check first - only for database keyspace scanning */ /* Expiration check first - only for database keyspace scanning.
robj kobj; * Use kv obj to avoid robj creation. */
sds keyname = kvobjGetKey(kv); if (expireIfNeeded(data->db, NULL, kv, 0) != KEY_VALID)
initStaticStringObject(kobj, keyname);
if (expireIfNeeded(data->db, &kobj, kv, 0) != KEY_VALID)
return; return;
/* Type filtering - only for database keyspace scanning */ /* Type filtering - only for database keyspace scanning */
@ -2480,11 +2479,10 @@ int keyIsExpired(redisDb *db, sds key, kvobj *kv) {
* You can optionally pass `kv` to save a lookup. * You can optionally pass `kv` to save a lookup.
*/ */
keyStatus expireIfNeeded(redisDb *db, robj *key, kvobj *kv, int flags) { keyStatus expireIfNeeded(redisDb *db, robj *key, kvobj *kv, int flags) {
serverAssert(key != NULL); debugAssert(key != NULL || kv != NULL);
sds keyname = key->ptr;
if ((server.allow_access_expired) || if ((server.allow_access_expired) ||
(flags & EXPIRE_ALLOW_ACCESS_EXPIRED) || (flags & EXPIRE_ALLOW_ACCESS_EXPIRED) ||
(!keyIsExpired(db, keyname, kv))) (!keyIsExpired(db, key ? key->ptr : NULL, kv)))
return KEY_VALID; return KEY_VALID;
/* If we are running in the context of a replica, instead of /* If we are running in the context of a replica, instead of
@ -2515,9 +2513,15 @@ keyStatus expireIfNeeded(redisDb *db, robj *key, kvobj *kv, int flags) {
* will have failed over and the new primary will send us the expire. */ * will have failed over and the new primary will send us the expire. */
if (isPausedActionsWithUpdate(PAUSE_ACTION_EXPIRE)) return KEY_EXPIRED; if (isPausedActionsWithUpdate(PAUSE_ACTION_EXPIRE)) return KEY_EXPIRED;
/* Delete the key */ /* Perform deletion */
if (key) {
deleteExpiredKeyAndPropagate(db, key); deleteExpiredKeyAndPropagate(db, key);
} else {
sds keyname = kvobjGetKey(kv);
robj *tmpkey = createStringObject(keyname, sdslen(keyname));
deleteExpiredKeyAndPropagate(db, tmpkey);
decrRefCount(tmpkey);
}
return KEY_DELETED; return KEY_DELETED;
} }