Skip to content

Fix MEMORY USAGE command#5

Open
tomerqodo wants to merge 2 commits intoaugment_full_base_fix_memory_usage_command_pr5from
augment_full_head_fix_memory_usage_command_pr5
Open

Fix MEMORY USAGE command#5
tomerqodo wants to merge 2 commits intoaugment_full_base_fix_memory_usage_command_pr5from
augment_full_head_fix_memory_usage_command_pr5

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#5

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented Jan 26, 2026

🤖 Augment PR Summary

Summary: Fixes MEMORY USAGE reporting to better account for the actual in-memory footprint of key/value objects.

Changes:

  • Renamed and updated the size estimator to kvobjComputeSize, aiming to include kv-object header + embedded key (and embedded value where applicable).
  • Adjusted per-type memory accounting for strings, lists, sets, zsets, hashes, streams, and module values.
  • Updated MEMORY USAGE command implementation to use the new estimator.
  • Refreshed unit-test coverage comments and added a new string-focused MEMORY USAGE test.

Technical Notes: The estimator relies on allocator-reported allocation sizes and sampling to approximate nested container memory usage.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 3 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/object.c
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);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kvobjComputeSize uses malloc_usable_size(), which may not be available / may not match the allocator Redis is built with (e.g., jemalloc uses je_malloc_usable_size via zmalloc_*). This can break portability and/or make MEMORY USAGE inconsistent with zmalloc_size-based accounting used elsewhere.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Comment thread src/object.c
samples++;
} while ((node = node->next) && samples < sample_size);
asize += (double)elesize/elecount*ql->count;
asize += (double)elesize/samples*ql->count;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the quicklist branch, asize += (double)elesize/samples*ql->count appears to mix “per-node” averaging (/samples) with “total element count” scaling (*ql->count), which can massively mis-estimate list memory usage. It looks like this should be normalized consistently (e.g., per-element vs per-node) before scaling.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Comment thread src/object.c
} else {
if (samples) lpsize /= samples; /* Compute the average. */
asize += lpsize * (s->rax->numele-1);
asize += lpsize * s->rax->numele;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the stream branch, asize += lpsize * s->rax->numele is followed by adding the actual size of the last listpack, which seems to double-count the last listpack (the old code used numele-1 before adding the last). This likely inflates MEMORY USAGE results for streams.

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants