Add overhead of all DBs and rehashing dict count to info.

[CharlesChen888]

Sometimes we need to make fast judgement about why Redis is suddenly taking more memory. One of the reasons is main DB's dicts doing rehashing.

We may use MEMORY STATS to monitor the overhead memory of each DB, but there still lacks a total sum to show an overall trend. So this PR adds the total overhead of all DBs to INFO MEMORY section, together with the total count of rehashing DB dicts, providing some intuitive metrics about main dicts rehashing.

This PR adds the following metrics to INFO MEMORY

• mem_overhead_db_hashtable_rehashing - only size of ht[0] in dictionaries we're rehashing (i.e. the memory that's gonna get released soon)

and a similar ones to MEMORY STATS:

• overhead.db.hashtable.lut (complements the existing overhead.hashtable.main and overhead.hashtable.expires which also counts the dictEntry structs too)
• overhead.db.hashtable.rehashing - temporary rehashing overhead.
• db.dict.rehashing.count - number of top level dictionaries being rehashed.

[oranagra]

good idea. but i think i'd like a few changes:

1. i don't think we need to have separate metric for the expires dict and the main one, we can count them together in the same metric.
2. i'd like another metric to count the overhead of the rehashing (i.e. the total memory used by ht[0] in dicts that also have ht[1])

WDYT?

[soloestoy]

i'd like another metric to count the overhead of the rehashing (i.e. the total memory used by ht[0] in dicts that also have ht[1])

I like this idea, but one concern is that cluster with 16384 slots, in the worst case scenario, we would need to iterate through all the slots to obtain complete information about ht[0] and ht[1]. Perhaps we need to use rehashingStarted and rehashingCompleted to optimize this process.

[oranagra]

yes, let's do that (keep track of it rather than calculate it on the spot)

[oranagra]

@redis/core-team please approve the new metrics for INFO MEMORY and MEMORY STATS

[CharlesChen888]

Now that "kvstore" is merged, I have to rewrite a lot of things.

mem_overhead_hashtable_rehashing and mem_overhead_hashtable_lut are now maintained in kvstoreDictRehashingStarted and kvstoreDictRehashingCompleted, and therefore it is inconvenient to tell whether the rehashing dict is from DBs or from PUBSUB dicts. But I guess including the overhead of pubsub is somehow OK? Sometimes pubsub also takes a lot of memory and we need to check it.

And since databases_rehashing_dict_count may not be so useful and now we need to iterate all DBs to get the exact count, I temporarily dropped it.

Or we can maintain the overhead inside kvstore structure and when info is called, we gather the overhead from all DBs' kvstore. And since in this case we have to iterate all DBs, we can keep the databases_rehashing_dict_count.

[CharlesChen888]

Just realized that in test "Redis can resize empty dict" in other.tcl, it is assumed that memory stats shows only the overhead of non-empty DBs. But now all DBs' overhead is displayed and so we need to change the test.

[oranagra]

Just realized that in test "Redis can resize empty dict" in other.tcl, it is assumed that memory stats shows only the overhead of non-empty DBs. But now all DBs' overhead is displayed and so we need to change the test.

i see the test passes, what do you mean?
the deletion of this?

        # Set a key to enable overhead display of db 0
        r set a b

or do you mean we need another way to distinguish between empty and non empty dicts?

[CharlesChen888]

i see the test passes, what do you mean?

@oranagra

The test should read overhead.hashtable.main of DB9 (the DB whose dict expanded and then emptied). In the test, it use to be that only DB9 has a key (r set a b) and so memory stats shows only DB9's overhead. In this case we only need to find the first overhead.hashtable.main field and read its value.

However in this PR getMemoryOverheadData is modified and memory stats shows not only non-empty DBs but all DBs, (#12913 (comment)), so we need to specify it is the DB9's overhead that we need to read.

The test would also pass if it reads other DB's overhead, but that's meaningless.

[oranagra]

ohh, you mean that so far it just looked for any overhead.hashtable.main, and you now modified it to be db.9 overhead.hashtable.main.
ok, so that's solved then.

let's wait to see if @soloestoy has anything else to argue and then merge it.

[CharlesChen888]

@soloestoy Any more problems?

[soloestoy]

@oranagra please approve the new naming database.dict.rehashing.count and the change about kvstoreMemUsage

[oranagra]

i'm ok with the new name.
please update the top comment, and prepare a redis-doc PR

[soloestoy]

My OCD is kicking... I feel it's best to add "db" to the names of these metrics to avoid the misunderstanding that they refer to all hash data types:

INFO MEMORY

• mem_overhead_hashtable_rehashing -> mem_overhead_db_hashtable_rehashing

MEMORY STATS

• overhead.hashtable.lut -> overhead.db.hashtable.lut
• overhead.hashtable.rehashing -> overhead.db.hashtable.rehashing

@oranagra @madolson WDYT?

about the db prefix error, @CharlesChen888 could you change database.dict.rehashing.count to db.dict.rehashing.count and modify the regular expression in memory-stats.json to "^db\\.\\d+$", then try again?

[oranagra]

LGTM

[madolson]

Just retrospectively acknowledging that the updating wording seems better to me as well.