Add per slot memory accounting#10
Conversation
WalkthroughThis pull request introduces per-slot memory allocation tracking infrastructure for Redis cluster deployments. New public APIs compute allocation sizes for various data structures, and internal hooks update per-slot accounting whenever objects are modified. A new server configuration flag enables memory tracking per slot when cluster slot stats are active, with corresponding cluster command updates to report memory-bytes metrics. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/t_stream.c (1)
4391-4443: Fix per-slot accounting when DELREF removes PEL refs without deleting entries.
streamCleanupEntryCGroupRefs()can reduces->alloc_sizeeven when no stream items are deleted. Since the accounting update is gated byif (deleted), per-slot stats can become stale. Move the update outside the deletion check (and guard by size change).🐛 Proposed fix
- /* Update the stream's first ID. */ - if (deleted) { - if (server.memory_tracking_per_slot) - updateSlotAllocSize(c->db,getKeySlot(c->argv[1]->ptr),old_alloc,s->alloc_size); + if (server.memory_tracking_per_slot && old_alloc != s->alloc_size) + updateSlotAllocSize(c->db,getKeySlot(c->argv[1]->ptr),old_alloc,s->alloc_size); + + /* Update the stream's first ID. */ + if (deleted) { if (s->length == 0) { s->first_id.ms = 0; s->first_id.seq = 0;src/kvstore.h (1)
42-123: Add explicit alloc_size reset per-dict in kvstoreEmpty, matching keysizes_hist pattern.The
alloc_sizefield is initialized to 0 viazcalloc()on kvstore creation, but inkvstoreEmpty()it lacks explicit per-dict reset. Line 298 resetskeysizes_histwithin the dict loop, butalloc_sizerelies solely on destructor callbacks to decrement it viakvstoreTrackDeallocation(). SincekvstoreTrackDeallocation()returns early ifserver.memory_tracking_per_slotis disabled (line 221),alloc_sizemay not reach 0, causing the assertion at line 306 (viafreeDictIfNeeded()) to fail.Add explicit reset for
alloc_sizeafterdictEmpty()or alongsidekeysizes_hist:if (kvs->flags & KVSTORE_ALLOC_META_KEYS_HIST) { kvstoreDictMetaEx *metaExt = (kvstoreDictMetaEx *) metadata; memset(&metaExt->meta.keysizes_hist, 0, sizeof(metaExt->meta.keysizes_hist)); metaExt->meta.alloc_size = 0; // Add this line }src/t_list.c (2)
972-981: Potential accounting drift if shrinking conversion occurs.The
updateSlotAllocSizecall at lines 972-973 happens before the shrinking conversion at line 980. IflistTypeTryConversionchanges the encoding (e.g., quicklist → listpack), the slot allocation size recorded won't reflect the post-conversion size.Compare with
listElementsRemovedwhere the conversion happens first, then the update. Consider reordering to update after the potential conversion.Suggested fix
notifyKeyspaceEvent(NOTIFY_LIST,"ltrim",c->argv[1],c->db->id); if ((llenNew = listTypeLength(o)) == 0) { dbDeleteSkipKeysizesUpdate(c->db,c->argv[1]); notifyKeyspaceEvent(NOTIFY_GENERIC,"del",c->argv[1],c->db->id); llenNew = -1; /* Indicate key deleted to updateKeysizesHist() */ } else { listTypeTryConversion(o,LIST_CONV_SHRINKING,NULL,NULL); } + if (clusterSlotStatsEnabled()) + updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, listTypeAllocSize(o)); - if (clusterSlotStatsEnabled()) - updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, listTypeAllocSize(o)); updateKeysizesHist(c->db, getKeySlot(c->argv[1]->ptr), OBJ_LIST, llen, llenNew);Actually, the fix requires moving the updateSlotAllocSize call after the if/else block to capture post-conversion size:
- if (clusterSlotStatsEnabled()) - updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, listTypeAllocSize(o)); notifyKeyspaceEvent(NOTIFY_LIST,"ltrim",c->argv[1],c->db->id); if ((llenNew = listTypeLength(o)) == 0) { + if (clusterSlotStatsEnabled()) + updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, listTypeAllocSize(o)); dbDeleteSkipKeysizesUpdate(c->db,c->argv[1]); notifyKeyspaceEvent(NOTIFY_GENERIC,"del",c->argv[1],c->db->id); llenNew = -1; /* Indicate key deleted to updateKeysizesHist() */ } else { listTypeTryConversion(o,LIST_CONV_SHRINKING,NULL,NULL); + if (clusterSlotStatsEnabled()) + updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, listTypeAllocSize(o)); }
1144-1154: Same ordering issue: shrinking conversion after slot update.Similar to
ltrimCommand, the shrinking conversion at line 1153 happens afterupdateSlotAllocSizeat lines 1144-1145. If the list encoding changes due to shrinking, the slot allocation will not reflect the post-conversion size.Suggested fix
if (removed) { long ll = listTypeLength(subject); - if (clusterSlotStatsEnabled()) - updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, listTypeAllocSize(subject)); updateKeysizesHist(c->db, getKeySlot(c->argv[1]->ptr), OBJ_LIST, ll + removed, ll); notifyKeyspaceEvent(NOTIFY_LIST,"lrem",c->argv[1],c->db->id); if (ll == 0) { + if (clusterSlotStatsEnabled()) + updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, listTypeAllocSize(subject)); dbDelete(c->db,c->argv[1]); notifyKeyspaceEvent(NOTIFY_GENERIC,"del",c->argv[1],c->db->id); } else { listTypeTryConversion(subject,LIST_CONV_SHRINKING,NULL,NULL); + if (clusterSlotStatsEnabled()) + updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, listTypeAllocSize(subject)); } signalModifiedKey(c,c->db,c->argv[1]); }src/module.c (1)
4610-4634: Update per-slot accounting after list shrink conversion.
listTypeTryConversion()can change allocations after the pop, but the per-slot delta is only updated before it. This can skew slot memory stats when a pop triggers a shrink. Consider mirroring theRM_ListDeletepattern to capture the post-conversion delta (Line 4633).🛠️ Proposed fix
- if (!moduleDelKeyIfEmpty(key)) { - listTypeTryConversion(key->kv, LIST_CONV_SHRINKING, moduleFreeListIterator, key); - } + if (!moduleDelKeyIfEmpty(key)) { + if (server.memory_tracking_per_slot) + oldsize = listTypeAllocSize(key->kv); + listTypeTryConversion(key->kv, LIST_CONV_SHRINKING, moduleFreeListIterator, key); + if (server.memory_tracking_per_slot) + updateSlotAllocSize(key->db, getKeySlot(key->key->ptr), + oldsize, listTypeAllocSize(key->kv)); + }src/server.h (1)
3102-3118: Fix line 1236: use pre-capturedoldsizeinstead of recalculating after mutation.The call at line 1236 in
src/t_list.cincorrectly passeslistTypeAllocSize(kvsrc)as theoldsizeparameter, but this is calculated after thelistTypePop(kvsrc, wherefrom)mutation on line 1236. The pre-mutation size was already captured on line 1235. Change:listElementsRemoved(c, skey, wherefrom, kvsrc, 1, listTypeAllocSize(kvsrc), 1, NULL);to:
listElementsRemoved(c, skey, wherefrom, kvsrc, 1, oldsize, 1, NULL);The other four callers (lines 705, 848, 860, 1336) correctly capture
oldsizebefore mutations.
🤖 Fix all issues with AI agents
In `@src/debug.c`:
- Around line 550-555: The DEBUG subcommand "ALLOCSIZE-SLOTS-ASSERT" is
implemented but missing from the DEBUG help output; add an entry for
"ALLOCSIZE-SLOTS-ASSERT" to the DEBUG help list (the help text array or the
block that builds the DEBUG help reply in the debug command handler) so users
can discover it, include a short description like "enable/disable per-slot
allocsize assertions" and ensure it appears alongside the other DEBUG
subcommands and matches the existing formatting/placement conventions used for
help entries in the debug command output.
In `@src/server.c`:
- Around line 2848-2850: The config flag cluster-slot-stats-enabled is
MODIFIABLE_CONFIG but server.memory_tracking_per_slot is only set in
initServer(), so runtime CONFIG SET won't flip memory tracking; fix by adding a
CONFIG SET update handler that updates server.memory_tracking_per_slot whenever
cluster-slot-stats-enabled changes (or alternatively change the config
declaration to IMMUTABLE_CONFIG). Implement the handler to read the new boolean
value, assign it to server.memory_tracking_per_slot, and perform any start/stop
work needed for memory tracking (referencing memory_tracking_per_slot and
initServer() for where it's originally set) so the runtime change takes effect
immediately.
In `@src/server.h`:
- Around line 2044-2045: Add an apply callback for the MODIFIABLE_CONFIG
"cluster-slot-stats-enabled" that updates the global memory_tracking_per_slot by
setting memory_tracking_per_slot = clusterSlotStatsEnabled() when the config is
changed at runtime; register this callback in the config table entry for
"cluster-slot-stats-enabled" and, after updating memory_tracking_per_slot, call
the existing slot metadata reinitialization routine (e.g., the function used
elsewhere to rebuild or reinit per-slot metadata) so per-slot tracking state is
consistent with the new setting.
In `@src/t_hash.c`:
- Around line 1343-1357: hashTypeAllocSize is undercounting memory for
OBJ_ENCODING_HT by not including the dict metadata bytes; update the
OBJ_ENCODING_HT branch in hashTypeAllocSize(const robj *o) to add the dict
metadata size returned by htGetMetadataSize(d) (i.e., include
*htGetMetadataSize(d)) alongside sizeof(dict) and dictMemUsage(d) so the total
allocation includes the metadata bytes for the dict structure.
In `@src/t_set.c`:
- Around line 784-799: oldsize is never captured before membership checks so
updateSlotAllocSize() receives 0 and over-increments the slot; capture the
current allocation size into oldsize by calling oldsize = setTypeAllocSize(set)
once after confirming set exists (replace the discarded setTypeAllocSize(set)
call), then after the loop call updateSlotAllocSize(c->db,
getKeySlot(c->argv[1]->ptr), oldsize, setTypeAllocSize(set)) so you pass the
original and the new size; references: lookupKeyRead, checkType,
setTypeAllocSize, setTypeIsMember, updateSlotAllocSize.
- Around line 658-675: When a set becomes empty we must pass newsize=0 to
updateSlotAllocSize so the slot's allocation is cleared synchronously; replace
the call to updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize,
setTypeAllocSize(set)) that occurs inside the branch where setTypeSize(set) == 0
with updateSlotAllocSize(..., oldsize, 0) immediately before calling
dbDeleteSkipKeysizesUpdate(c->db, c->argv[1]); apply the same change in the
equivalent locations referenced in t_hash.c (around the hash deletion path) and
in t_zset.c (the three zset deletion paths) by updating their
updateSlotAllocSize calls to use newsize=0 when the data structure becomes
empty, using the surrounding symbols setTypeRemove, setTypeSize,
setTypeAllocSize, updateSlotAllocSize and dbDeleteSkipKeysizesUpdate to locate
the spots.
🧹 Nitpick comments (8)
src/intset.h (1)
52-52: Consider const-correctness for the new API.The allocation-size helper doesn’t mutate the intset, so accepting
const intset *would better communicate intent and align with other read-only APIs.src/commands/cluster-slot-stats.json (1)
39-41: Optional: documentmemory-bytessemantics in schemaAdd a short description (and note it’s present only when per-slot tracking is enabled) to make the schema self‑explanatory for clients.
src/rdb.c (1)
1449-1454: Only update slot accounting after a successful save.
updateSlotAllocSize()runs before theres < 0check. If the save fails, slot stats shouldn’t be touched. Consider moving the error check above the update.Suggested fix
- res = rdbSaveKeyValuePair(rdb, &key, kv, expire, dbid); - if (server.memory_tracking_per_slot) - updateSlotAllocSize(db, curr_slot, oldsize, kvobjAllocSize(kv)); - if (res < 0) goto werr; + res = rdbSaveKeyValuePair(rdb, &key, kv, expire, dbid); + if (res < 0) goto werr; + if (server.memory_tracking_per_slot) + updateSlotAllocSize(db, curr_slot, oldsize, kvobjAllocSize(kv));src/sort.c (1)
480-492: Avoid redundant alloc-size updates in the read-only zset iteration.This block doesn’t mutate the zset, so
zsetAllocSize()before/after is a no-op with extra cost. Consider removing it.Suggested cleanup
- if (server.memory_tracking_per_slot) - oldsize = zsetAllocSize(sortval); dictInitIterator(&di, set); while((setele = dictNext(&di)) != NULL) { sdsele = dictGetKey(setele); vector[j].obj = createStringObject(sdsele,sdslen(sdsele)); vector[j].u.score = 0; vector[j].u.cmpobj = NULL; j++; } dictResetIterator(&di); - if (server.memory_tracking_per_slot) - updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, zsetAllocSize(sortval));src/t_list.c (1)
507-508: Inconsistent guard condition for oldsize capture.This function uses
server.memory_tracking_per_slotto guard the oldsize capture, but all other functions in this file (e.g.,linsertCommand,lsetCommand,popGenericCommand,ltrimCommand, etc.) useclusterSlotStatsEnabled()for the same purpose. For consistency and maintainability, consider using the same guard condition throughout.Suggested fix for consistency
- if (server.memory_tracking_per_slot) + if (clusterSlotStatsEnabled()) oldsize = listTypeAllocSize(lobj);src/object.c (1)
1221-1250: Implementation is correct; note the MODULE type limitation.The base size calculation and per-type delegation are correct. The embedded string handling (line 1230-1231) combined with
stringObjectAllocSizereturning 0 for EMBSTR avoids double-counting.Note: The TODO at line 1247 means MODULE objects have incomplete per-slot memory accounting (only the kvobj header is counted, not the module value). This is acceptable as a known limitation, but consider adding a brief comment at the
updateSlotAllocSizecall sites indicating that MODULE types are undercounted.src/t_zset.c (2)
2952-2958: Consider removing memory tracking for read-only source sets.The ZUNION/ZINTER/ZDIFF operations iterate over source sets but never modify them. The source sets' memory allocation cannot change during these read-only operations, so
zuiAllocSize(&src[i])will always equalsrc[i].oldsize.This tracking adds overhead without providing value. Consider removing it if per-slot accounting is only needed for actual mutations.
3918-3926: Memory tracking on read-only commands is unnecessary.Commands like ZSCORE, ZMSCORE, ZRANK, ZSCAN, ZRANGE, and ZRANDMEMBER are read-only operations that cannot modify the zset's memory allocation. The
updateSlotAllocSizecall will always compute a zero delta.This applies to multiple locations in this file where read-only commands have tracking added. While the pattern is consistent and harmless, it adds overhead. Consider gating these updates behind a check like:
// Only track if command can modify the object if (is_write_operation && server.memory_tracking_per_slot) updateSlotAllocSize(...);Or simply remove tracking from read-only command paths.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/bitops.csrc/cluster_slot_stats.csrc/commands/cluster-slot-stats.jsonsrc/db.csrc/debug.csrc/defrag.csrc/hyperloglog.csrc/intset.csrc/intset.hsrc/kvstore.csrc/kvstore.hsrc/module.csrc/object.csrc/pubsub.csrc/rdb.csrc/sds.hsrc/server.csrc/server.hsrc/sort.csrc/t_hash.csrc/t_list.csrc/t_set.csrc/t_stream.csrc/t_string.csrc/t_zset.ctests/unit/cluster/slot-stats.tcltests/unit/info-keysizes.tcl
🧰 Additional context used
🧬 Code graph analysis (19)
src/intset.h (1)
src/intset.c (1)
intsetAllocSize(105-109)
src/debug.c (2)
src/object.c (1)
getLongLongFromObjectOrReply(1122-1134)src/networking.c (1)
addReply(461-476)
src/hyperloglog.c (2)
src/object.c (2)
stringObjectAllocSize(1020-1033)stringObjectLen(1011-1018)src/db.c (3)
updateSlotAllocSize(128-137)getKeySlot(410-437)updateKeysizesHist(76-126)
src/sort.c (2)
src/t_zset.c (2)
zsetAllocSize(1235-1249)zsetConvert(1285-1287)src/db.c (2)
updateSlotAllocSize(128-137)getKeySlot(410-437)
src/kvstore.h (1)
src/kvstore.c (2)
kvstoreDictAllocSize(770-775)kvstoreTrackDeallocation(220-231)
src/pubsub.c (1)
src/server.h (1)
clusterSlotStatsEnabled(3738-3738)
src/t_string.c (3)
src/object.c (1)
stringObjectAllocSize(1020-1033)src/sds.c (2)
sdsgrowzero(502-513)sdscatlen(520-529)src/db.c (2)
updateSlotAllocSize(128-137)getKeySlot(410-437)
src/t_list.c (4)
src/quicklist.c (1)
quicklistAllocSize(201-201)src/listpack.c (1)
lpBytes(1533-1535)src/server.h (1)
clusterSlotStatsEnabled(3738-3738)src/db.c (2)
updateSlotAllocSize(128-137)getKeySlot(410-437)
src/rdb.c (2)
src/object.c (1)
kvobjAllocSize(1221-1250)src/db.c (1)
updateSlotAllocSize(128-137)
src/bitops.c (3)
src/object.c (1)
stringObjectAllocSize(1020-1033)src/sds.c (1)
sdsgrowzero(502-513)src/db.c (2)
updateSlotAllocSize(128-137)getKeySlot(410-437)
src/server.c (3)
src/kvstore.c (1)
kvstoreTrackDeallocation(220-231)src/server.h (1)
clusterSlotStatsEnabled(3738-3738)src/db.c (1)
dbgAssertAllocSizePerSlot(183-204)
src/t_hash.c (5)
src/listpack.c (1)
lpBytes(1533-1535)src/db.c (1)
updateSlotAllocSize(128-137)src/object.c (1)
kvobjGetKey(234-246)src/dict.c (1)
dictMemUsage(1096-1099)src/server.h (1)
htGetMetadataSize(3599-3601)
src/kvstore.c (2)
src/object.c (1)
kvobjAllocSize(1221-1250)src/dict.c (1)
dictEmpty(1778-1793)
src/db.c (3)
src/kvstore.c (6)
kvstoreGetDictMetadata(1006-1013)kvstoreIteratorInit(642-649)kvstoreIteratorNext(689-704)kvstoreIteratorGetCurrentDictIndex(683-686)kvstoreIteratorRelease(652-658)kvstoreNumDicts(604-606)src/server.h (1)
dictGetKV(3810-3810)src/object.c (1)
kvobjAllocSize(1221-1250)
src/t_set.c (5)
src/dict.c (1)
dictMemUsage(1096-1099)src/server.h (1)
htGetMetadataSize(3599-3601)src/intset.c (1)
intsetAllocSize(105-109)src/listpack.c (2)
lpBytes(1533-1535)lpNew(221-228)src/db.c (2)
updateSlotAllocSize(128-137)getKeySlot(410-437)
src/defrag.c (2)
src/object.c (1)
kvobjAllocSize(1221-1250)src/db.c (1)
updateSlotAllocSize(128-137)
src/server.h (5)
src/t_list.c (2)
listTypeAllocSize(215-226)listElementsRemoved(787-808)src/object.c (2)
stringObjectAllocSize(1020-1033)kvobjAllocSize(1221-1250)src/t_zset.c (1)
zsetAllocSize(1235-1249)src/t_hash.c (1)
hashTypeAllocSize(1343-1358)src/db.c (2)
updateSlotAllocSize(128-137)dbgAssertAllocSizePerSlot(183-204)
src/module.c (2)
src/t_list.c (4)
listTypeAllocSize(215-226)listTypePush(147-170)listTypeLength(205-213)listTypePop(176-203)src/t_zset.c (1)
zsetAllocSize(1235-1249)
src/object.c (7)
src/sds.h (1)
sdsAllocSize(167-181)src/zmalloc.c (1)
zmalloc_size(458-462)src/module.c (1)
moduleGetMemUsage(12456-12469)src/t_list.c (1)
listTypeAllocSize(215-226)src/t_set.c (1)
setTypeAllocSize(482-496)src/t_zset.c (1)
zsetAllocSize(1235-1249)src/t_hash.c (1)
hashTypeAllocSize(1343-1358)
🔇 Additional comments (99)
src/t_set.c (12)
482-496: LGTM: setTypeAllocSize covers all encodings.The allocation accounting looks consistent across HT/intset/listpack.
617-640: LGTM: per-slot alloc deltas around conversion and add loop are consistent.
694-761: LGTM: SMOVE per-slot alloc tracking is consistent for src/dst.
766-779: LGTM: sismember per-slot tracking is benign for read-only access.
822-951: LGTM: per-slot alloc tracking in SPOP(count) branches is consistent.
1048-1074: LGTM: SPOP alloc tracking around mutation looks correct.
1318-1336: LGTM: SRANDMEMBER per-slot tracking is safe for read-only access.
1344-1364: LGTM: setopsrc wrapper + comparator updates look sound.
1379-1517: LGTM: SINTER per-slot tracking is consistently applied.
1561-1587: LGTM: SMEMBERS per-slot tracking is safe for read-only access.
1629-1791: LGTM: SUNION/SDIFF per-slot tracking integrates cleanly with setopsrc.
1852-1862: LGTM: SSCAN per-slot tracking is benign for read-only access.tests/unit/cluster/slot-stats.tcl (1)
789-789: LGTM: ORDERBY test now covers memory-bytes.src/sds.h (1)
170-178: LGTM: sdsAllocSize now includes the implicit null terminator.tests/unit/info-keysizes.tcl (1)
487-494: Confirm per-slot tracking is enabled for the debug assert
DEBUG ALLOCSIZE-SLOTS-ASSERTappears to depend on per-slot allocation tracking. This block doesn’t explicitly enablecluster-slot-stats-enabled(or other tracking flags), so please verify the command itself turns tracking on, or add the required config to keep the test deterministic.src/pubsub.c (1)
485-486: LGTM: helper keeps gating consistentThe helper preserves existing behavior while simplifying the condition.
src/bitops.c (1)
787-804: LGTM: alloc delta tracking on string growthThe old/new allocation accounting is gated correctly and captures growth deltas.
src/intset.c (1)
105-109: LGTM: safe alloc-size helperThe overflow guard aligns with resize logic and makes allocation sizing explicit.
src/sort.c (2)
187-188: LGTM —oldsizepre-declaration is clean.
342-348: Per-slot accounting around zsetConvert looks solid.src/t_string.c (2)
585-590: Looks good — per-slot accounting around SETRANGE growth.
803-830: Looks good — per-slot accounting around APPEND growth.src/server.c (3)
352-356: Per-slot deallocation hook looks correct.Calling
kvstoreTrackDeallocation()beforedecrRefCount()keeps accounting consistent with the object still intact.
2239-2244: Debug flag default is sensible.Initializing
dbg_assert_alloc_per_slotto disabled by default matches the existing debug-assert patterns.
4033-4038: Debug per-slot assertion hook looks good.The new assertion mirrors the existing debug histogram check and is safely gated.
src/defrag.c (2)
984-1071: Per-slot accounting around defragged KV updates looks consistent.The oldsize capture and post-defrag update are correctly guarded and scoped to the current slot.
1197-1218: Per-slot accounting for deferred defrag looks correct.Capturing oldsize before the deferred step and updating afterward aligns with incremental defrag behavior.
src/t_stream.c (1)
2346-2394: Memory-tracking hooks around XADD look solid.Old/new alloc accounting is captured and updated in both the success and error paths.
src/kvstore.c (4)
152-157: Test-only alloc_size assert looks good.Nice safety check to ensure per-dict allocation metadata is fully drained before releasing an empty dict.
220-231: Per-slot deallocation tracking is consistent.The guard on
server.memory_tracking_per_slotand metadata flags keeps the update safe and scoped.
326-332: Test-time cleanup invariants are solid.The explicit
dictEmpty()plus alloc_size assert makes release-time checks deterministic in tests.
770-775: Alloc-size accessor is straightforward.Returning 0 when metadata is unavailable avoids unexpected callers dereferencing NULL.
src/hyperloglog.c (3)
1614-1647: PFADD memory accounting is correctly bracketed.Capturing
oldsizebefore mutation and updating in both error and success paths is solid.Also applies to: 1653-1654
1765-1765: PFMERGE memory tracking looks consistent.The update after potential sparse→dense conversion uses the pre-mutation size as expected.
Also applies to: 1807-1808, 1836-1837
1970-2001: PFDEBUG tracking integration is clean.The size capture before mutation and updates only on encoding changes keep accounting accurate.
Also applies to: 2014-2015, 2083-2085
src/cluster_slot_stats.c (4)
18-26: MEMORY_BYTES stat wiring is clear.Enum addition and stat retrieval via
kvstoreDictAllocSize()are aligned with new tracking.Also applies to: 49-55
96-108: Reply shape update is correct.Conditional map length and
memory-bytesemission match the enablement gates.
139-139: Gating via clusterSlotStatsEnabled() is a good consolidation.Both network byte counters now respect the unified enablement logic.
Also applies to: 251-252
303-305: ORDERBY memory-bytes gate is correct.Nice to restrict sorting to cases where memory tracking is active.
src/t_hash.c (19)
429-435: ListpackEx expiry tracking is consistent.The old/new size delta around
lpDeleteRange()keeps per-slot accounting aligned.
739-759: Lazy-expire accounting in hashTypeGetValue() is handled well.Capturing size before mutations and updating after deletion keeps metadata stable.
Also applies to: 794-799
2073-2099: HSETNX tracking is consistent.The oldsize capture and post-update accounting align with the mutation flow.
2104-2134: HSET tracking hooks look correct.Updates are applied once after the batch mutation, which is ideal.
2324-2345: HSETEX tracking and lazy flags are well-integrated.The
HFE_LAZY_NO_UPDATE_*flags prevent double counting, and the final update consolidates changes.Also applies to: 2356-2357, 2430-2432
2463-2502: HINCRBY accounting is correct.Pre/post size tracking matches the mutation path.
2515-2560: HINCRBYFLOAT accounting is consistent.Oldsize/newsize boundaries are in the right places.
2647-2704: HGETDEL memory tracking is aligned.Using the lazy flags and a single post-update keeps accounting correct and avoids double updates.
Also applies to: 2727-2731
2754-2841: HGETEX tracking is in good shape.Single post-update after possible mutations is the right approach.
2905-2937: HDEL tracking is correct.The conditional update on early key removal avoids inconsistent deltas.
3011-3049: HGETALL iterator path accounting is clean.Accounting is properly bracketing potential lazy-expiry side effects.
3079-3090: HSCAN tracking looks fine.Size updates are gated and occur after scan execution.
3123-3339: HRANDFIELD (count) tracking is correct.The single update at
out:ensures all paths are covered.
3376-3411: HRANDFIELD single-element tracking is consistent.Old/new size capture brackets the random access safely.
3531-3545: Active-expire path accounting is correct.Tracking around
hashTypeDelete()properly reflects memory changes.
3657-3687: HTTL tracking is consistent with the read path.Accounting remains guarded and stable.
3731-3809: HEXPIRE tracking is consistent.*Pre/post sizing around
hashTypeSetExDone()keeps per-slot usage accurate.
3954-3989: HPERSIST listpackEx tracking is correct.The update after TTL removal keeps memory stats aligned.
3994-4023: HPERSIST HT tracking is consistent.Accounting is gated and stable for the HT encoding.
src/t_list.c (9)
215-226: LGTM!The implementation correctly handles both list encodings and follows the established pattern in this file for encoding-specific dispatch with a panic fallback for unknown encodings.
572-573: LGTM!The oldsize is correctly captured before potential encoding conversion, and the slot allocation is updated after the insert operation completes.
Also applies to: 589-590
670-673: LGTM!Good comment explaining why the update happens unconditionally—encoding conversion may occur regardless of whether the replace succeeds.
700-705: LGTM!The oldsize is correctly captured before the delete operation and passed to
listElementsRemovedfor proper accounting.
787-808: LGTM!The signature change correctly threads the oldsize parameter through to enable accurate slot accounting. The update logic properly handles both the deletion case (where update precedes
dbDelete) and the shrinking case (where update followslistTypeTryConversion).
838-860: LGTM!The oldsize is correctly captured before mutation and passed to
listElementsRemovedfor both single-element and multi-element pop operations.
1163-1174: LGTM!Correctly handles both the new list case (where oldsize is appropriately 0) and the existing list case. The update happens after both conversion and push operations.
1226-1236: LGTM!The two-stage update is intentional and correct:
- Lines 1230-1231 account for the pop operation (pre-pop → post-pop size)
- Line 1236 passes the post-pop size to
listElementsRemoved, which handles potential shrinking conversion and/or deletionThis correctly separates the pop accounting from the shrinking/deletion accounting.
1326-1336: LGTM!Standard pattern correctly applied: capture oldsize before mutation, pass to
listElementsRemovedfor accounting.src/module.c (12)
4569-4593: Per-slot list push accounting looks solid.Capturing
oldsizebefore conversion and applyingupdateSlotAllocSizeafter the push correctly reflects both encoding changes and element growth.
4684-4707: Per-slot list set accounting is consistent.Capturing
oldsizebefore mutation and applying the delta regardless of success keeps the slot stats aligned with possible encoding changes.
4727-4762: Per-slot list insert accounting looks correct.The delta update after insertion (plus the keysizes histogram update) keeps both size and memory accounting aligned.
4779-4792: Per-slot list delete accounting is thorough.Tracking both the delete and the post-delete shrink conversion mirrors the actual allocation changes well.
4877-4895: Per-slot zset add accounting is correct.The old/new allocation tracking is properly gated and mirrors the mutation paths.
4914-4931: Per-slot zset incr accounting is consistent.Capturing the pre-change size and updating after
zsetAddkeeps slot memory deltas accurate.
4958-4977: Per-slot zset remove accounting is correct.The empty-key guard plus the delta update on both remove and no-op paths keeps stats stable.
5379-5462: Per-slot hash accounting is well integrated.Updating per-field deltas during set/delete keeps slot memory stats aligned with hash mutations.
5701-5710: Per-slot stream add accounting looks correct.Using
s->alloc_sizebefore/after append provides accurate deltas.
5754-5759: Per-slot stream delete accounting is correct.The delta update after deletion reflects
alloc_sizechanges without overcounting.
6057-6062: Per-slot stream trim-by-length accounting is correct.Tracking
alloc_sizebefore/after trim ensures accurate slot deltas even when trimming is approximate.
6093-6098: Per-slot stream trim-by-id accounting is correct.The delta update around
streamTrimByIDmirrors the allocation changes properly.src/db.c (7)
128-137: LGTM - Clean per-slot size update helper.The implementation correctly handles the NULL dictMeta case (for released slot dicts) and performs atomic delta updates. The test-only underflow assertion at lines 133-134 is appropriate for catching bugs during testing.
179-204: LGTM - Solid debugging assertion for per-slot allocation consistency.The function correctly iterates through all keys, computes expected per-slot sizes using
kvobjAllocSize, and validates against the stored metadata. This will be valuable for catching accounting bugs during development.
386-388: LGTM!Correctly accounts for newly added keys with
oldsize=0and captures the full allocation size after the key is fully initialized (including expire handling at line 381).
496-498: LGTM!Mirrors the accounting pattern in
dbAddInternalfor RDB-loaded keys.
523-523: LGTM - Correctly captures old size before replacement.The
oldsizecapture at lines 553-554 correctly happens after theRM_StringDMArefetch at line 551, ensuring we measure the actual object being replaced.Also applies to: 553-554, 609-611
2517-2517: LGTM - Correctly handles reallocation accounting for expire field addition.The logic properly:
- Only captures
oldsizewhen the key didn't previously have an expire (line 2530-2531)- Only updates accounting when actual reallocation occurs (kv != kvnew at line 2540)
This ensures no spurious accounting updates when just modifying an existing expire value.
Also applies to: 2530-2531, 2542-2543
792-799: The per-slot accounting is properly applied in both sync and async deletion paths. For synchronous deletion,kvstoreDictTwoPhaseUnlinkFreecallsdictFreeKey, which invokes the dict type's key destructor (dictDestructorKV). This destructor callskvstoreTrackDeallocation, which decrements the slot'salloc_size. The async path explicitly callsupdateSlotAllocSizebefore the asynchronous free. Both mechanisms ensure proper accounting—no fix is required.src/object.c (2)
1020-1033: LGTM - Correctly handles all string encoding types.The function properly returns:
0for INT encoding (value stored in pointer)sdsAllocSize()for RAW encoding (external sds allocation)0for EMBSTR (already counted in kvobj base size via line 1230-1231)
1207-1219: LGTM - Clean simplification leveraging pre-computed allocation sizes.The refactoring correctly uses
kvobjAllocSize()for all core types (where allocation sizes are computed from data structure properties or maintained incrementally likestream->alloc_size), while preserving the sampling-based approach for MODULE types viamoduleGetMemUsage().src/server.h (5)
2381-2382: Nice addition for per-slot debug assertions.The new debug flag aligns well with
dbg_assert_keysizesand provides a clear knob for targeted checks.
3164-3203: LGTM: allocation helpers exposed in the header.The prototypes for string/kvobj allocation sizing are consistent with the new per-slot accounting surface.
3776-3779: Header prototypes for slot accounting APIs look good.No concerns with the added
updateSlotAllocSize/dbgAssertAllocSizePerSlotdeclarations.
3738-3738: No duplicate or conflicting definitions found. TheclusterSlotStatsEnabledfunction is defined exactly once as a static inline insrc/server.h:3738and is correctly used across multiple files (t_list.c, server.c, pubsub.c, cluster_slot_stats.c). The implementation is sound.
3424-3625: HFE_LAZY_NO_UPDATE_ALLOCSIZES flag and allocsize helpers are properly wired throughout all mutation paths.The flag is correctly respected in lazy hash-field deletion code:
hashTypeGetValue()checks the flag before tracking oldsize and updating allocsize (lines 755, 757, 794, 797)onFieldExpire()active expiration handler properly callsupdateSlotAllocSize()withhashTypeAllocSize()- Batch operations (HGET, HSET variants) set the flag to suppress per-field updates and perform a single bulk allocsize update after the loop completes
All three alloc-size helpers (
hashTypeAllocSize,zsetAllocSize,setTypeAllocSize) are used consistently across their respective data types with correspondingupdateSlotAllocSize()calls. No per-slot allocation drift issues detected.src/t_zset.c (6)
1235-1249: LGTM: Memory size computation for zsets is correctly implemented.The function properly handles both encodings:
- Listpack: returns
lpBytes()for the contiguous memory block- Skiplist: aggregates the zset struct, skiplist cached size, dict struct, and dict memory usage
1882-1912: Memory tracking for ZADD is correctly implemented.The tracking properly handles:
- Encoding conversion via
zsetTypeMaybeConvert(lines 1882-1886)- Element additions in the loop (lines 1889-1912)
- Error path on NaN (lines 1901-1903)
2291-2302: LGTM: Helper function for union/inter/diff operations.Correctly delegates to the appropriate type-specific allocator based on the source object type.
4086-4165: LGTM: Memory tracking for ZPOP operations is correctly implemented.The tracking properly captures size before the removal loop and updates after all elements are popped. This is a write operation where tracking is necessary.
1951-1966: LGTM: Memory tracking for ZREM is correctly implemented.The tracking correctly handles both the key deletion path (updating before
dbDeleteSkipKeysizesUpdate) and the partial removal path (updating after the loop when key remains).
4389-4389: Clean exit consolidation viagoto outfor tracking updates.The
goto outpattern ensures all exit paths converge at the memory tracking update. However, as noted earlier, ZRANDMEMBER is a read-only command, so this tracking won't observe any changes.Also applies to: 4419-4419, 4440-4440, 4529-4531
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| } else if (!strcasecmp(c->argv[1]->ptr,"ALLOCSIZE-SLOTS-ASSERT") && c->argc == 3) { | ||
| long long flag; | ||
| if (getLongLongFromObjectOrReply(c, c->argv[2], &flag, NULL) != C_OK) | ||
| return; | ||
| server.dbg_assert_alloc_per_slot = (flag != 0); | ||
| addReply(c, shared.ok); |
There was a problem hiding this comment.
Add the new subcommand to DEBUG help output.
The handler is present, but the help list doesn’t mention ALLOCSIZE-SLOTS-ASSERT, which makes it undiscoverable.
✏️ Suggested help entry
"KEYSIZES-HIST-ASSERT <0|1>",
" Enable/disable keysizes histogram assertion after each command.",
+"ALLOCSIZE-SLOTS-ASSERT <0|1>",
+" Enable/disable per-slot alloc-size assertion after each command.",🤖 Prompt for AI Agents
In `@src/debug.c` around lines 550 - 555, The DEBUG subcommand
"ALLOCSIZE-SLOTS-ASSERT" is implemented but missing from the DEBUG help output;
add an entry for "ALLOCSIZE-SLOTS-ASSERT" to the DEBUG help list (the help text
array or the block that builds the DEBUG help reply in the debug command
handler) so users can discover it, include a short description like
"enable/disable per-slot allocsize assertions" and ensure it appears alongside
the other DEBUG subcommands and matches the existing formatting/placement
conventions used for help entries in the debug command output.
| server.client_mem_usage_buckets = NULL; | ||
| server.memory_tracking_per_slot = clusterSlotStatsEnabled(); | ||
| resetReplicationBuffer(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where server.memory_tracking_per_slot and cluster_slot_stats_enabled are updated.
rg -n -C3 '\bmemory_tracking_per_slot\b'
rg -n -C3 '\bcluster_slot_stats_enabled\b'Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 50396
🏁 Script executed:
#!/bin/bash
# Search for where cluster_slot_stats_enabled is assigned
rg -n 'cluster_slot_stats_enabled\s*=' --max-count=20
# Search for where memory_tracking_per_slot is assigned
rg -n 'memory_tracking_per_slot\s*=' --max-count=20
# Search for CONFIG SET handler
rg -n 'CONFIG.*SET|configSet|config.*set' -i --max-count=20Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 50397
🏁 Script executed:
#!/bin/bash
# Search for cluster-slot-stats-enabled configuration handling
rg -n 'cluster-slot-stats-enabled|cluster_slot_stats_enabled' src/config.c
# Search for cluster_slot_stats_enabled in server.h with context
rg -n -B2 -A2 'cluster_slot_stats_enabled' src/server.hRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 840
🏁 Script executed:
#!/bin/bash
# Search for any set or apply function related to cluster-slot-stats-enabled
rg -n -B5 -A10 'cluster-slot-stats-enabled' src/config.c | head -40
# Search for any place memory_tracking_per_slot is updated after initialization
rg -n 'memory_tracking_per_slot\s*=' src/ --type cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 2202
🏁 Script executed:
#!/bin/bash
# Verify the clusterSlotStatsEnabled function logic
rg -n -A3 'static inline int clusterSlotStatsEnabled' src/server.h
# Verify cluster_enabled doesn't change at runtime
rg -n 'server.cluster_enabled\s*=' src/server.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Search for any function that might update memory_tracking_per_slot at runtime
rg -n 'updateSlotAllocSize|memory_tracking_per_slot' src/server.c | head -20
# Check if cluster_enabled can change at runtime
rg -n 'MODIFIABLE_CONFIG|IMMUTABLE_CONFIG' src/config.c | grep cluster-enabledRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 262
Runtime updates to cluster-slot-stats-enabled don't sync memory_tracking_per_slot.
The config cluster-slot-stats-enabled is marked MODIFIABLE_CONFIG (runtime-mutable), but memory_tracking_per_slot is set only during initServer() and never updated afterward. Enabling this config at runtime won't activate memory tracking, and disabling it won't stop the overhead. Either mark cluster-slot-stats-enabled as IMMUTABLE_CONFIG or add an update handler that synchronizes memory_tracking_per_slot when the setting changes via CONFIG SET.
🤖 Prompt for AI Agents
In `@src/server.c` around lines 2848 - 2850, The config flag
cluster-slot-stats-enabled is MODIFIABLE_CONFIG but
server.memory_tracking_per_slot is only set in initServer(), so runtime CONFIG
SET won't flip memory tracking; fix by adding a CONFIG SET update handler that
updates server.memory_tracking_per_slot whenever cluster-slot-stats-enabled
changes (or alternatively change the config declaration to IMMUTABLE_CONFIG).
Implement the handler to read the new boolean value, assign it to
server.memory_tracking_per_slot, and perform any start/stop work needed for
memory tracking (referencing memory_tracking_per_slot and initServer() for where
it's originally set) so the runtime change takes effect immediately.
| int memory_tracking_per_slot; /* Account used memory per slot */ | ||
| unsigned int max_new_tls_conns_per_cycle; /* The maximum number of tls connections that will be accepted during each invocation of the event loop. */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "memory_tracking_per_slot" src
rg -n "cluster_slot_stats_enabled" src
rg -n "clusterSlotStatsEnabled" srcRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 15194
🏁 Script executed:
# Check where cluster_slot_stats_enabled is modified at runtime
rg -n "cluster_slot_stats_enabled\s*=" src
# Look for resetSlotStats and slot metadata initialization
rg -n "resetSlotStats|resetSlotUsage|slot.*alloc" src/cluster_slot_stats.c
# Check server.c for initialization ordering
grep -n "memory_tracking_per_slot\|cluster_slot_stats_enabled\|clusterSlotStatsEnabled" src/server.c | head -30Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 153
🏁 Script executed:
# Look for config-related callbacks that might handle cluster_slot_stats_enabled changes
rg -n "cluster_slot_stats_enabled.*callback|setModuleConfig.*cluster|CONFIG SET" src/config.c | head -20
# Check cluster init code
rg -n "clusterInit|resetSlot" src/cluster.c | head -20
# Look for any metadata reset on cluster enable/disable
rg -n "resetSlot|initSlot|clearSlot" src/cluster_slot_stats.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1391
🏁 Script executed:
# Check how createBoolConfig is defined and its callback parameters
rg -n "createBoolConfig.*cluster_slot_stats_enabled" -A 1 src/config.c
# Look at the actual createBoolConfig definition
rg -n "^[^/]*createBoolConfig" src/config.c | head -5
# Check if there's an afterUpdateConfig or similar hook
rg -n "afterUpdate|onUpdate|modifyConfig" src/config.c | head -10
# Look for where cluster_slot_stats_enabled config is actually applied
rg -n "cluster_slot_stats_enabled.*=" src --type c | grep -v "server\."Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 732
🏁 Script executed:
# Look for cluster reinitialization or slot stats reset on config change
rg -n "cluster.*(reset|reinit|init)" src/cluster.c | head -20
# Check if there's any documentation or comments about dynamic cluster_slot_stats_enabled changes
rg -B2 -A2 "cluster_slot_stats_enabled.*MODIFIABLE" src
# Look for server-wide config revalidation or hook after CONFIG SET
rg -n "configSetCommand|processConfigSet" src/config.c | head -10Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 122
🏁 Script executed:
# Look at configSetCommand implementation
sed -n '801,900p' src/config.c
# Check for any post-config hooks or reinitialization
rg -n "afterConfigUpdate|postConfigSet|configReload" srcRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 4032
🏁 Script executed:
# Continue reading configSetCommand to see if there's any post-apply hooks
sed -n '900,950p' src/config.c
# Check if cluster_slot_stats_enabled has any apply callback or if it's just NULL
rg -B5 -A5 "createBoolConfig.*cluster-slot-stats-enabled" src/config.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 3180
🏁 Script executed:
# Check if there's any code that re-syncs memory_tracking_per_slot after startup
rg -n "memory_tracking_per_slot\s*=" src
# Look at the broader context of clusterSlotStatsEnabled usage to understand intent
rg -B3 -A3 "clusterSlotStatsEnabled()" src/server.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 507
🏁 Script executed:
# Get more context around line 2849
sed -n '2840,2860p' src/server.c
# Check if there's intentional duplication or if this is in different code paths
rg -n "memory_tracking_per_slot = clusterSlotStatsEnabled" src/server.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1013
🏁 Script executed:
# Check if there's any comment or documentation about dynamic cluster_slot_stats_enabled changes
rg -B5 "cluster_slot_stats_enabled" src/server.h
# Look for any warning or restriction about runtime changes to cluster config
rg -n "cluster.*dynamic|runtime.*cluster|toggle.*cluster" src
# Check if cluster is cluster-only (can't be toggled at runtime)
rg -n "cluster_enabled.*IMMUTABLE|cluster.*PROTECTED" src/config.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 890
🏁 Script executed:
# Check if cluster_enabled is immutable
rg -n "cluster.*IMMUTABLE|createBoolConfig.*cluster-enabled" src/config.c
# Look at the initialization sequence - when is cluster_enabled determined?
rg -n "cluster_enabled.*=" src/server.c | head -20Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 826
🏁 Script executed:
# Check if there's any discussion or TODO about syncing memory_tracking_per_slot on config change
rg -B3 -A3 "memory_tracking|slot.*stats" src/server.c | head -40
# Look for any config validation or post-set hooks related to cluster
rg -n "configNeedsApply" src/config.c | head -10Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1442
🏁 Script executed:
# Check if clusterInit initializes slot stats metadata
sed -n '1,100p' src/cluster.c | grep -A20 "clusterInit"
# Look at what cluster_slot_stats initialization looks like
rg -n "cluster_slot_stats\s*=" src/server.c
# Check if there's any documentation about when cluster_slot_stats_enabled should be set
rg -B5 -A5 "cluster_slot_stats_enabled" src/config.c | grep -A10 "createBoolConfig"Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1350
🏁 Script executed:
# Verify the actual runtime behavior: can cluster_slot_stats_enabled be changed at runtime in practice?
# Check if there are any tests or examples
rg -n "CONFIG SET.*cluster-slot-stats" . --type md --type txt
# Check if there's any guard against changing this at runtime
rg -n "cluster_slot_stats_enabled" src/cluster_slot_stats.c | head -20Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 748
🏁 Script executed:
# Let me check the actual slot stats initialization more carefully
rg -n "clusterSlotStat|slot.*stats.*=" src/cluster.c | head -30
# Check if slot alloc metadata is initialized anywhere
rg -n "slot.*alloc|alloc_size.*slot" src/cluster_slot_stats.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 119
🏁 Script executed:
# Check what clusterSlotStatResetAll does
rg -B5 -A15 "clusterSlotStatResetAll" src/cluster_slot_stats.c
# Look for alloc_size initialization in slot stats
rg -n "alloc_size" src/cluster_slot_stats.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1236
Add apply callback for cluster-slot-stats-enabled to sync memory_tracking_per_slot at runtime.
cluster-slot-stats-enabled is MODIFIABLE_CONFIG but lacks an apply callback. If toggled at runtime via CONFIG SET, memory_tracking_per_slot remains stale, causing inconsistent per-slot tracking. Implement an apply callback to update memory_tracking_per_slot = clusterSlotStatsEnabled() on config change, and reinitialize slot metadata as needed.
🤖 Prompt for AI Agents
In `@src/server.h` around lines 2044 - 2045, Add an apply callback for the
MODIFIABLE_CONFIG "cluster-slot-stats-enabled" that updates the global
memory_tracking_per_slot by setting memory_tracking_per_slot =
clusterSlotStatsEnabled() when the config is changed at runtime; register this
callback in the config table entry for "cluster-slot-stats-enabled" and, after
updating memory_tracking_per_slot, call the existing slot metadata
reinitialization routine (e.g., the function used elsewhere to rebuild or reinit
per-slot metadata) so per-slot tracking state is consistent with the new
setting.
| size_t hashTypeAllocSize(const robj *o) { | ||
| serverAssertWithInfo(NULL,o,o->type == OBJ_HASH); | ||
| size_t size = 0; | ||
| if (o->encoding == OBJ_ENCODING_LISTPACK) { | ||
| size = lpBytes(o->ptr); | ||
| } else if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { | ||
| listpackEx *lpt = o->ptr; | ||
| size = sizeof(listpackEx) + lpBytes(lpt->lp); | ||
| } else if (o->encoding == OBJ_ENCODING_HT) { | ||
| dict *d = o->ptr; | ||
| size += sizeof(dict) + dictMemUsage(d) + *htGetMetadataSize(d); | ||
| } else { | ||
| serverPanic("Unknown hash encoding"); | ||
| } | ||
| return size; |
There was a problem hiding this comment.
Include dict metadata bytes in hashTypeAllocSize().
hashTypeAllocSize() currently omits the dict metadata allocation (e.g., sizeof(size_t) or sizeof(htMetadataEx)), which causes per-slot memory undercounting. Please add the metadata bytes from the dict type.
🛠️ Proposed fix
} else if (o->encoding == OBJ_ENCODING_HT) {
dict *d = o->ptr;
- size += sizeof(dict) + dictMemUsage(d) + *htGetMetadataSize(d);
+ size_t meta_bytes = d->type->dictMetadataBytes ? d->type->dictMetadataBytes(d) : 0;
+ size += sizeof(dict) + meta_bytes + dictMemUsage(d) + *htGetMetadataSize(d);
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| size_t hashTypeAllocSize(const robj *o) { | |
| serverAssertWithInfo(NULL,o,o->type == OBJ_HASH); | |
| size_t size = 0; | |
| if (o->encoding == OBJ_ENCODING_LISTPACK) { | |
| size = lpBytes(o->ptr); | |
| } else if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { | |
| listpackEx *lpt = o->ptr; | |
| size = sizeof(listpackEx) + lpBytes(lpt->lp); | |
| } else if (o->encoding == OBJ_ENCODING_HT) { | |
| dict *d = o->ptr; | |
| size += sizeof(dict) + dictMemUsage(d) + *htGetMetadataSize(d); | |
| } else { | |
| serverPanic("Unknown hash encoding"); | |
| } | |
| return size; | |
| size_t hashTypeAllocSize(const robj *o) { | |
| serverAssertWithInfo(NULL,o,o->type == OBJ_HASH); | |
| size_t size = 0; | |
| if (o->encoding == OBJ_ENCODING_LISTPACK) { | |
| size = lpBytes(o->ptr); | |
| } else if (o->encoding == OBJ_ENCODING_LISTPACK_EX) { | |
| listpackEx *lpt = o->ptr; | |
| size = sizeof(listpackEx) + lpBytes(lpt->lp); | |
| } else if (o->encoding == OBJ_ENCODING_HT) { | |
| dict *d = o->ptr; | |
| size_t meta_bytes = d->type->dictMetadataBytes ? d->type->dictMetadataBytes(d) : 0; | |
| size += sizeof(dict) + meta_bytes + dictMemUsage(d) + *htGetMetadataSize(d); | |
| } else { | |
| serverPanic("Unknown hash encoding"); | |
| } | |
| return size; | |
| } |
🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 1343 - 1357, hashTypeAllocSize is undercounting
memory for OBJ_ENCODING_HT by not including the dict metadata bytes; update the
OBJ_ENCODING_HT branch in hashTypeAllocSize(const robj *o) to add the dict
metadata size returned by htGetMetadataSize(d) (i.e., include
*htGetMetadataSize(d)) alongside sizeof(dict) and dictMemUsage(d) so the total
allocation includes the metadata bytes for the dict structure.
| unsigned long oldSize = setTypeSize(set); | ||
| if (server.memory_tracking_per_slot) | ||
| oldsize = setTypeAllocSize(set); | ||
|
|
||
| for (j = 2; j < c->argc; j++) { | ||
| if (setTypeRemove(set,c->argv[j]->ptr)) { | ||
| deleted++; | ||
| if (setTypeSize(set) == 0) { | ||
| if (server.memory_tracking_per_slot) | ||
| updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, setTypeAllocSize(set)); | ||
| dbDeleteSkipKeysizesUpdate(c->db, c->argv[1]); | ||
| keyremoved = 1; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (server.memory_tracking_per_slot && !keyremoved) | ||
| updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, setTypeAllocSize(set)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "dbDeleteSkipKeysizesUpdate" -C 5Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 8726
🏁 Script executed:
rg -n "updateSlotAllocSize" -C 3 src/db.c src/kvstore.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 2267
🏁 Script executed:
fd -e c -e h | xargs grep -l "dbDeleteSkipKeysizesUpdate" | head -5Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 143
🏁 Script executed:
rg -n "int dbGenericDelete" -A 50 src/db.c | head -100Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 2325
🏁 Script executed:
sed -n '750,820p' src/db.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 2723
🏁 Script executed:
sed -n '800,810p' src/db.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 371
🏁 Script executed:
rg -n "server.lazyfree_lazy_server_del" -C 2 | head -30Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1740
Pass newsize=0 to updateSlotAllocSize() before deletion to account for all allocations being freed.
Currently line 667 calls updateSlotAllocSize(..., oldsize, setTypeAllocSize(set)) where the set is already empty. The subsequent dbDeleteSkipKeysizesUpdate() only updates per-slot allocation to 0 if async deletion is enabled. In sync mode, the slot retains the empty-set overhead.
Pass 0 as newsize to ensure the slot allocation is properly cleared regardless of deletion mode:
updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, 0);
dbDeleteSkipKeysizesUpdate(c->db, c->argv[1]);Apply the same fix to:
t_hash.cline 2927t_zset.clines 1957, 2060, 2082
🤖 Prompt for AI Agents
In `@src/t_set.c` around lines 658 - 675, When a set becomes empty we must pass
newsize=0 to updateSlotAllocSize so the slot's allocation is cleared
synchronously; replace the call to updateSlotAllocSize(c->db,
getKeySlot(c->argv[1]->ptr), oldsize, setTypeAllocSize(set)) that occurs inside
the branch where setTypeSize(set) == 0 with updateSlotAllocSize(..., oldsize, 0)
immediately before calling dbDeleteSkipKeysizesUpdate(c->db, c->argv[1]); apply
the same change in the equivalent locations referenced in t_hash.c (around the
hash deletion path) and in t_zset.c (the three zset deletion paths) by updating
their updateSlotAllocSize calls to use newsize=0 when the data structure becomes
empty, using the surrounding symbols setTypeRemove, setTypeSize,
setTypeAllocSize, updateSlotAllocSize and dbDeleteSkipKeysizesUpdate to locate
the spots.
| size_t oldsize = 0; | ||
| kvobj *set = lookupKeyRead(c->db, c->argv[1]); | ||
| if (set && checkType(c,set,OBJ_SET)) return; | ||
|
|
||
| addReplyArrayLen(c,c->argc - 2); | ||
|
|
||
| if (server.memory_tracking_per_slot && set) | ||
| setTypeAllocSize(set); | ||
| for (int j = 2; j < c->argc; j++) { | ||
| if (set && setTypeIsMember(set,c->argv[j]->ptr)) | ||
| addReply(c,shared.cone); | ||
| else | ||
| addReply(c,shared.czero); | ||
| } | ||
| if (server.memory_tracking_per_slot && set) | ||
| updateSlotAllocSize(c->db, getKeySlot(c->argv[1]->ptr), oldsize, setTypeAllocSize(set)); |
There was a problem hiding this comment.
Fix per-slot alloc accounting in SMISMEMBER (oldsize never set).
oldsize stays 0 because the alloc size result is discarded, so updateSlotAllocSize() will inflate the slot’s alloc_size on every call.
🐛 Proposed fix
- if (server.memory_tracking_per_slot && set)
- setTypeAllocSize(set);
+ if (server.memory_tracking_per_slot && set)
+ oldsize = setTypeAllocSize(set);🤖 Prompt for AI Agents
In `@src/t_set.c` around lines 784 - 799, oldsize is never captured before
membership checks so updateSlotAllocSize() receives 0 and over-increments the
slot; capture the current allocation size into oldsize by calling oldsize =
setTypeAllocSize(set) once after confirming set exists (replace the discarded
setTypeAllocSize(set) call), then after the loop call updateSlotAllocSize(c->db,
getKeySlot(c->argv[1]->ptr), oldsize, setTypeAllocSize(set)) so you pass the
original and the new size; references: lookupKeyRead, checkType,
setTypeAllocSize, setTypeIsMember, updateSlotAllocSize.
2 participants
Benchmark PR from qodo-benchmark#34
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.