@oranagra I humbly beg that you compare your 2 to mine - https://github.com/antirez/redis/pull/4883 - iterating is slow :)

@itamarhaber thanks, indeed my script counting code would have slowed down INFO in cases were there are many many scripts.
Removing this commit from my branch. @antirez please take #4883 instead.

Thank you, other one merged.

Reopening this one as I think there is more meat to merge.

@antirez yes, this PR contained 4 commits, one of which was overlapping another PR (which was better in some aspects), so i removed that commit from this PR (now contains 3 commits).
you may decide to merge just the first one, first two, or all three.

@antirez a reminder about this one in case you meant to review and forgot. also do you want me to resolve that minor conflict that was created by merging the other PR, or will you handle that in case you merge?

Thanks for bumping this on the list! I'll check tomorrow, a rebase would be appreciated, and sorry for breaking the patch merging other stuff.

P.S. I wish Github could warn users about "merging this will make X,Y,Z impossible to merge". For instance a few days ago I merged a typos PR that ruined a number of other PRs...

resolved (push -f). the conflicts were created by another PR i submitted (which i intended you to merge only if you decide not to merge this one).

i share the same pain about merge conflicts (mainly when cherry picking between branches).. many times i prefer not to fix types and formatting problems (white space and others), or even not to refactor ugly code (if it's bug free), so that it won't cause conflicts.

Hi @oranagra, I was checking the first commit implementing the argv memory reporting.
Here we are ending with two similar functions:

1. size_t getStringObjectSize(robj *o);
2. size_t getStringObjectSdsUsedMemory(robj *o):

The first is introduced by your commit, the second is already in the source tree.
The difference between the two should be that one includes external fragmentation and one does not.

In turn the two calls two different functions to return the memory used by an SDS string:

1. size_t sdsAllocSize(sds s);
2. size_t sdsZmallocSize(sds s);

The first returns the SDS logical memory used: header + size of the allocation including the final null term. The second instead reports the size of the allocation where the SDS is stored.

I wonder if we need both (probably yes, because sometimes we want to understand the total memory usage and sometimes just what the higher level API tried to allocate), but anyway the naming scheme of such functions look very confusing. So I think here there is a good chance to fix this.

I would change them in the following way:

• sdsAllocSize() -> sdsLogicalSize()
• sdsZmallocSize() -> sdsAllocationSize()
• getStringObjectSize() -> getStringObjectLogicalSize()
• getStringObjectSdsUsedMemory() -> getStringObjectAllocationSize()

WDYT?

@antirez thanks for reviewing.
to me, considering we have sdslen, and sdsfree, i would expect their sum to be named "alloc", but maybe "size" would be ok too (i'm worried it "size" may be confused with "len").
also, considering we have many calls to zmalloc_size in the project (to get the real allocated size including internal fragmentation), it sounds logical to me that the sds function doing that will be named "zmalloc", this would not look good in the sds repo, but this may be the reason why this function is in networking.c and not sds.c.

anyway, this is just naming, so after the above arguments, feel free to decide what sounds best to you, and either rename yourself, or ask me to fix the PR.

however, like i did several times in the past, this PR introduces the changes in several stages (so that you can decide to merge just the first commit, or first two, without merging the last).
but if you do decide to merge the last commit, then the difference between these two functions disappears, since now any sds will take responsibility of it's internal fragmentation.

the reason i didn't use sdsZmallocSize in the first commit, and instead introduced a new function that reports the "logical" size without fragmentation, was that i didn't want the performance overhead of calling sdsZmallocSize. but the 3rd commit changes sds.c in a way that every sds is now aware of it's internal fragmentation.
The two benefits of doing that are:

1. memory reporting becomes implicitly more accurate.
2. less cases were the sds needs to be regrown / realloced.

p.s. in the light of the recent #5397, i've reviewed my second commit and i see two things that need fixing:

1. object.c line 783 (OBJ_ENCODING_EMBSTR) we need to call getStringObjectSdsUsedMemory
2. object.c line 1318 (when MEMORY USAGE adds the keyname), we need to either call getStringObjectSdsUsedMemory, or preferably, do that on the key in the dict rather than the one in argv (anyway, we can use sdsZmallocSize on obj->ptr).

so anyway, please finish your review and decide what parts you want to merge.
then either fix / refactor what you wish during merge. or if you prefer, tell me what you want to fix / merge and i'll update the PR.

Thank you @oranagra. A few remarks:

1. I've the feeling that the argv accounting change may be better like that to avoid a cache miss:

      c->argv_bytes += bulklen;

I think that this preserves completely the goal of your original commit, that is to understand if some client is doing crazy stuff. However I've the feeling that calling the function and potentially having to cache-miss, may have an impact in certain work loads.

Otherwise I'm taking the commit exactly like you proposed it, with the changes needed to apply it (since we accumulated other differences).

2. MEMORY USAGE -> I'm positive about this change, even if it's an odd API change. In some way one could argue that MEMORY USAGE should report the logical memory used by the system, so that different versions of Redis running with different allocators will always report the same numbers for objects allocated in a similar way. However the reality is that such a tool is especially useful for provisioning and debugging, so IMHO such "purity" is pointless, and is better to go for your approach.

3. The SDS change looks good as well to me, since this way we may use the full potential of the allocation. However I wonder why we don't juse use the API provided by malloc_size() & co, so that we have to export a single additional function to SDS. I'm not sure there is much gain in doing it in a single function, since anyway such function is doing another function call regardless. Moreover it would be important to be able to avoid setting such API in sdsalloc.h, so that SDS can work without that. And this is how we should ship SDS from upstream (https://github.com/antirez/sds) for max compatibility by default.

Ok that's the idea about the point "1" above: https://github.com/antirez/redis/compare/argv-accounting

@antirez i'm not 100% sure i follow you, i'll try to respond, but if you feel that i lost you, please help me get back on the track.

your 1,2,3 are talking on each of the 3 commits, right?
in that case:

1. indeed the main purpose of this PR is to be able to detect that crazy client(s), so in that sense, the sdslen may be enough, but if possible i would like to get a little bit closer to the trough, since sdslen is not only missing the fragmentation, but also the "free" portion of the sds, which may be very big.
   this is why my fist commit used sdsAllocSize, rather than sdslen... i.e. i wanted to avoid the unknown cost of zmalloc_size, and use only data that is present in the sds header anyway.
   I'm not sure why you consider sdsAllocSize another cache miss more than sdslen?
   both variables are part of the sds header.
   Do you mean code cache? maybe you prefer to use sdsalloc instead of sdsAllocSize?
   since sdsalloc is in in sds.h and is most likely inlined by the compiler? (we can probably move sdsAllocSize and sdsHdrSizeto sds.h too)

besides that, IIUC your code from the above mentioned branch, it looks like c->argv_bytes is mixing values from various sources, in which case there may be a creeping leak. i.e. if one place adds sdslen, and the other adds or deducts getStringObjectSdsUsedMemory. we must make sure we always use the same source.

2. i agree, the only sensible use for MEMORY USAGE is to try and understand used_memory (from INFO MEMORY), and both should be of the same units (with internal fragmentation).

3. what i wanted to avoid in that commit, in not the extra call to a function in zmalloc.c, but the extra call to the allocator library. i don't want two consecutive calls to malloc_size and it's unknown cost. especially if we have it for free.
   so i'm willing to slightly "pollute" the cleanness of the standalone sds repository, for a slightly better redis.

@antirez you may have missed my response above.
where do we stand about this? do you want to add this info to redis one way or another?

@oranagra thanks, yes checking those as well, started from the bug fixes to release a new RC ASAP.

Closing this old PR, changes and above feedback where ported to the above 3 mentioned PRs