Switch from preallocating all databases to lazy allocation, reducing overhead and improving scalability for large database counts. #1609

xbasel · 2025-01-24T10:37:01Z

ranshid

some immediate comments.

ranshid · 2025-01-24T12:41:24Z

src/aof.c

@@ -2210,7 +2210,8 @@ int rewriteAppendOnlyFileRio(rio *aof) {

    for (j = 0; j < server.dbnum; j++) {
        char selectcmd[] = "*2\r\n$6\r\nSELECT\r\n";
-        serverDb *db = server.db + j;
+        if (server.db[j] == NULL || kvstoreSize(server.db[j]->keys) == 0) continue;
+        serverDb *db = server.db[j];
        if (kvstoreSize(db->keys) == 0) continue;


we can probably remove this check now right?

ranshid · 2025-01-24T12:43:19Z

src/cluster_legacy.c

@@ -5704,7 +5704,7 @@ int verifyClusterConfigWithData(void) {

    /* Make sure we only have keys in DB0. */
    for (j = 1; j < server.dbnum; j++) {
-        if (kvstoreSize(server.db[j].keys)) return C_ERR;
+        if (server.db[j] && kvstoreSize(server.db[j]->keys)) return C_ERR;


this condition will probably repeat many times. suggest we encapsulate in a function?

introduced new function, databaseEmpty

src/db.c

codecov · 2025-01-27T00:24:52Z

Codecov Report

Attention: Patch coverage is 99.17355% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.05%. Comparing base (6657757) to head (c70492e).
Report is 19 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/rdb.c	87.50%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1609      +/-   ##
============================================
+ Coverage     71.01%   71.05%   +0.03%     
============================================
  Files           121      121              
  Lines         65169    65273     +104     
============================================
+ Hits          46281    46377      +96     
- Misses        18888    18896       +8

Files with missing lines	Coverage Δ
src/aof.c	`80.24% <100.00%> (-0.06%)`	⬇️
src/cluster.c	`89.23% <100.00%> (+0.01%)`	⬆️
src/cluster_legacy.c	`86.29% <100.00%> (-0.80%)`	⬇️
src/db.c	`89.62% <100.00%> (+0.04%)`	⬆️
src/debug.c	`52.53% <100.00%> (+0.12%)`	⬆️
src/defrag.c	`89.64% <100.00%> (-0.29%)`	⬇️
src/evict.c	`98.12% <100.00%> (+0.40%)`	⬆️
src/expire.c	`96.61% <100.00%> (+0.01%)`	⬆️
src/object.c	`82.04% <100.00%> (ø)`
src/replication.c	`87.40% <100.00%> (-0.06%)`	⬇️
... and 3 more

... and 19 files with indirect coverage changes

reducing overhead and improving scalability for large database counts. Signed-off-by: xbasel <[email protected]>

xbasel · 2025-01-27T16:58:11Z

Running valkey with 10 million databases:

valkey-server --databases 10000000

empty dbs:

Before:

# Memory
used_memory:6086209848
used_memory_human:5.67G
used_memory_rss:6063915008
used_memory_rss_human:5.65G

After:

# Memory
used_memory:84790224
used_memory_human:80.86M
used_memory_rss:7602176
used_memory_rss_human:7.25M

db 8 million:

# Keyspace
db0:keys=1,expires=0,avg_ttl=0
db8000000:keys=1,expires=0,avg_ttl=0
127.0.0.1:6379[8000000]>

zuiderkwast

Great, this looks pretty strait-forward.

The benchmark with 10M databases is great, but not a very common I think. :) The main motivation for this optimization IMO is in combination with #1319. For cluster mode with 16 empty databases, does it go from 10MB to 1MB?

zuiderkwast · 2025-01-30T20:27:45Z

src/server.c

+// /* Initialize temporary db on replica for use during diskless replication. */
+// serverDb *initTempDb(int id) {
+//     int slot_count_bits = 0;


Not finished?

Should have been removed. createDatabase is not used, including for temp databases. I'll remove.

zuiderkwast · 2025-01-30T20:36:15Z

src/server.c

+void initDatabase(int id) {
+    if (server.db[id] == NULL) {
+        server.db[id] = createDatabase(id);
+    }
+}


In kvstore.c there is lazy creation of hashtables. It is similar to lazy creation of databases. In kvstore, a function called createHashtableIfNeeded is used in many places. It lazy-creates it and returns it.

I think we can use a similar pattern here. The name with "if needed" is also more clear than "init".

Suggested change

void initDatabase(int id) {

if (server.db[id] == NULL) {

server.db[id] = createDatabase(id);

}

}

serverDb *createDatabaseIfNeeded(int id) {

if (server.db[id] == NULL) {

server.db[id] = createDatabase(id);

}

return server.db[id];

}

It can be used for example like this for the SELECT command:

c->db = createDatabaseIfNeeded(id);

zuiderkwast · 2025-01-30T22:58:04Z

src/server.c

+/* Return non-zero if the database is empty  */
+int databaseEmpty(int id) {
+    return id < 0 || id >= server.dbnum || !server.db[id] || kvstoreSize(server.db[id]->keys) == 0;


Include "is" to make the function name more clear, like databaseIsEmpty. Without it, it may looks like the function empties the database. We have some other functions like emptyData that does that.

It's not forbidded to use bool anymore. We made a decision to allow it. Using int for booleans is also fine though.

Even if a database has no keys, it can have clients waiting for keys to be created (blocked on BLPOP), watched keys, etc., so the db structure it is not completely empty. Isn't what we need just to check if the database has been created or not, i.e. just check server.db[id] != NULL? In that case, we can call it databaseIsCreated or databaseExists instead.

The naming is bad here. This function is used to check if a certain db has no keys. I've replaced with:

bool dbHasNoKeys(int dbid)

eg. https://github.com/valkey-io/valkey/pull/1609/files#diff-ade042c73ad8dc4e34f1a75b2a9e26a5342d1dee539a6188c2565feea0372a20L2214

xbasel · 2025-02-04T13:37:08Z

Great, this looks pretty strait-forward.

The benchmark with 10M databases is great, but not a very common I think. :) The main motivation for this optimization IMO is in combination with #1319. For cluster mode with 16 empty databases, does it go from 10MB to 1MB?

This has been created artificially (this change hasn't been merged into multi-database code, i changed the code to create 16k slots without cluster mode)

before (preallocated) with (16 databases, 16k slots):

# Memory
used_memory:15887960
used_memory_human:15.15M
used_memory_rss:15204352
used_memory_rss_human:14.50M
used_memory_peak:15887960
used_memory_peak_human:15.15M

allocated-on-demand (16 databases, 16k slots), database 0 is always pre-allocated..

# Memory
used_memory:7327440
used_memory_human:6.99M
used_memory_rss:14548992
used_memory_rss_human:13.88M

Signed-off-by: xbasel <[email protected]>

ranshid reviewed Jan 24, 2025

View reviewed changes

xbasel force-pushed the lazyinit branch from 12ef00e to 33da0ea Compare January 27, 2025 00:09

xbasel force-pushed the lazyinit branch 2 times, most recently from 1bfa8b4 to 9f07a4f Compare January 27, 2025 08:49

xbasel requested a review from ranshid January 27, 2025 09:14

xbasel force-pushed the lazyinit branch from 26256e0 to 25028e4 Compare January 27, 2025 14:48

Switch from preallocating all databases to lazy allocation,

028c7df

reducing overhead and improving scalability for large database counts. Signed-off-by: xbasel <[email protected]>

xbasel force-pushed the lazyinit branch from 25028e4 to 028c7df Compare January 27, 2025 15:15

xbasel mentioned this pull request Jan 27, 2025

[BUG] The engine isn't usable when running with a large number of databases. #1597

Open

zuiderkwast reviewed Jan 30, 2025

View reviewed changes

xbasel requested a review from zuiderkwast February 4, 2025 14:31

xbasel force-pushed the lazyinit branch from de09b94 to f553b46 Compare February 4, 2025 15:04

changes following code review.

c70492e

Signed-off-by: xbasel <[email protected]>

xbasel force-pushed the lazyinit branch from f553b46 to c70492e Compare February 4, 2025 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from preallocating all databases to lazy allocation, reducing overhead and improving scalability for large database counts. #1609

Switch from preallocating all databases to lazy allocation, reducing overhead and improving scalability for large database counts. #1609

xbasel commented Jan 24, 2025 •

edited

Loading

ranshid left a comment

ranshid Jan 24, 2025

ranshid Jan 24, 2025

xbasel Jan 27, 2025

codecov bot commented Jan 27, 2025 •

edited

Loading

xbasel commented Jan 27, 2025 •

edited

Loading

zuiderkwast left a comment

zuiderkwast Jan 30, 2025

xbasel Feb 4, 2025

zuiderkwast Jan 30, 2025

zuiderkwast Jan 30, 2025

xbasel Feb 4, 2025

xbasel commented Feb 4, 2025

Switch from preallocating all databases to lazy allocation, reducing overhead and improving scalability for large database counts. #1609

Are you sure you want to change the base?

Switch from preallocating all databases to lazy allocation, reducing overhead and improving scalability for large database counts. #1609

Conversation

xbasel commented Jan 24, 2025 • edited Loading

ranshid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 27, 2025 • edited Loading

Codecov Report

xbasel commented Jan 27, 2025 • edited Loading

zuiderkwast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xbasel commented Feb 4, 2025

xbasel commented Jan 24, 2025 •

edited

Loading

codecov bot commented Jan 27, 2025 •

edited

Loading

xbasel commented Jan 27, 2025 •

edited

Loading