Account for shared strings proportionally in JSON memory usage - #22
TalBarYakar wants to merge 2 commits into
Conversation
| } else { | ||
| Self::layout(self.len()).unwrap().size() | ||
| Self::layout(self.len()).unwrap().size() as f64 | ||
| / self.header().rc.load(std::sync::atomic::Ordering::Relaxed) as f64 |
There was a problem hiding this comment.
Please write a SAFETY comment on why we can deliberately divide by the rc value
| /// Shared strings are divided among all live references (including temporary | ||
| /// owners). Container capacity is fully counted; the intern table is excluded. | ||
| /// Retain fractional bytes until rounding the final measurement. | ||
| pub fn mem_allocated(&self) -> f64 { |
There was a problem hiding this comment.
Just verify the callers/client of JSON.DEBUG ... doesn't expect an integer only and this is not an API change on JSON side
There was a problem hiding this comment.
Verified. RedisJSON rounds the final measurement and converts it to an integer. JSON.DEBUG MEMORY still returns an integer (or an array of integers for JSONPath), and MEMORY USAGE also remains integer-valued. Only the accounting changes, not the command response types.
There was a problem hiding this comment.
But can it be changed to return float? If not, I wouldn't return float here wither
There was a problem hiding this comment.
I guess changing it to float is indeed breaking changed we dont want to do , However ,
I think we should return a float. If we sum up many float values, we could end up with a significant number of bytes that would simply be lost if the result were not a float.
There was a problem hiding this comment.
But anyway you say we will round the result. Need to check if we can really return float(I guess we can, redis command api usually dont difference between float and integer AFAIR)
There was a problem hiding this comment.
assume we have 1000 diffrent strings with each 3 ref count
each will have mem usage of X.33 .
If we wiil round it in the ijson path we might ignore 1000*0.3 bytes
I prefer return a regular int
There was a problem hiding this comment.
I prefer to be exact if we can, so let's first understand if there is any issue at all
2 participants
JSON memory reporting currently charges the full string allocation for every reference, even when multiple references share the same allocation.
This change divides each string’s allocation size—including its header and padding—by its global reference count. Arrays and objects sum these contributions, retaining fractional bytes until the final result is rounded to an integer.
This improves memory attribution across documents without an additional deduplication set. Container capacity remains fully counted. It changes reported memory usage, not actual RAM allocation.
Note
Medium Risk
Public API change (
usize→f64) and different reported totals for shared strings; RC-based estimates may race under concurrent clones but do not affect real allocations.Overview
IValue::mem_allocated()now returnsf64and reports proportional dynamic bytes instead of charging the full heap allocation for every handle to the same interned string.Heap
IStringaccounting divides each string’s layout size (header + payload) by its live reference count (bothstring.rsandunsafe_string.rs); inline/empty strings stay at 0.0.IArrayandIObjectpropagatef64sums so nested values keep fractional shares until callers aggregate. Container capacity is still counted in full; the intern table is still excluded. Docs onIValue::mem_allocateddescribe the new semantics.Unit tests and expectations were updated for floating-point results.
tests/memory_share.rsasserts that many documents sharing one string sum to a single allocation, including nested JSON with repeated keys/values.Reviewed by Cursor Bugbot for commit 87aefd1. Bugbot is set up for automated code reviews on this repo. Configure here.