Fix MEMORY USAGE command#5
Fix MEMORY USAGE command#5tomerqodo wants to merge 2 commits intocursor_full_base_fix_memory_usage_command_pr5from
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| samples++; | ||
| } while ((node = node->next) && samples < sample_size); | ||
| asize += (double)elesize/elecount*ql->count; | ||
| asize += (double)elesize/samples*ql->count; |
There was a problem hiding this comment.
Quicklist divides by nodes instead of items
High Severity
The memory calculation for quicklist divides by samples (number of nodes) instead of elecount (number of items). Since quicklist nodes contain multiple items, this causes a unit mismatch: elesize/samples gives bytes-per-node, but it's then multiplied by ql->count (total items). This will massively over-report memory usage—by roughly a factor equal to the average items per node.
| } else { | ||
| if (samples) lpsize /= samples; /* Compute the average. */ | ||
| asize += lpsize * (s->rax->numele-1); | ||
| asize += lpsize * s->rax->numele; |
There was a problem hiding this comment.
Stream double-counts last listpack in memory estimate
Medium Severity
The change from lpsize * (s->rax->numele - 1) to lpsize * s->rax->numele contradicts the documented algorithm in the comment above. The code computes the average size for ALL listpacks, then explicitly adds the last listpack's actual size on line 1336, causing double-counting of the last listpack.
| size_t elesize = 0, elecount = 0, samples = 0; | ||
|
|
||
| /* All kv-objects has at least kvobj header and embedded key */ | ||
| size_t asize = malloc_usable_size((void *)o); |
There was a problem hiding this comment.
Uses wrong allocator function causing portability issues
High Severity
The code uses malloc_usable_size() directly instead of the Redis wrapper zmalloc_size(). When Redis is built with jemalloc (the default for production), this calls glibc's function on jemalloc-allocated memory, returning incorrect sizes. On macOS, malloc_usable_size doesn't exist at all (macOS uses malloc_size), causing compilation failure. The rest of this same function correctly uses zmalloc_size() for all other allocation size queries.
2 participants
Benchmark PR from agentic-review-benchmarks#5
Note
Improves accuracy of memory introspection and aligns it with kv-embedded objects.
objectComputeSizewithkvobjComputeSizethat includeskvobjheader and embedded key viamalloc_usable_size, and refine per-type size accounting (strings, lists/quicklist sampling, sets, zsets, hashes, streams, modules)kvobjComputeSizeinMEMORY USAGEcommand; update comments and help text accordinglystring.tcl,list.tcl, andhash.tclto assert nonzero/expectedMEMORY USAGE, including string size bounds and jemalloc-specific casesWritten by Cursor Bugbot for commit b6ee4f1. Configure here.