From 0bc2cb8dc2a1540f3fee162c155d26ee169f8fd6 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com> Date: Thu, 1 Oct 2026 15:22:20 +0300 Subject: [PATCH] perf(csc): build the key list only on a miss (#4037) * perf(csc): build the key list only on a miss extractRedisKeys and the namespaced copy ran before the cache-hit check, but only Reserve uses them and Reserve is on the miss path. At a 94-99% hit rate that allocated a key slice, a namespaced slice and one string per key on nearly every read and discarded all three -- by object count about 18% of everything the client allocated. Object count is what matters here: after cache-key pooling cut allocated bytes by 30%, memclrNoHeapPointers was still 20% of CPU, because malloc and zeroing track the number of objects, not their size. Deferring the "no key list, not cacheable" gate along with it is safe: extractRedisKeys depends only on the command's shape, so a command with no key list was never cacheable and can never be a hit. The gate still runs before anything is cached. * fix(csc): check key types before the cache key Deferring the key list to the miss path also deferred the key-type check, so buildCacheKeyNS encoded the arguments first. For a key the cache cannot render, such as a BinaryMarshaler, MarshalBinary then ran twice: once for the cache key and once for the request. The key positions of every cacheable command are one contiguous span. processCached now reads that span and checks its types before it builds the cache key, without allocating, and builds the key strings only on a miss. extractRedisKeys moves to the tests. * fix(csc): reject negative key positions SetFirstKeyPos is public and takes any int8. isCacheable accepted any nonzero position and cscKeySpan rejected only 0, so a negative one let cscKeysRenderable index args[-1] and panic the process. The keyArg bounds check this replaced had treated such a command as uncacheable. Both now require a position above 0, so the command is sent normally. --- csc_commands.go | 124 ++++++++++++++++-------------------- csc_extractkeys_test.go | 20 ++++++ csc_integration.go | 47 ++++++++++---- csc_keygate_test.go | 76 ++++++++++++++++++++++ csc_keypos_negative_test.go | 35 ++++++++++ csc_keyspan_test.go | 32 ++++++++++ 6 files changed, 255 insertions(+), 79 deletions(-) create mode 100644 csc_extractkeys_test.go create mode 100644 csc_keygate_test.go create mode 100644 csc_keypos_negative_test.go create mode 100644 csc_keyspan_test.go diff --git a/csc_commands.go b/csc_commands.go index 4664181f..2b9d1d57 100644 --- a/csc_commands.go +++ b/csc_commands.go @@ -71,7 +71,7 @@ func isCacheable(cmd Cmder) bool { if cmd.Name() == "sort_ro" && sortROHasByGet(cmd) { return false } - return cmdFirstKeyPosWithInfo(cmd, nil) != 0 + return cmdFirstKeyPosWithInfo(cmd, nil) > 0 } // sortROHasByGet reports whether a SORT_RO invocation uses BY or GET @@ -200,109 +200,97 @@ func buildCacheKeyNS(cmd Cmder, ns string) (string, bool) { // cacheKeyScratchMaxCap bounds the buffer capacity kept in the pool. const cacheKeyScratchMaxCap = 64 << 10 -// keyArg renders the key argument at pos exactly as proto.Writer sends it to -// the server, so invalidation lookups match the key names in the server's -// "invalidate" pushes. Only types whose stringArg rendering is byte-identical -// to the wire encoding are accepted (fmt.Sprint of any integer matches the +// isWireKeyType reports whether a key argument of v's type renders, through +// stringArg, exactly as proto.Writer sends it to the server, so invalidation +// lookups match the key names in the server's "invalidate" pushes. Only +// byte-identical types are accepted (fmt.Sprint of any integer matches the // writer's base-10 strconv output); for anything else — pointers, bools, // times, durations, floats, BinaryMarshaler values — the rendering can // diverge, the invalidation would never match, and the entry would be served -// stale forever, so ok=false and the caller skips caching (see processCached). -func keyArg(cmd Cmder, pos int) (string, bool) { - args := cmd.Args() - if pos < 0 || pos >= len(args) { - return "", false - } - switch args[pos].(type) { +// stale forever, so the caller skips caching (see processCached). +func isWireKeyType(v any) bool { + switch v.(type) { case string, []byte, int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64: - return cmd.stringArg(pos), true + return true } - return "", false + return false } -// extractRedisKeys returns the Redis key arguments from cmd. The result lets -// the cache map incoming invalidations back to affected entries. Returns nil -// (caller skips caching) when any key -// argument cannot be rendered in its wire form (see keyArg). -func extractRedisKeys(cmd Cmder) []string { +// cscKeySpan returns the inclusive argument range [lo, hi] that holds cmd's +// Redis keys. For every cacheable command shape the keys are contiguous. +// ok is false when cmd has no key. +func cscKeySpan(cmd Cmder) (lo, hi int, ok bool) { firstKey := cmdFirstKeyPosWithInfo(cmd, nil) - if firstKey == 0 { - return nil + // SetFirstKeyPos is public and takes any int8, so a negative position is + // possible. It is not a key: reject it here rather than index args with it. + if firstKey <= 0 { + return 0, 0, false } - argsLen := len(cmd.Args()) if firstKey >= argsLen { - return nil + return 0, 0, false } switch cmd.Name() { // All remaining args from firstKeyPos are keys. case "mget", "exists", "sdiff", "sinter", "sunion": - keys := make([]string, 0, argsLen-firstKey) - for i := firstKey; i < argsLen; i++ { - k, ok := keyArg(cmd, i) - if !ok { - return nil - } - keys = append(keys, k) - } - return keys + return firstKey, argsLen - 1, true // Numkeys pattern: numkeys at args[1], keys from args[2]. case "sintercard", "zdiff", "zinter", "zunion": if argsLen < 3 { - return nil + return 0, 0, false } - numKeys, err := strconv.Atoi(cmd.stringArg(1)) - if err != nil || numKeys <= 0 { - return nil + numKeys, ok := cscNumKeys(cmd) + if !ok || numKeys <= 0 { + return 0, 0, false } - keys := make([]string, 0, numKeys) - for i := 2; i < 2+numKeys && i < argsLen; i++ { - k, ok := keyArg(cmd, i) - if !ok { - return nil - } - keys = append(keys, k) - } - return keys + return 2, min(2+numKeys, argsLen) - 1, true // LCS: exactly two consecutive keys starting at firstKeyPos. case "lcs": if firstKey+1 >= argsLen { - return nil + return 0, 0, false } - k1, ok1 := keyArg(cmd, firstKey) - k2, ok2 := keyArg(cmd, firstKey+1) - if !ok1 || !ok2 { - return nil - } - return []string{k1, k2} + return firstKey, firstKey + 1, true // JSON.MGET: keys from firstKeyPos to second-to-last (last arg is the // JSON path, not a key). case "json.mget": - lastKey := argsLen - 2 - if lastKey < firstKey { - return nil + if argsLen-2 < firstKey { + return 0, 0, false } - keys := make([]string, 0, lastKey-firstKey+1) - for i := firstKey; i <= lastKey; i++ { - k, ok := keyArg(cmd, i) - if !ok { - return nil - } - keys = append(keys, k) - } - return keys + return firstKey, argsLen - 2, true } // Single key at firstKeyPos (GET, HGET, LRANGE, ...). - k, ok := keyArg(cmd, firstKey) - if !ok { - return nil + return firstKey, firstKey, true +} + +// cscNumKeys parses the numkeys argument (args[1]). The typed commands pass +// an int, which is read without rendering it to a string. +func cscNumKeys(cmd Cmder) (int, bool) { + switch v := cmd.Args()[1].(type) { + case int: + return v, true + case int64: + return int(v), true } - return []string{k} + n, err := strconv.Atoi(cmd.stringArg(1)) + return n, err == nil +} + +// cscKeysRenderable reports whether every key in [lo, hi] is a type +// isWireKeyType accepts. It allocates nothing, so processCached runs it on every read, +// before it builds the cache key. +func cscKeysRenderable(cmd Cmder, lo, hi int) bool { + args := cmd.Args() + for i := lo; i <= hi; i++ { + if !isWireKeyType(args[i]) { + return false + } + } + return true } diff --git a/csc_extractkeys_test.go b/csc_extractkeys_test.go new file mode 100644 index 00000000..9438e749 --- /dev/null +++ b/csc_extractkeys_test.go @@ -0,0 +1,20 @@ +package redis + +// extractRedisKeys returns the Redis key arguments from cmd. The result lets +// the cache map incoming invalidations back to affected entries. Returns nil +// (caller skips caching) when any key argument cannot be rendered in its wire +// form (see isWireKeyType). +// +// Production code reads the key span in processCached and builds the strings +// only on a miss (cscMissKeys), so this helper has no non-test caller. +func extractRedisKeys(cmd Cmder) []string { + lo, hi, ok := cscKeySpan(cmd) + if !ok || !cscKeysRenderable(cmd, lo, hi) { + return nil + } + keys := make([]string, 0, hi-lo+1) + for i := lo; i <= hi; i++ { + keys = append(keys, cmd.stringArg(i)) + } + return keys +} diff --git a/csc_integration.go b/csc_integration.go index a2ab8946..e1e57abe 100644 --- a/csc_integration.go +++ b/csc_integration.go @@ -129,6 +129,21 @@ func cscNamespacedKey(prefix, key string) string { return prefix + key } +// cscMissKeys returns the namespaced redis keys in args [lo, hi] for Reserve. +// processCached has already checked the span with cscKeySpan and +// cscKeysRenderable. +// +// This is called on the MISS path only. It used to run before the cache-hit +// check, which meant a hit -- the 94-99% case -- allocated a raw key slice, +// this slice, and one string per key, then discarded all three. +func cscMissKeys(cmd Cmder, keyPrefix string, lo, hi int) []string { + ns := make([]string, 0, hi-lo+1) + for i := lo; i <= hi; i++ { + ns = append(ns, cscNamespacedKey(keyPrefix, cmd.stringArg(i))) + } + return ns +} + // invalidateHandler propagates RESP3 "invalidate" push notifications into the // shared client-side cache. keyPrefix scopes incoming key names so a shared // cache cannot collide across databases or fixed ACL identities. @@ -1538,29 +1553,36 @@ func (c *baseClient) processCached(ctx context.Context, cmd Cmder, state *proces return c.processWithRetry(ctx, cmd, nil, state, startAttempt) } - redisKeys := extractRedisKeys(cmd) - if len(redisKeys) == 0 { - // Without a key list we cannot react to invalidations for this command. - return c.processWithRetry(ctx, cmd, nil, state, startAttempt) - } - // Read the namespace BEFORE building the key: the key is now built with // the namespace already in it, in one allocation, so the prefix has to be - // known first. Both checks are cheap and neither depends on the other. + // known first. keyPrefix := c.cscKeyPrefix if keyPrefix == "" { // A successfully attached client always has a namespace. Fail closed if // an incomplete custom baseClient reaches this path. return c.processWithRetry(ctx, cmd, nil, state, startAttempt) } + // Check the key arguments BEFORE building the cache key. Building it + // encodes every argument, which calls MarshalBinary on a marshaler key; an + // uncacheable command would then marshal its key twice, once here and once + // for the request. The check allocates nothing. + keyLo, keyHi, ok := cscKeySpan(cmd) + if !ok || !cscKeysRenderable(cmd, keyLo, keyHi) { + // Without a key list we cannot react to invalidations for this + // command, so it must not be cached. + return c.processWithRetry(ctx, cmd, nil, state, startAttempt) + } key, ok := buildCacheKeyNS(cmd, keyPrefix) if !ok { return c.processWithRetry(ctx, cmd, nil, state, startAttempt) } - nsRedisKeys := make([]string, len(redisKeys)) - for i, k := range redisKeys { - nsRedisKeys[i] = cscNamespacedKey(keyPrefix, k) - } + + // The key strings are deliberately NOT built here. Only Reserve needs + // them, and Reserve is on the miss path -- so at a 94-99% hit rate this + // used to allocate a key slice, a namespaced slice and one string per key + // on every read and discard all of it. By object count that was ~18% of + // everything the client allocated. They are built in cscMissKeys below, + // after the hit check. // Serve hits straight from the cache. if data, ok := c.cscGet(ctx, key); ok { @@ -1582,6 +1604,9 @@ func (c *baseClient) processCached(ctx context.Context, cmd Cmder, state *proces // refreshes); only the early-flush latency is client-local. c.cscRefreshQueue.signalDemand(key) + // Miss path: now the key list is actually needed. + nsRedisKeys := cscMissKeys(cmd, keyPrefix, keyLo, keyHi) + token, shouldFetch := c.csc.Reserve(key, nsRedisKeys) if !shouldFetch { // Another goroutine is fetching; Get below waits until it completes. diff --git a/csc_keygate_test.go b/csc_keygate_test.go new file mode 100644 index 00000000..77b9b3e3 --- /dev/null +++ b/csc_keygate_test.go @@ -0,0 +1,76 @@ +package redis_test + +import ( + "context" + "strconv" + "testing" + "time" + + "github.com/redis/go-redis/v9" +) + +// countingKeyArg is a key argument the cache cannot render in wire form, so +// the command is uncacheable. It counts MarshalBinary calls. +type countingKeyArg struct { + key string + calls int +} + +func (a *countingKeyArg) MarshalBinary() ([]byte, error) { + a.calls++ + return []byte(a.key), nil +} + +// TestCSCUncacheableKeyMarshaledOnce pins the key-type gate order in +// processCached: a command whose key the cache cannot render must fall back +// before the cache key is built. Building the cache key encodes every +// argument, so a BinaryMarshaler key would be marshaled once for the cache +// key and again for the request. +func TestCSCUncacheableKeyMarshaledOnce(t *testing.T) { + if err := probeRedis(cscNativeAddr()); err != nil { + t.Skipf("redis not available at %s: %v", cscNativeAddr(), err) + } + cache := redis.NewLocalCache(redis.CacheConfig{MaxEntries: 32}) + c := redis.NewClient(&redis.Options{ + Addr: cscNativeAddr(), + Protocol: 3, + ClientSideCache: cache, + PoolSize: 1, + }) + t.Cleanup(func() { _ = c.Close() }) + + ctx := context.Background() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("redis not available at %s: %v", cscNativeAddr(), err) + } + plain := redis.NewClient(&redis.Options{Addr: cscNativeAddr()}) + t.Cleanup(func() { _ = plain.Close() }) + unavailable, err := probeClientTracking(ctx, plain) + if unavailable { + t.Skipf("CLIENT TRACKING is unavailable: %v", err) + } + if err != nil { + t.Fatalf("probe CLIENT TRACKING: %v", err) + } + + key := "csc-keygate:" + strconv.FormatInt(time.Now().UnixNano(), 10) + if err := plain.Set(ctx, key, "v", 0).Err(); err != nil { + t.Fatalf("SET: %v", err) + } + t.Cleanup(func() { _ = plain.Del(context.Background(), key).Err() }) + + arg := &countingKeyArg{key: key} + cmd := redis.NewStringCmd(ctx, "get", arg) + if err := c.Process(ctx, cmd); err != nil { + t.Fatalf("GET: %v", err) + } + if got := cmd.Val(); got != "v" { + t.Fatalf("GET = %q, want v", got) + } + if arg.calls != 1 { + t.Fatalf("MarshalBinary called %d times, want 1 (the request only)", arg.calls) + } + if n := cache.Len(); n != 0 { + t.Fatalf("cache.Len = %d, want 0: an unrenderable key must not be cached", n) + } +} diff --git a/csc_keypos_negative_test.go b/csc_keypos_negative_test.go new file mode 100644 index 00000000..0c6112af --- /dev/null +++ b/csc_keypos_negative_test.go @@ -0,0 +1,35 @@ +package redis + +import ( + "context" + "testing" +) + +// A cacheable command whose public key position was set to a negative value +// is not cacheable, and the key gate does not index args with it. The span +// let -1 through (only 0 was rejected), and cscKeysRenderable then read +// args[-1] and panicked the process; the old keyArg bounds check had sent such +// a command normally. +func TestCSCNegativeKeyPositionIsUncacheable(t *testing.T) { + ctx := context.Background() + get := NewStringCmd(ctx, "get", "k") + get.SetFirstKeyPos(-1) + mget := NewSliceCmd(ctx, "mget", "a", "b") + mget.SetFirstKeyPos(-3) + for _, cmd := range []Cmder{get, mget} { + if isCacheable(cmd) { + t.Errorf("%s with a negative key position reported cacheable", cmd.Name()) + } + func() { + defer func() { + if r := recover(); r != nil { + t.Errorf("%s: key gate panicked: %v", cmd.Name(), r) + } + }() + if lo, hi, ok := cscKeySpan(cmd); ok { + t.Errorf("%s: span (%d, %d) accepted for a negative key position", cmd.Name(), lo, hi) + _ = cscKeysRenderable(cmd, lo, hi) + } + }() + } +} diff --git a/csc_keyspan_test.go b/csc_keyspan_test.go new file mode 100644 index 00000000..8d4b4a91 --- /dev/null +++ b/csc_keyspan_test.go @@ -0,0 +1,32 @@ +package redis + +import ( + "context" + "testing" +) + +// TestCSCKeyGateAllocatesNothing pins that the key check processCached runs +// on every cached read, hits included, does not allocate. +func TestCSCKeyGateAllocatesNothing(t *testing.T) { + ctx := context.Background() + // A no-op cmdable builds the typed commands without any I/O. + c := cmdable(func(context.Context, Cmder) error { return nil }) + cmds := map[string]Cmder{ + "get": c.Get(ctx, "k"), + "mget": c.MGet(ctx, "a", "b", "c"), + "zinter": c.ZInter(ctx, &ZStore{Keys: []string{"a", "b"}}), + "lcs": c.LCS(ctx, &LCSQuery{Key1: "a", Key2: "b"}), + } + for name, cmd := range cmds { + lo, hi, ok := cscKeySpan(cmd) + if !ok || !cscKeysRenderable(cmd, lo, hi) { + t.Fatalf("%s: span (%d, %d, %v) not renderable", name, lo, hi, ok) + } + if n := testing.AllocsPerRun(100, func() { + lo, hi, ok := cscKeySpan(cmd) + _ = ok && cscKeysRenderable(cmd, lo, hi) + }); n != 0 { + t.Errorf("%s: key gate allocates %v objects per call, want 0", name, n) + } + } +}