Deep prefetching for hash and zset inner hashtables#3940
Open
roshkhatri wants to merge 3 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a deep prefetch path for hash/zset values: per-key argv/argc and member-key params are carried in batch state; batch init/teardown and addCommandToBatch wire per-key context; prefetchEntry/prefetchValue may go to PREFETCH_VALUE_DEEP; prefetchValueDeep runs a 3-phase inner lookup; command metadata, generator, and tests updated. ChangesDeep prefetch for hash/zset values
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
roshkhatri
force-pushed
the
prefetch-inner-hashtable
branch
from
34ba678 to
af659f3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/memory_prefetch.c (1)
212-213: 💤 Low valueConsider documenting the
argv[2]assumption.The deep prefetch relies on
argv[2]being the field/member to look up, which holds for standard single-field commands (HGET, HSETNX, ZSCORE, etc.) but not for multi-field commands (HMGET, HGETALL) or commands with different argument layouts. Theargc >= 3check filters out no-field commands, but the assumption could be made more explicit with a comment here for future maintainability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory_prefetch.c` around lines 212 - 213, Document the assumption that argv[2] is the target field used for deep-prefetch lookups: add a concise comment next to the hashtableIncrementalFindInit call (referencing hashtableIncrementalFindInit, info->inner_hashtab_state, and objectGetVal(info->client->argv[2])) explaining that this relies on single-field command layouts (e.g., HGET, HSETNX, ZSCORE), that argc >= 3 guards against no-field commands, and note that multi-field or different-arg-layout commands (HMGET, HGETALL) are not covered so future changes must adjust this usage or add additional handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/memory_prefetch.c`:
- Around line 15-26: The enum definitions PrefetchState and DeepPrefetchPhase
have spacing/alignment issues causing clang-format CI failures; run
clang-format-18 -i on the source file that contains these enums and reformat so
identifiers and trailing comments align consistently (fix spacing around commas,
comment alignment, and enum indentation) ensuring both PrefetchState and
DeepPrefetchPhase follow the project's clang-format style.
---
Nitpick comments:
In `@src/memory_prefetch.c`:
- Around line 212-213: Document the assumption that argv[2] is the target field
used for deep-prefetch lookups: add a concise comment next to the
hashtableIncrementalFindInit call (referencing hashtableIncrementalFindInit,
info->inner_hashtab_state, and objectGetVal(info->client->argv[2])) explaining
that this relies on single-field command layouts (e.g., HGET, HSETNX, ZSCORE),
that argc >= 3 guards against no-field commands, and note that multi-field or
different-arg-layout commands (HMGET, HGETALL) are not covered so future changes
must adjust this usage or add additional handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b95dce8-f5e0-431d-9020-99a097cd82a3
📒 Files selected for processing (1)
src/memory_prefetch.c
roshkhatri
force-pushed
the
prefetch-inner-hashtable
branch
from
af659f3 to
64f0e51
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/memory_prefetch.c (1)
380-389:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse-after-free:
key_clientsentries not cleared when client is removed.When a client is removed from the batch, only
batch->clients[i]is set toNULL, butbatch->key_clients[j]entries for that client's keys remain as dangling pointers. Later,initBatchInfocopies these dangling pointers toinfo->client, andprefetchEntry/prefetchValueDeepdereference them.Crash scenario:
- Client A adds keys →
key_clients[0] = A- Client A disconnects before batch processes
removeClientFromPendingCommandsBatch(A)setsclients[idx] = NULLkey_clients[0]still points to freed client AprefetchValueDeepaccessesinfo->client->argv[2]→ use-after-free🐛 Proposed fix: clear key_clients when removing client
/* Removes the given client from the pending prefetch batch, if present. */ void removeClientFromPendingCommandsBatch(client *c) { if (!batch) return; for (size_t i = 0; i < batch->client_count; i++) { if (batch->clients[i] == c) { batch->clients[i] = NULL; - return; + break; } } + + /* Clear key_clients entries pointing to this client to prevent dangling pointers */ + for (size_t i = 0; i < batch->key_count; i++) { + if (batch->key_clients[i] == c) { + batch->key_clients[i] = NULL; + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory_prefetch.c` around lines 380 - 389, removeClientFromPendingCommandsBatch currently only nulls batch->clients[i], leaving dangling pointers in batch->key_clients which later get copied into info->client and dereferenced in initBatchInfo / prefetchValueDeep; update removeClientFromPendingCommandsBatch so after finding the client in batch->clients it also scans all key-related client pointers in the batch (the batch->key_clients arrays / per-key client lists) and sets any entry equal to c to NULL (and adjust any per-key client counts if the batch structure tracks them) to prevent use-after-free; locate and modify removeClientFromPendingCommandsBatch and ensure this clearing mirrors how initBatchInfo expects key_clients to be populated.
🧹 Nitpick comments (1)
src/memory_prefetch.c (1)
228-231: 💤 Low valueUnreachable code after switch statement.
All cases in the switch statement return, making line 230 unreachable. Consider removing the dead code.
♻️ Proposed fix
/* Done - inner entry is now prefetched */ markKeyAsdone(info); return; } - - markKeyAsdone(info); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory_prefetch.c` around lines 228 - 231, The call to markKeyAsdone(info) after the switch in memory_prefetch.c is dead code because every case in the preceding switch returns; remove the unreachable call (markKeyAsdone(info)) or, if it must always run, move that call to a common pre-return location (e.g., immediately before the switch or into each case where appropriate). Update the switch handling in the function containing markKeyAsdone to ensure the function's control flow still marks keys correctly without leaving unreachable statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/memory_prefetch.c`:
- Around line 380-389: removeClientFromPendingCommandsBatch currently only nulls
batch->clients[i], leaving dangling pointers in batch->key_clients which later
get copied into info->client and dereferenced in initBatchInfo /
prefetchValueDeep; update removeClientFromPendingCommandsBatch so after finding
the client in batch->clients it also scans all key-related client pointers in
the batch (the batch->key_clients arrays / per-key client lists) and sets any
entry equal to c to NULL (and adjust any per-key client counts if the batch
structure tracks them) to prevent use-after-free; locate and modify
removeClientFromPendingCommandsBatch and ensure this clearing mirrors how
initBatchInfo expects key_clients to be populated.
---
Nitpick comments:
In `@src/memory_prefetch.c`:
- Around line 228-231: The call to markKeyAsdone(info) after the switch in
memory_prefetch.c is dead code because every case in the preceding switch
returns; remove the unreachable call (markKeyAsdone(info)) or, if it must always
run, move that call to a common pre-return location (e.g., immediately before
the switch or into each case where appropriate). Update the switch handling in
the function containing markKeyAsdone to ensure the function's control flow
still marks keys correctly without leaving unreachable statements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7d83e50-7164-4991-8281-bcb8c3489cc0
📒 Files selected for processing (1)
src/memory_prefetch.c
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/memory_prefetch.c`:
- Around line 332-340: The code stores only the owning client in
batch->key_clients inside addCommandToBatch, but prefetchValueDeep reads argv
from info->client (e.g., info->client->argv[2]) which is wrong for
queued/pipelined commands; thread the per-key lookup argument through
KeyPrefetchInfo (add a key_lookup_args[] entry populated from
argv[result.keys[i].pos] in addCommandToBatch) and update KeyPrefetchInfo to
carry that lookup arg, then change prefetchValueDeep to use info->key_lookup_arg
(or info->key_lookup_args[i]) instead of info->client->argv[...] and ensure all
code paths that build batches (including the similar block at 358-366) populate
the new field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 32edbd86-b64a-4432-a1ee-f0d8e4ed6e15
📒 Files selected for processing (1)
src/memory_prefetch.c
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #3940 +/- ##
============================================
- Coverage 76.68% 76.63% -0.06%
============================================
Files 162 162
Lines 80731 80807 +76
============================================
+ Hits 61910 61924 +14
- Misses 18821 18883 +62
🚀 New features to boost your workflow:
|
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
roshkhatri
force-pushed
the
prefetch-inner-hashtable
branch
from
ebe4125 to
44b4763
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/server.h (1)
2699-2703: ⚡ Quick winClarify what
member_key_count == 0resolves to.
memory_prefetch.ccurrently normalizes0to-1, so omitted/default metadata behaves as “prefetch all remaining members.” This header still documents0as “use default,” which leaves the public contract ambiguous for future command specs and generator output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.h` around lines 2699 - 2703, Update the comment for member_key_count to reflect actual normalized behavior: explain that a value of 0 is normalized by memory_prefetch.c to -1 and therefore means "prefetch all remaining members" (not just "use default"), and mention the normalization occurs in memory_prefetch.c so generators/command specs know 0 is equivalent to -1 at runtime; reference the field name member_key_count and the normalization in memory_prefetch.c in the comment for clarity.src/commands/zscore.json (1)
8-8: 💤 Low valueConsider adding
member_key_count: 1for consistency.While the current implementation works correctly (the argc check prevents processing beyond the single member), other single-member commands like
ZINCRBYandZREVRANKexplicitly setmember_key_count: 1. Adding it here would make the intent clearer and align with those commands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/zscore.json` at line 8, The command spec in src/commands/zscore.json currently sets "member_key_index": 2 but omits "member_key_count", so add "member_key_count": 1 alongside "member_key_index": 2 to explicitly declare that ZSCORE accepts a single member argument (matching patterns used by ZINCRBY/ZREVRANK) and keep the command metadata consistent.utils/generate-command-code.py (1)
499-504: ⚖️ Poor tradeoffConsider adding validation for member_key_ fields.*
The generator emits
member_key_index,member_key_step, andmember_key_countwithout validating that:
member_key_indexis within the command's arity boundsmember_key_stepis positive (if present)member_key_countis positive or -1 (if present)While runtime behavior will surface incorrect values during testing, adding generator-time validation could catch configuration errors earlier.
Example validation logic
if self.desc.get("member_key_index"): + idx = self.desc["member_key_index"] + if self.desc["arity"] > 0 and idx >= self.desc["arity"]: + print("Warning: %s member_key_index=%d exceeds arity=%d" % ( + self.fullname(), idx, self.desc["arity"])) + if idx < 1: + print("Error: %s member_key_index=%d must be >= 1" % (self.fullname(), idx)) + exit(1) s += ".member_key_index=%d," % self.desc["member_key_index"] if self.desc.get("member_key_step"): + step = self.desc["member_key_step"] + if step < 1: + print("Error: %s member_key_step=%d must be >= 1" % (self.fullname(), step)) + exit(1) s += ".member_key_step=%d," % self.desc["member_key_step"] if self.desc.get("member_key_count"): + count = self.desc["member_key_count"] + if count < -1 or count == 0: + print("Error: %s member_key_count=%d must be -1 or >= 1" % (self.fullname(), count)) + exit(1) s += ".member_key_count=%d," % self.desc["member_key_count"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/generate-command-code.py` around lines 499 - 504, Validate member_key_* fields before emitting them: check that if self.desc contains "member_key_index" its value is >=0 and < (self.desc["arity"] or self.desc.get("num_args") or raise), that "member_key_step" if present is >0, and that "member_key_count" if present is >0 or == -1; perform these checks in the same generation block where you read self.desc and append to s (the code emitting lines with .member_key_index/.member_key_step/.member_key_count), and raise a clear ValueError (or log and skip emission) with a message referencing the failing field and the command arity so misconfigurations are caught at generator time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands.def`:
- Around line 12053-12077: The GEO commands (GEODIST, GEOHASH, GEOPOS,
GEORADIUSBYMEMBER, GEORADIUSBYMEMBER_RO) lack the new member_key_* metadata so
they miss the encoding-based/deep-prefetch path; update their MAKE_CMD entries
to include member_key_index=<appropriate arg index> and member_key_count=1
(matching the sorted-set member lookups like ZMSCORE/ZRANK) so these
fixed-position GEO member lookups are covered by the same prefetch logic.
---
Nitpick comments:
In `@src/commands/zscore.json`:
- Line 8: The command spec in src/commands/zscore.json currently sets
"member_key_index": 2 but omits "member_key_count", so add "member_key_count": 1
alongside "member_key_index": 2 to explicitly declare that ZSCORE accepts a
single member argument (matching patterns used by ZINCRBY/ZREVRANK) and keep the
command metadata consistent.
In `@src/server.h`:
- Around line 2699-2703: Update the comment for member_key_count to reflect
actual normalized behavior: explain that a value of 0 is normalized by
memory_prefetch.c to -1 and therefore means "prefetch all remaining members"
(not just "use default"), and mention the normalization occurs in
memory_prefetch.c so generators/command specs know 0 is equivalent to -1 at
runtime; reference the field name member_key_count and the normalization in
memory_prefetch.c in the comment for clarity.
In `@utils/generate-command-code.py`:
- Around line 499-504: Validate member_key_* fields before emitting them: check
that if self.desc contains "member_key_index" its value is >=0 and <
(self.desc["arity"] or self.desc.get("num_args") or raise), that
"member_key_step" if present is >0, and that "member_key_count" if present is >0
or == -1; perform these checks in the same generation block where you read
self.desc and append to s (the code emitting lines with
.member_key_index/.member_key_step/.member_key_count), and raise a clear
ValueError (or log and skip emission) with a message referencing the failing
field and the command arity so misconfigurations are caught at generator time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a598238-d3a9-4746-97ab-bc51834e0594
📒 Files selected for processing (28)
src/cli_commands.hsrc/commands.defsrc/commands/hdel.jsonsrc/commands/hexists.jsonsrc/commands/hexpiretime.jsonsrc/commands/hget.jsonsrc/commands/hgetdel.jsonsrc/commands/hincrby.jsonsrc/commands/hincrbyfloat.jsonsrc/commands/hmget.jsonsrc/commands/hmset.jsonsrc/commands/hpersist.jsonsrc/commands/hpexpiretime.jsonsrc/commands/hpttl.jsonsrc/commands/hset.jsonsrc/commands/hsetnx.jsonsrc/commands/hstrlen.jsonsrc/commands/httl.jsonsrc/commands/zincrby.jsonsrc/commands/zmscore.jsonsrc/commands/zrank.jsonsrc/commands/zrem.jsonsrc/commands/zrevrank.jsonsrc/commands/zscore.jsonsrc/memory_prefetch.csrc/server.htests/unit/deep-prefetch.tclutils/generate-command-code.py
✅ Files skipped from review due to trivial changes (8)
- src/commands/hmset.json
- src/commands/zrank.json
- src/commands/hexpiretime.json
- src/commands/hmget.json
- src/commands/hincrby.json
- src/commands/hset.json
- src/commands/httl.json
- src/commands/hsetnx.json
Comment on lines
+12053
to
+12077
| {MAKE_CMD("zincrby","Increments the score of a member in a sorted set.","O(log(N)) where N is the number of elements in the sorted set.","1.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZINCRBY_History,0,ZINCRBY_Tips,0,zincrbyCommand,4,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZINCRBY_Keyspecs,1,NULL,3),.args=ZINCRBY_Args,.member_key_index=3,.member_key_count=1}, | ||
| {MAKE_CMD("zinter","Returns the intersect of multiple sorted sets.","O(N*K)+O(M*log(M)) worst case with N being the smallest input sorted set, K being the number of input sorted sets and M being the number of elements in the resulting sorted set.","6.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZINTER_History,0,ZINTER_Tips,0,zinterCommand,-3,CMD_READONLY,ACL_CATEGORY_READ|ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET,NULL,ZINTER_Keyspecs,1,zunionInterDiffGetKeys,5),.args=ZINTER_Args}, | ||
| {MAKE_CMD("zintercard","Returns the number of members of the intersect of multiple sorted sets.","O(N*K) worst case with N being the smallest input sorted set, K being the number of input sorted sets.","7.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZINTERCARD_History,0,ZINTERCARD_Tips,0,zinterCardCommand,-3,CMD_READONLY,ACL_CATEGORY_READ|ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET,NULL,ZINTERCARD_Keyspecs,1,zunionInterDiffGetKeys,3),.args=ZINTERCARD_Args}, | ||
| {MAKE_CMD("zinterstore","Stores the intersect of multiple sorted sets in a key.","O(N*K)+O(M*log(M)) worst case with N being the smallest input sorted set, K being the number of input sorted sets and M being the number of elements in the resulting sorted set.","2.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZINTERSTORE_History,0,ZINTERSTORE_Tips,0,zinterstoreCommand,-4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZINTERSTORE_Keyspecs,2,zunionInterDiffStoreGetKeys,5),.args=ZINTERSTORE_Args}, | ||
| {MAKE_CMD("zlexcount","Returns the number of members in a sorted set within a lexicographical range.","O(log(N)) with N being the number of elements in the sorted set.","2.8.9",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZLEXCOUNT_History,0,ZLEXCOUNT_Tips,0,zlexcountCommand,4,CMD_READONLY|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_READ|ACL_CATEGORY_SORTEDSET,NULL,ZLEXCOUNT_Keyspecs,1,NULL,3),.args=ZLEXCOUNT_Args}, | ||
| {MAKE_CMD("zmpop","Returns the highest- or lowest-scoring members from one or more sorted sets after removing them. Deletes the sorted set if the last member was popped.","O(K) + O(M*log(N)) where K is the number of provided keys, N being the number of elements in the sorted set, and M being the number of elements popped.","7.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZMPOP_History,0,ZMPOP_Tips,0,zmpopCommand,-4,CMD_WRITE,ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZMPOP_Keyspecs,1,zmpopGetKeys,4),.args=ZMPOP_Args}, | ||
| {MAKE_CMD("zmscore","Returns the score of one or more members in a sorted set.","O(N) where N is the number of members being requested.","6.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZMSCORE_History,0,ZMSCORE_Tips,0,zmscoreCommand,-3,CMD_READONLY|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_READ|ACL_CATEGORY_SORTEDSET,NULL,ZMSCORE_Keyspecs,1,NULL,2),.args=ZMSCORE_Args}, | ||
| {MAKE_CMD("zmscore","Returns the score of one or more members in a sorted set.","O(N) where N is the number of members being requested.","6.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZMSCORE_History,0,ZMSCORE_Tips,0,zmscoreCommand,-3,CMD_READONLY|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_READ|ACL_CATEGORY_SORTEDSET,NULL,ZMSCORE_Keyspecs,1,NULL,2),.args=ZMSCORE_Args,.member_key_index=2}, | ||
| {MAKE_CMD("zpopmax","Returns the highest-scoring members from a sorted set after removing them. Deletes the sorted set if the last member was popped.","O(log(N)*M) with N being the number of elements in the sorted set, and M being the number of elements popped.","5.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZPOPMAX_History,0,ZPOPMAX_Tips,0,zpopmaxCommand,-2,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZPOPMAX_Keyspecs,1,NULL,2),.args=ZPOPMAX_Args}, | ||
| {MAKE_CMD("zpopmin","Returns the lowest-scoring members from a sorted set after removing them. Deletes the sorted set if the last member was popped.","O(log(N)*M) with N being the number of elements in the sorted set, and M being the number of elements popped.","5.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZPOPMIN_History,0,ZPOPMIN_Tips,0,zpopminCommand,-2,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZPOPMIN_Keyspecs,1,NULL,2),.args=ZPOPMIN_Args}, | ||
| {MAKE_CMD("zrandmember","Returns one or more random members from a sorted set.","O(N) where N is the number of members returned","6.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZRANDMEMBER_History,0,ZRANDMEMBER_Tips,1,zrandmemberCommand,-2,CMD_READONLY,ACL_CATEGORY_READ|ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET,NULL,ZRANDMEMBER_Keyspecs,1,NULL,2),.args=ZRANDMEMBER_Args}, | ||
| {MAKE_CMD("zrange","Returns members in a sorted set within a range of indexes.","O(log(N)+M) with N being the number of elements in the sorted set and M the number of elements returned.","1.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZRANGE_History,1,ZRANGE_Tips,0,zrangeCommand,-4,CMD_READONLY,ACL_CATEGORY_READ|ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET,NULL,ZRANGE_Keyspecs,1,NULL,7),.args=ZRANGE_Args}, | ||
| {MAKE_CMD("zrangebylex","Returns members in a sorted set within a lexicographical range.","O(log(N)+M) with N being the number of elements in the sorted set and M the number of elements being returned. If M is constant (e.g. always asking for the first 10 elements with LIMIT), you can consider it O(log(N)).","2.8.9",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZRANGEBYLEX_History,0,ZRANGEBYLEX_Tips,0,zrangebylexCommand,-4,CMD_READONLY,ACL_CATEGORY_READ|ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET,NULL,ZRANGEBYLEX_Keyspecs,1,NULL,4),.args=ZRANGEBYLEX_Args}, | ||
| {MAKE_CMD("zrangebyscore","Returns members in a sorted set within a range of scores.","O(log(N)+M) with N being the number of elements in the sorted set and M the number of elements being returned. If M is constant (e.g. always asking for the first 10 elements with LIMIT), you can consider it O(log(N)).","1.0.5",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZRANGEBYSCORE_History,1,ZRANGEBYSCORE_Tips,0,zrangebyscoreCommand,-4,CMD_READONLY,ACL_CATEGORY_READ|ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET,NULL,ZRANGEBYSCORE_Keyspecs,1,NULL,5),.args=ZRANGEBYSCORE_Args}, | ||
| {MAKE_CMD("zrangestore","Stores a range of members from sorted set in a key.","O(log(N)+M) with N being the number of elements in the sorted set and M the number of elements stored into the destination key.","6.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZRANGESTORE_History,0,ZRANGESTORE_Tips,0,zrangestoreCommand,-5,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZRANGESTORE_Keyspecs,2,NULL,7),.args=ZRANGESTORE_Args}, | ||
| {MAKE_CMD("zrank","Returns the index of a member in a sorted set ordered by ascending scores.","O(log(N))","2.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZRANK_History,1,ZRANK_Tips,0,zrankCommand,-3,CMD_READONLY|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_READ|ACL_CATEGORY_SORTEDSET,NULL,ZRANK_Keyspecs,1,NULL,3),.args=ZRANK_Args}, | ||
| {MAKE_CMD("zrem","Removes one or more members from a sorted set. Deletes the sorted set if all members were removed.","O(M*log(N)) with N being the number of elements in the sorted set and M the number of elements to be removed.","1.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZREM_History,1,ZREM_Tips,0,zremCommand,-3,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZREM_Keyspecs,1,NULL,2),.args=ZREM_Args}, | ||
| {MAKE_CMD("zrank","Returns the index of a member in a sorted set ordered by ascending scores.","O(log(N))","2.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZRANK_History,1,ZRANK_Tips,0,zrankCommand,-3,CMD_READONLY|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_READ|ACL_CATEGORY_SORTEDSET,NULL,ZRANK_Keyspecs,1,NULL,3),.args=ZRANK_Args,.member_key_index=2,.member_key_count=1}, | ||
| {MAKE_CMD("zrem","Removes one or more members from a sorted set. Deletes the sorted set if all members were removed.","O(M*log(N)) with N being the number of elements in the sorted set and M the number of elements to be removed.","1.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZREM_History,1,ZREM_Tips,0,zremCommand,-3,CMD_WRITE|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZREM_Keyspecs,1,NULL,2),.args=ZREM_Args,.member_key_index=2}, | ||
| {MAKE_CMD("zremrangebylex","Removes members in a sorted set within a lexicographical range. Deletes the sorted set if all members were removed.","O(log(N)+M) with N being the number of elements in the sorted set and M the number of elements removed by the operation.","2.8.9",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZREMRANGEBYLEX_History,0,ZREMRANGEBYLEX_Tips,0,zremrangebylexCommand,4,CMD_WRITE,ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZREMRANGEBYLEX_Keyspecs,1,NULL,3),.args=ZREMRANGEBYLEX_Args}, | ||
| {MAKE_CMD("zremrangebyrank","Removes members in a sorted set within a range of indexes. Deletes the sorted set if all members were removed.","O(log(N)+M) with N being the number of elements in the sorted set and M the number of elements removed by the operation.","2.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZREMRANGEBYRANK_History,0,ZREMRANGEBYRANK_Tips,0,zremrangebyrankCommand,4,CMD_WRITE,ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZREMRANGEBYRANK_Keyspecs,1,NULL,3),.args=ZREMRANGEBYRANK_Args}, | ||
| {MAKE_CMD("zremrangebyscore","Removes members in a sorted set within a range of scores. Deletes the sorted set if all members were removed.","O(log(N)+M) with N being the number of elements in the sorted set and M the number of elements removed by the operation.","1.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZREMRANGEBYSCORE_History,0,ZREMRANGEBYSCORE_Tips,0,zremrangebyscoreCommand,4,CMD_WRITE,ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_WRITE,NULL,ZREMRANGEBYSCORE_Keyspecs,1,NULL,3),.args=ZREMRANGEBYSCORE_Args}, | ||
| {MAKE_CMD("zrevrange","Returns members in a sorted set within a range of indexes in reverse order.","O(log(N)+M) with N being the number of elements in the sorted set and M the number of elements returned.","1.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZREVRANGE_History,0,ZREVRANGE_Tips,0,zrevrangeCommand,-4,CMD_READONLY,ACL_CATEGORY_READ|ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET,NULL,ZREVRANGE_Keyspecs,1,NULL,4),.args=ZREVRANGE_Args}, | ||
| {MAKE_CMD("zrevrangebylex","Returns members in a sorted set within a lexicographical range in reverse order.","O(log(N)+M) with N being the number of elements in the sorted set and M the number of elements being returned. If M is constant (e.g. always asking for the first 10 elements with LIMIT), you can consider it O(log(N)).","2.8.9",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZREVRANGEBYLEX_History,0,ZREVRANGEBYLEX_Tips,0,zrevrangebylexCommand,-4,CMD_READONLY,ACL_CATEGORY_READ|ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET,NULL,ZREVRANGEBYLEX_Keyspecs,1,NULL,4),.args=ZREVRANGEBYLEX_Args}, | ||
| {MAKE_CMD("zrevrangebyscore","Returns members in a sorted set within a range of scores in reverse order.","O(log(N)+M) with N being the number of elements in the sorted set and M the number of elements being returned. If M is constant (e.g. always asking for the first 10 elements with LIMIT), you can consider it O(log(N)).","2.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZREVRANGEBYSCORE_History,1,ZREVRANGEBYSCORE_Tips,0,zrevrangebyscoreCommand,-4,CMD_READONLY,ACL_CATEGORY_READ|ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET,NULL,ZREVRANGEBYSCORE_Keyspecs,1,NULL,5),.args=ZREVRANGEBYSCORE_Args}, | ||
| {MAKE_CMD("zrevrank","Returns the index of a member in a sorted set ordered by descending scores.","O(log(N))","2.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZREVRANK_History,1,ZREVRANK_Tips,0,zrevrankCommand,-3,CMD_READONLY|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_READ|ACL_CATEGORY_SORTEDSET,NULL,ZREVRANK_Keyspecs,1,NULL,3),.args=ZREVRANK_Args}, | ||
| {MAKE_CMD("zrevrank","Returns the index of a member in a sorted set ordered by descending scores.","O(log(N))","2.0.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZREVRANK_History,1,ZREVRANK_Tips,0,zrevrankCommand,-3,CMD_READONLY|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_READ|ACL_CATEGORY_SORTEDSET,NULL,ZREVRANK_Keyspecs,1,NULL,3),.args=ZREVRANK_Args,.member_key_index=2,.member_key_count=1}, | ||
| {MAKE_CMD("zscan","Iterates over members and scores of a sorted set.","O(1) for every call. O(N) for a complete iteration, including enough command calls for the cursor to return back to 0. N is the number of elements inside the collection.","2.8.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZSCAN_History,1,ZSCAN_Tips,1,zscanCommand,-3,CMD_READONLY,ACL_CATEGORY_READ|ACL_CATEGORY_SLOW|ACL_CATEGORY_SORTEDSET,NULL,ZSCAN_Keyspecs,1,NULL,5),.args=ZSCAN_Args}, | ||
| {MAKE_CMD("zscore","Returns the score of a member in a sorted set.","O(1)","1.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZSCORE_History,0,ZSCORE_Tips,0,zscoreCommand,3,CMD_READONLY|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_READ|ACL_CATEGORY_SORTEDSET,NULL,ZSCORE_Keyspecs,1,NULL,2),.args=ZSCORE_Args}, | ||
| {MAKE_CMD("zscore","Returns the score of a member in a sorted set.","O(1)","1.2.0",CMD_DOC_NONE,NULL,NULL,"sorted_set",COMMAND_GROUP_SORTED_SET,ZSCORE_History,0,ZSCORE_Tips,0,zscoreCommand,3,CMD_READONLY|CMD_FAST,ACL_CATEGORY_FAST|ACL_CATEGORY_READ|ACL_CATEGORY_SORTEDSET,NULL,ZSCORE_Keyspecs,1,NULL,2),.args=ZSCORE_Args,.member_key_index=2}, |
There was a problem hiding this comment.
Cover GEO member lookups with the same member_key_* metadata.
This new metadata makes the deep-prefetch path work for sorted-set member lookups, but the fixed-position GEO member lookups in the same table (GEODIST, GEOHASH, GEOPOS, GEORADIUSBYMEMBER, and GEORADIUSBYMEMBER_RO) still have no member_key_* annotation. Those commands hit the same zset skiplist/dict encoding, so they currently miss the new “encoding-based” prefetch path.
Suggested follow-up in the generated table
-{MAKE_CMD("geodist","Returns the distance between two members of a geospatial index.","O(1)","3.2.0",CMD_DOC_NONE,NULL,NULL,"geo",COMMAND_GROUP_GEO,GEODIST_History,0,GEODIST_Tips,0,geodistCommand,-4,CMD_READONLY,ACL_CATEGORY_GEO|ACL_CATEGORY_READ|ACL_CATEGORY_SLOW,NULL,GEODIST_Keyspecs,1,NULL,4),.args=GEODIST_Args},
+{MAKE_CMD("geodist","Returns the distance between two members of a geospatial index.","O(1)","3.2.0",CMD_DOC_NONE,NULL,NULL,"geo",COMMAND_GROUP_GEO,GEODIST_History,0,GEODIST_Tips,0,geodistCommand,-4,CMD_READONLY,ACL_CATEGORY_GEO|ACL_CATEGORY_READ|ACL_CATEGORY_SLOW,NULL,GEODIST_Keyspecs,1,NULL,4),.args=GEODIST_Args,.member_key_index=2,.member_key_count=2},
-{MAKE_CMD("geohash","Returns members from a geospatial index as geohash strings.","O(1) for each member requested.","3.2.0",CMD_DOC_NONE,NULL,NULL,"geo",COMMAND_GROUP_GEO,GEOHASH_History,0,GEOHASH_Tips,0,geohashCommand,-2,CMD_READONLY,ACL_CATEGORY_GEO|ACL_CATEGORY_READ|ACL_CATEGORY_SLOW,NULL,GEOHASH_Keyspecs,1,NULL,2),.args=GEOHASH_Args},
+{MAKE_CMD("geohash","Returns members from a geospatial index as geohash strings.","O(1) for each member requested.","3.2.0",CMD_DOC_NONE,NULL,NULL,"geo",COMMAND_GROUP_GEO,GEOHASH_History,0,GEOHASH_Tips,0,geohashCommand,-2,CMD_READONLY,ACL_CATEGORY_GEO|ACL_CATEGORY_READ|ACL_CATEGORY_SLOW,NULL,GEOHASH_Keyspecs,1,NULL,2),.args=GEOHASH_Args,.member_key_index=2},
-{MAKE_CMD("geopos","Returns the longitude and latitude of members from a geospatial index.","O(1) for each member requested.","3.2.0",CMD_DOC_NONE,NULL,NULL,"geo",COMMAND_GROUP_GEO,GEOPOS_History,0,GEOPOS_Tips,0,geoposCommand,-2,CMD_READONLY,ACL_CATEGORY_GEO|ACL_CATEGORY_READ|ACL_CATEGORY_SLOW,NULL,GEOPOS_Keyspecs,1,NULL,2),.args=GEOPOS_Args},
+{MAKE_CMD("geopos","Returns the longitude and latitude of members from a geospatial index.","O(1) for each member requested.","3.2.0",CMD_DOC_NONE,NULL,NULL,"geo",COMMAND_GROUP_GEO,GEOPOS_History,0,GEOPOS_Tips,0,geoposCommand,-2,CMD_READONLY,ACL_CATEGORY_GEO|ACL_CATEGORY_READ|ACL_CATEGORY_SLOW,NULL,GEOPOS_Keyspecs,1,NULL,2),.args=GEOPOS_Args,.member_key_index=2},
-{MAKE_CMD("georadiusbymember","Queries a geospatial index for members within a distance from a member, optionally stores the result.","O(N+log(M)) where N is the number of elements inside the bounding box of the circular area delimited by center and radius and M is the number of items inside the index.","3.2.0",CMD_DOC_NONE,NULL,NULL,"geo",COMMAND_GROUP_GEO,GEORADIUSBYMEMBER_History,2,GEORADIUSBYMEMBER_Tips,0,georadiusbymemberCommand,-5,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_GEO|ACL_CATEGORY_SLOW|ACL_CATEGORY_WRITE,NULL,GEORADIUSBYMEMBER_Keyspecs,3,georadiusGetKeys,10),.args=GEORADIUSBYMEMBER_Args},
+{MAKE_CMD("georadiusbymember","Queries a geospatial index for members within a distance from a member, optionally stores the result.","O(N+log(M)) where N is the number of elements inside the bounding box of the circular area delimited by center and radius and M is the number of items inside the index.","3.2.0",CMD_DOC_NONE,NULL,NULL,"geo",COMMAND_GROUP_GEO,GEORADIUSBYMEMBER_History,2,GEORADIUSBYMEMBER_Tips,0,georadiusbymemberCommand,-5,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_GEO|ACL_CATEGORY_SLOW|ACL_CATEGORY_WRITE,NULL,GEORADIUSBYMEMBER_Keyspecs,3,georadiusGetKeys,10),.args=GEORADIUSBYMEMBER_Args,.member_key_index=2,.member_key_count=1},
-{MAKE_CMD("georadiusbymember_ro","Returns members from a geospatial index that are within a distance from a member.","O(N+log(M)) where N is the number of elements inside the bounding box of the circular area delimited by center and radius and M is the number of items inside the index.","3.2.10",CMD_DOC_NONE,NULL,NULL,"geo",COMMAND_GROUP_GEO,GEORADIUSBYMEMBER_RO_History,2,GEORADIUSBYMEMBER_RO_Tips,0,georadiusbymemberroCommand,-5,CMD_READONLY,ACL_CATEGORY_GEO|ACL_CATEGORY_READ|ACL_CATEGORY_SLOW,NULL,GEORADIUSBYMEMBER_RO_Keyspecs,1,NULL,9),.args=GEORADIUSBYMEMBER_RO_Args},
+{MAKE_CMD("georadiusbymember_ro","Returns members from a geospatial index that are within a distance from a member.","O(N+log(M)) where N is the number of elements inside the bounding box of the circular area delimited by center and radius and M is the number of items inside the index.","3.2.10",CMD_DOC_NONE,NULL,NULL,"geo",COMMAND_GROUP_GEO,GEORADIUSBYMEMBER_RO_History,2,GEORADIUSBYMEMBER_RO_Tips,0,georadiusbymemberroCommand,-5,CMD_READONLY,ACL_CATEGORY_GEO|ACL_CATEGORY_READ|ACL_CATEGORY_SLOW,NULL,GEORADIUSBYMEMBER_RO_Keyspecs,1,NULL,9),.args=GEORADIUSBYMEMBER_RO_Args,.member_key_index=2,.member_key_count=1},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands.def` around lines 12053 - 12077, The GEO commands (GEODIST,
GEOHASH, GEOPOS, GEORADIUSBYMEMBER, GEORADIUSBYMEMBER_RO) lack the new
member_key_* metadata so they miss the encoding-based/deep-prefetch path; update
their MAKE_CMD entries to include member_key_index=<appropriate arg index> and
member_key_count=1 (matching the sorted-set member lookups like ZMSCORE/ZRANK)
so these fixed-position GEO member lookups are covered by the same prefetch
logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/memory_prefetch.c`:
- Around line 379-381: The code collapses cmd->member_key_count==0 into -1;
change the assignment so zero is preserved instead of being treated as "all
remaining." Replace the ternary expression in memory_prefetch.c that sets
batch->key_member_counts[batch->key_count] (currently using
cmd->member_key_count ? cmd->member_key_count : -1) with a direct assignment of
cmd->member_key_count (i.e., batch->key_member_counts[batch->key_count] =
cmd->member_key_count) so that both 0 (default) and -1 (walk all remaining)
remain distinct.
- Around line 377-381: prefetchCommands() currently only warms client->argv but
doesn't warm queued command argv/member objects saved into the batch arrays, so
update prefetchCommands() to iterate the batch entries (use batch->key_count and
the arrays batch->key_argv, batch->key_argc, batch->key_member_indices,
batch->key_member_steps, batch->key_member_counts) and warm each queued argv
entry the same way client->argv is warmed (touch argv[j]->ptr); if
key_member_indices is set, also compute the member slot for each argument as
key_member_indices + j*key_member_steps and warm the member object up to
key_member_counts bounds. Ensure this warming happens before prefetchValueDeep()
is expected to access info->argv[current_field_idx] so queued entries arrive
with their member objects and argv[j]->ptr already hot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c4d2efd0-53ee-4336-88c5-0990cae96c8e
📒 Files selected for processing (28)
src/cli_commands.hsrc/commands.defsrc/commands/hdel.jsonsrc/commands/hexists.jsonsrc/commands/hexpiretime.jsonsrc/commands/hget.jsonsrc/commands/hgetdel.jsonsrc/commands/hincrby.jsonsrc/commands/hincrbyfloat.jsonsrc/commands/hmget.jsonsrc/commands/hmset.jsonsrc/commands/hpersist.jsonsrc/commands/hpexpiretime.jsonsrc/commands/hpttl.jsonsrc/commands/hset.jsonsrc/commands/hsetnx.jsonsrc/commands/hstrlen.jsonsrc/commands/httl.jsonsrc/commands/zincrby.jsonsrc/commands/zmscore.jsonsrc/commands/zrank.jsonsrc/commands/zrem.jsonsrc/commands/zrevrank.jsonsrc/commands/zscore.jsonsrc/memory_prefetch.csrc/server.htests/unit/deep-prefetch.tclutils/generate-command-code.py
✅ Files skipped from review due to trivial changes (8)
- src/commands/hdel.json
- src/commands/zrank.json
- src/commands/hgetdel.json
- src/commands/zmscore.json
- src/commands/hsetnx.json
- src/commands/hstrlen.json
- src/commands/hmset.json
- tests/unit/deep-prefetch.tcl
🚧 Files skipped from review as they are similar to previous changes (18)
- src/commands/hexpiretime.json
- src/commands/hpersist.json
- src/commands/zrevrank.json
- src/commands/hset.json
- src/commands/hincrbyfloat.json
- src/commands/hincrby.json
- src/commands/zincrby.json
- src/commands/hmget.json
- src/commands/zscore.json
- src/commands/httl.json
- src/commands/hexists.json
- src/commands/hpexpiretime.json
- src/commands/hpttl.json
- src/commands/hget.json
- utils/generate-command-code.py
- src/server.h
- src/commands.def
- src/cli_commands.h
Comment on lines
+379
to
+381
| batch->key_member_indices[batch->key_count] = cmd->member_key_index; | ||
| batch->key_member_steps[batch->key_count] = cmd->member_key_step ? cmd->member_key_step : 1; | ||
| batch->key_member_counts[batch->key_count] = cmd->member_key_count ? cmd->member_key_count : -1; |
There was a problem hiding this comment.
Preserve member_key_count == 0; it is not equivalent to “all remaining.”
Line 381 rewrites every zero count to -1, but the command contract already makes those two states distinct: 0 means “use default”, while -1 means “walk all remaining member args”. Collapsing them here means any command that relies on the default count can spill the deep walk into trailing non-member arguments.
src/server.h already documents this distinction in the command metadata contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/memory_prefetch.c` around lines 379 - 381, The code collapses
cmd->member_key_count==0 into -1; change the assignment so zero is preserved
instead of being treated as "all remaining." Replace the ternary expression in
memory_prefetch.c that sets batch->key_member_counts[batch->key_count]
(currently using cmd->member_key_count ? cmd->member_key_count : -1) with a
direct assignment of cmd->member_key_count (i.e.,
batch->key_member_counts[batch->key_count] = cmd->member_key_count) so that both
0 (default) and -1 (walk all remaining) remain distinct.
hpatro
reviewed
Jun 10, 2026
hpatro
left a comment
hpatro
left a comment
Contributor
There was a problem hiding this comment.
- I think isolated testing of these composite data structure is good to show the increase in throughput. However, I'm curious how does this impact operations at main dictionary level. Would it thrash the memory and drop the throughput? If you could benchmark further, could you capture parallel running valkey-benchmark one for GET workload and one for HGET?
- It will be also good to capture the perf stat around cache hits/misses with this change.
- Why stop at nested key, should we try prefetching the nested value as well?
| PREFETCH_DONE /* Indicates that prefetching for this key is complete */ | ||
| PREFETCH_ENTRY, /* Initial state, prefetch entries associated with the given key's hash */ | ||
| PREFETCH_VALUE, /* prefetch the value object of the entry found in the previous step */ | ||
| PREFETCH_VALUE_DEEP, /* deep prefetch of inner hashtable for hash/zset types */ |
Contributor
There was a problem hiding this comment.
How about PREFETCH_VALUE_NESTED?
Comment on lines
+66
to
+71
| test "Deep prefetch - prefetch stats non-negative" { | ||
| # Just verify the stats exist and are non-negative (prefetch may not | ||
| # trigger in test env with single client, but should never be negative) | ||
| set entries [getInfoProperty [r info stats] io_threaded_total_prefetch_entries] | ||
| assert {$entries >= 0} | ||
| } |
Contributor
There was a problem hiding this comment.
This check should be performed after each deep/nested fetch and verify the total prefetch entries is greater than zero.
Member
Author
It doesn't seem to effect the get throughput on running a parallel benchmark. here are the results for that benchmark.
perf stat (20s during parallel GET + HGET)
I dont think that would give us good improvement. For embedded values the value is in the contiguous memory block, so there is no pointer to fetch and for larger values its one pointer hop to the value Let me also benchmark this, so we can prove this hypothesis. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extends the memory prefetch system to walk inner hashtables for hash (
OBJ_ENCODING_HASHTABLE) and sorted set (OBJ_ENCODING_SKIPLIST) types using a 3-phase approach:val->ptrto bring the data structure header into cacheargv[2]as the lookup keyThis is encoding-based (not per-command), automatically benefiting any command that does a single-field/member lookup on a large hash or sorted set.
Benchmarks on 2x c7g.4xlarge (Graviton3, ~32MB L3), separate server/client, io-threads=8, 200 clients, P=1, 30M-field hash, 10M-member zset:
This PR is inspired by #2180, also resolves all the feedbacks and adds deep prefetching for
OBJ_ENCODING_SKIPLISTas well.