Fix MEMORY USAGE command#36
Open
tomerqodo wants to merge 2 commits intoqodo_claude_vs_qodo_base_fix_memory_usage_command_pr5from
Open
Fix MEMORY USAGE command#36tomerqodo wants to merge 2 commits intoqodo_claude_vs_qodo_base_fix_memory_usage_command_pr5from
tomerqodo wants to merge 2 commits intoqodo_claude_vs_qodo_base_fix_memory_usage_command_pr5from
Conversation
Review Summary by QodoFix MEMORY USAGE command memory calculation and kvobj handling
WalkthroughsDescription• Rename objectComputeSize to kvobjComputeSize for kvobj type handling • Fix memory calculation to include object header and key upfront • Change assignment operators from = to += for proper accumulation • Fix stream memory calculation bug (use numele instead of numele-1) • Add comprehensive test coverage for MEMORY USAGE command Diagramflowchart LR
A["objectComputeSize function"] -->|Rename & refactor| B["kvobjComputeSize function"]
B -->|Initialize asize| C["malloc_usable_size of kvobj"]
C -->|Accumulate with +=| D["Add type-specific sizes"]
D -->|Fix stream calc| E["Use numele not numele-1"]
E -->|Return| F["Accurate memory usage"]
G["Test coverage"] -->|Add string tests| H["MEMORY USAGE validation"]
File Changes1. src/object.c
|
Code Review by Qodo
1. kvobjComputeSize not static
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| * are checked and averaged to estimate the total size. */ | ||
| #define OBJ_COMPUTE_SIZE_DEF_SAMPLES 5 /* Default sample size. */ | ||
| size_t objectComputeSize(robj *key, robj *o, size_t sample_size, int dbid) { | ||
| size_t kvobjComputeSize(robj *key, kvobj *o, size_t sample_size, int dbid) { |
There was a problem hiding this comment.
1. kvobjcomputesize not static 📘 Rule violation ⛯ Reliability
kvobjComputeSize is a file-local helper but is declared without static, unnecessarily exporting the symbol. This violates the requirement to declare internal helper functions as static to avoid symbol pollution.
Agent Prompt
## Issue description
`kvobjComputeSize` is a helper function defined in `src/object.c` but it is not declared `static`, which exports the symbol unnecessarily.
## Issue Context
Per the compliance requirement, helper/internal functions that are only used within a single source file should be `static` to prevent symbol pollution.
## Fix Focus Areas
- src/object.c[1210-1210]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1216
to
+1217
| /* 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.
2. Non-portable usable-size call 🐞 Bug ⛯ Reliability
kvobjComputeSize() uses malloc_usable_size(o) directly, bypassing Redis’ allocator abstraction; under USE_JEMALLOC/USE_TCMALLOC this can be undefined (wrong symbol / wrong allocator) or fail to compile on platforms without malloc_usable_size.
Agent Prompt
### Issue description
`kvobjComputeSize()` calls `malloc_usable_size()` directly, bypassing the project allocator abstraction. This can break builds (symbol not available) or return invalid sizes / crash when memory was allocated via jemalloc/tcmalloc APIs.
### Issue Context
This repo uses `zmalloc_size()` / `zmalloc_usable_size()` to map to `je_malloc_usable_size`, `tc_malloc_size`, `malloc_size`, etc. `kvobjComputeSize()` should use that abstraction.
### Fix Focus Areas
- src/object.c[1216-1218]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
1234
to
+1239
| do { | ||
| elesize += sizeof(quicklistNode)+zmalloc_size(node->entry); | ||
| elecount += node->count; | ||
| 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.
3. Quicklist average miscomputed 🐞 Bug ✓ Correctness
For OBJ_LIST quicklist encoding, kvobjComputeSize() divides sampled bytes by sampled nodes (samples) but multiplies by total elements (ql->count), overestimating list memory usage by roughly the average elements-per-node factor.
Agent Prompt
### Issue description
The quicklist memory estimation computes an average per *node* (`elesize/samples`) but multiplies it by total *elements* (`ql->count`), causing inflated MEMORY USAGE for lists.
### Issue Context
Within the sampling loop, `samples` increments once per node, while `elecount` accumulates elements across sampled nodes (`node->count`). `ql->count` is the total number of elements.
### Fix Focus Areas
- src/object.c[1231-1240]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
1327
to
1333
| asize += lpsize; | ||
| } else { | ||
| if (samples) lpsize /= samples; /* Compute the average. */ | ||
| asize += lpsize * (s->rax->numele-1); | ||
| asize += lpsize * s->rax->numele; | ||
| /* No need to check if seek succeeded, we enter this branch only | ||
| * if there are a few elements in the radix tree. */ | ||
| raxSeek(&ri,"$",NULL,0); |
There was a problem hiding this comment.
4. Stream listpacks double-counted 🐞 Bug ✓ Correctness
For streams with more listpacks than the sampling budget, kvobjComputeSize() adds average listpack size multiplied by s->rax->numele and then adds the last listpack’s real size again, overcounting stream memory usage.
Agent Prompt
### Issue description
The stream listpack estimation counts `numele` listpacks via the average and then adds the last listpack size again, inflating MEMORY USAGE for streams.
### Issue Context
The code explicitly adds the last listpack after doing an average-based multiplication, and the surrounding comment describes estimating N-1 and then adding the last.
### Fix Focus Areas
- src/object.c[1311-1337]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Benchmark PR from agentic-review-benchmarks#5