chore: Fix test_rss_used_mem_gap for all types

[chakaz]

The test fails when it checks the gap between used_memory and object_used_memory, by assuming that all used memory is consumed by the type it DEBUG POPULATEs with.

This assumption is wrong, because there are other overheads, for example the dash table and string keys.

The test failed for types STRING and LIST because they used a larger number of keys as part of the test parameters, which added a larger overhead.

I fixed the parameters such that all types use the same number of keys, and also the same number of elements, modifying only the element sizes (except for STRING which doesn't have sub-elements) so that the overall min_rss requirement of 3.5gb still passes.

Fixes #3723

[adiholden]

So we still have problem with list?

[chakaz]

Yes, Roman recently talked about taking ownership over the lists code. We don't track it too well, and we have a slow version of used memory. It's still WIP.

[kostasrim]

We now got a list class if I remember correctly? It should be easier to bake in "memory tracking"

[adiholden]

so used_memory counts the dash table and the key memory while object_used_memory does not there for the delta is bigger
Not sure why we compare in this test the rss to object_used_memory in this case
does is makes sense to compare it to val_size*elements_count ?

[chakaz]

That's a very good idea. Hopefully the overhead for that could be low and relatively equal for all types.

[chakaz]

Unfortunately, it looks like your proposal (which I liked a lot!) won't fly..

These are the values we use to try to generate 3gb of RSS. All is pretty optimized except for string, I think.

	keys	elements	element sz	total
"JSON"	250,000	100	75	1,875,000,000
"SET"	250,000	100	110	2,750,000,000
"HASH"	250,000	100	100	2,500,000,000
"ZSET"	250,000	100	100	2,500,000,000
"LIST"	250,000	100	125	3,125,000,000
"STRING"	250,000	20,000	1	5,000,000,000
"STREAM"	250,000	100	120	3,000,000,000

So the overhead is quite different, and I don't know if it makes much sense to compare that to anything. I mean, sure, JSON has a huge overhead.. what's there to test?

After thinking about this some more, perhaps we should just remove that last part of the test 🤷

[adiholden]

I believe that by this test we found that we dont account correctly object memory for steams.
Therefore I suggest to ease the check in this test and to check that object_used_memory > keyselementselements_size and object_used_memory < used_memory
This way we will still have some check to catch extreme cases where we somehow drop counting

[kostasrim]

we dont account correctly object memory for steams.

I think it was an issue. See: #4028

This way we will still have some check to catch extreme cases where we somehow drop counting

+1

[adiholden]

@chakaz why do we fail with delta check with the number you had before and just reverted?

[chakaz]

@chakaz why do we fail with delta check with the number you had before and just reverted?

It fails on existing tests (which I haven't modified in this PR):

stream:

>       assert delta < max_unaccounted
E       assert 870676944 < 629145600

string:

>       assert delta < max_unaccounted
E       assert 239981520 < 209715200

I wanted to see if reverting my changes fixes this, or if something changed.

This test is fragile :|

[chakaz]

It still fails, but now with the newly added checks:

JSON:

>       assert info["used_memory"] > info["object_used_memory"]
E       assert 4454321200 > 4456599360

STRING:

>       assert info["object_used_memory"] > keys * elements * val_size
E       assert 3136000000 > ((3500000 * 1) * 1000)

[chakaz]

The string difference if 104 bytes per string. I wonder if this is (partially?) explained by inline-strings

[adiholden]

The string difference if 104 bytes per string. I wonder if this is (partially?) explained by inline-strings

I believe it does.

[chakaz]

It seems like there is a small gap with JSON still. We over-account its memory (very slightly).
@adiholden how would you like me to proceed? I could exclude JSON from this specific check, or look into that (might be a difficult task)

[adiholden]

This is low priority. lets exclude json from this check