Fix MEMORY USAGE command

[tomerqodo]

Benchmark PR from agentic-review-benchmarks#5

Summary by CodeRabbit

• Tests

  • Added comprehensive test coverage for memory usage calculations, including new tests for various data types, encoding scenarios, and size combinations

• Refactor

  • Improved memory accounting functions with enhanced logic for better accuracy in memory usage metrics and more reliable tracking of data structures

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The changes rename and refactor a memory-size calculation function from objectComputeSize to kvobjComputeSize, update its parameter type from robj* to kvobj*, and rework memory accounting logic across multiple object encodings to use incremental size accumulation instead of explicit header calculations.

Changes

Cohort / File(s)	Summary
Core Function Refactor src/object.c	Function renamed from objectComputeSize to kvobjComputeSize with second parameter type changed from robj* to kvobj*. Memory accounting refactored across all encodings (STRING, LIST, SET, ZSET, HASH, STREAM, MODULE) to use incremental size accumulation (asize +=) instead of explicit header-inclusive calculations. Stream size computation logic adjusted for rax element handling.
Test Comment Updates tests/unit/type/hash.tcl, tests/unit/type/list.tcl	Coverage comments updated to reflect new function name kvobjComputeSize instead of objectComputeSize.
New String Memory Test tests/unit/type/string.tcl	Added "MEMORY USAGE - STRINGS" test block covering memory usage calculation for EMBSTR/RAW and OBJ\_ENCODING\_INT encodings across varying key and value sizes, with assertions on minimum and maximum bounds.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A function hops from old to new,
From robj to kvobj it flew,
Memory counting grows so lean,
Each encoding sums its piece seen,
Tests confirm the sizes true!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix MEMORY USAGE command' directly aligns with the primary change: refactoring the memory-size calculation function to fix MEMORY USAGE command behavior, with comprehensive updates across multiple files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/object.c (1)

1318-1336: Stream listpack estimate appears to double‑count the last node.
The comment says “average for first N‑1, then add last,” but Line 1331 multiplies by numele and then adds the last node again. Consider matching the comment to avoid systematic overcounting.

🔧 Proposed fix

-            asize += lpsize * s->rax->numele;
+            asize += lpsize * (s->rax->numele - 1);

🤖 Fix all issues with AI agents

In `@src/object.c`:
- Around line 1216-1218: Replace the non-portable call to malloc_usable_size
with the Redis allocator wrapper: change the line computing asize (size_t asize
= malloc_usable_size((void *)o);) to use zmalloc_usable_size((void *)o) (or the
appropriate zmalloc wrapper provided by your build) so the code uses Redis's
allocator API; update any includes if needed to ensure zmalloc_usable_size is
declared.
- Around line 1231-1239: The code computes average per-node size
(elesize/samples) but multiplies by ql->count (total elements) which
overestimates size; change the multiplier to ql->len (number of quicklist nodes)
and guard against division by zero. Locate the block using quicklist *ql,
quicklistNode *node, variables elesize/samples and replace asize +=
(double)elesize/samples*ql->count with asize += (samples>0)?
(double)elesize/samples*ql->len : 0 (or equivalent) so the scaling uses node
count (ql->len) and avoids divide-by-zero.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d0e57b and b6ee4f1.

📒 Files selected for processing (4)

• src/object.c
• tests/unit/type/hash.tcl
• tests/unit/type/list.tcl
• tests/unit/type/string.tcl

🧰 Additional context used

🧬 Code graph analysis (1)

src/object.c (4)

src/networking.c (1)

• sdsZmallocSize (43-46)

src/zmalloc.c (1)

• zmalloc_size (443-447)

src/dict.c (1)

• dictInitIterator (1129-1137)

src/module.c (1)

• moduleGetMemUsage (12160-12173)

🪛 GitHub Actions: External Server Tests

src/object.c

[error] 1217-1217: Implicit declaration of function 'malloc_usable_size'; did you mean 'zmalloc_usable_size'? [-Werror=implicit-function-declaration]

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: test-sanitizer-address
• GitHub Check: Analyze (cpp)
• GitHub Check: code-coverage
• GitHub Check: build-centos-jemalloc
• GitHub Check: test-sanitizer-address
• GitHub Check: code-coverage

🔇 Additional comments (4)

tests/unit/type/hash.tcl (1)

46-47: Coverage label update is clear.
No concerns.

tests/unit/type/list.tcl (1)

2244-2245: LGTM – coverage probe is reasonable.

tests/unit/type/string.tcl (1)

687-712: Nice coverage expansion for MEMORY USAGE.

src/object.c (1)

1787-1788: LGTM – MEMORY USAGE now routes through kvobjComputeSize.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Build break: replace malloc_usable_size with Redis allocator wrapper.
Line 1217 fails the build (implicit declaration of function 'malloc_usable_size'). Use the zmalloc wrapper to keep portability.

🔧 Proposed fix

-    size_t asize = malloc_usable_size((void *)o);
+    size_t asize = zmalloc_size((void *)o);

📝 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.

Suggested change

	/* All kv-objects has at least kvobj header and embedded key */
	size_t asize = malloc_usable_size((void *)o);
	/* All kv-objects has at least kvobj header and embedded key */
	size_t asize = zmalloc_size((void *)o);

🧰 Tools

🪛 GitHub Actions: External Server Tests

[error] 1217-1217: Implicit declaration of function 'malloc_usable_size'; did you mean 'zmalloc_usable_size'? [-Werror=implicit-function-declaration]

🤖 Prompt for AI Agents

In `@src/object.c` around lines 1216 - 1218, Replace the non-portable call to
malloc_usable_size with the Redis allocator wrapper: change the line computing
asize (size_t asize = malloc_usable_size((void *)o);) to use
zmalloc_usable_size((void *)o) (or the appropriate zmalloc wrapper provided by
your build) so the code uses Redis's allocator API; update any includes if
needed to ensure zmalloc_usable_size is declared.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Quicklist size scaling should use node count, not element count.
Line 1239 samples per-node size but multiplies by ql->count (elements), which inflates MEMORY USAGE for long nodes. Use ql->len.

🔧 Proposed fix

-            asize += (double)elesize/samples*ql->count;
+            asize += (double)elesize/samples*ql->len;

🤖 Prompt for AI Agents

In `@src/object.c` around lines 1231 - 1239, The code computes average per-node
size (elesize/samples) but multiplies by ql->count (total elements) which
overestimates size; change the multiplier to ql->len (number of quicklist nodes)
and guard against division by zero. Locate the block using quicklist *ql,
quicklistNode *node, variables elesize/samples and replace asize +=
(double)elesize/samples*ql->count with asize += (samples>0)?
(double)elesize/samples*ql->len : 0 (or equivalent) so the scaling uses node
count (ql->len) and avoids divide-by-zero.