Skip to content

Conversation

@xbasel
Copy link
Member

@xbasel xbasel commented Jan 24, 2025

Allocate database structures lazily to prevent excessive memory usage
when a large number of databases is configured but not actually used.

fixes #1597

Copy link
Member

@ranshid ranshid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some immediate comments.

@xbasel xbasel changed the title [DRAFT] Switch from preallocating all databases to lazy allocation, reducing overhead and improving scalability for large database counts. Switch from preallocating all databases to lazy allocation, reducing overhead and improving scalability for large database counts. Jan 27, 2025
@codecov
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

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

Project coverage is 71.53%. Comparing base (3789b29) to head (57573e5).
Report is 10 commits behind head on unstable.

Files with missing lines Patch % Lines
src/replication.c 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1609      +/-   ##
============================================
+ Coverage     71.43%   71.53%   +0.09%     
============================================
  Files           122      122              
  Lines         66210    66484     +274     
============================================
+ Hits          47300    47561     +261     
- Misses        18910    18923      +13     
Files with missing lines Coverage Δ
src/aof.c 80.40% <ø> (+0.08%) ⬆️
src/cluster.c 90.38% <100.00%> (ø)
src/cluster_legacy.c 86.79% <100.00%> (+0.07%) ⬆️
src/db.c 90.07% <100.00%> (-0.01%) ⬇️
src/debug.c 53.58% <100.00%> (+0.12%) ⬆️
src/defrag.c 81.59% <100.00%> (-1.04%) ⬇️
src/evict.c 98.50% <100.00%> (+0.02%) ⬆️
src/expire.c 96.62% <100.00%> (+0.02%) ⬆️
src/object.c 81.41% <100.00%> (ø)
src/rdb.c 76.89% <100.00%> (+0.56%) ⬆️
... and 3 more

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xbasel xbasel force-pushed the lazyinit branch 2 times, most recently from 1bfa8b4 to 9f07a4f Compare January 27, 2025 08:49
@xbasel xbasel requested a review from ranshid January 27, 2025 09:14
@xbasel xbasel changed the title Switch from preallocating all databases to lazy allocation, reducing overhead and improving scalability for large database counts. [DRAFT] Switch from preallocating all databases to lazy allocation, reducing overhead and improving scalability for large database counts. Jan 27, 2025
reducing overhead and improving scalability for large database counts.

Signed-off-by: xbasel <[email protected]>
@xbasel
Copy link
Member Author

xbasel commented Jan 27, 2025

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]>

@xbasel xbasel changed the title [DRAFT] Switch from preallocating all databases to lazy allocation, reducing overhead and improving scalability for large database counts. Switch from preallocating all databases to lazy allocation, reducing overhead and improving scalability for large database counts. Jan 27, 2025
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@xbasel
Copy link
Member Author

xbasel commented Feb 4, 2025

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

@xbasel
Copy link
Member Author

xbasel commented Feb 17, 2025

@zuiderkwast / @ranshid reminder :)

@zuiderkwast
Copy link
Contributor

It's not forgotten, but the focus now is on stabilizing the 8.1 release and fix the flaky tests before 8.1 GA. This one will have to wait until after 8.1. It will be good to release it together with the related cluster-multidb feature.

@ranshid ranshid added performance cluster release-notes This issue should get a line item in the release notes labels Feb 17, 2025
@soloestoy
Copy link
Member

I see many changes from server.db->keys to server.db[0]->keys, such as in countKeysInSlot. This will conflict with multi-databases. I think this PR should be aligned after the multi-databases merge.

xbasel added 2 commits May 9, 2025 23:37
Signed-off-by: xbasel <[email protected]>
Signed-off-by: xbasel <[email protected]>
@xbasel xbasel marked this pull request as ready for review May 9, 2025 21:17
xbasel and others added 3 commits May 11, 2025 17:32
Co-authored-by: Viktor Söderqvist <[email protected]>
Signed-off-by: xbasel <[email protected]>
Signed-off-by: xbasel <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
Signed-off-by: xbasel <[email protected]>
@xbasel xbasel requested a review from zuiderkwast May 26, 2025 10:01
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this looks good to merge.

xbasel and others added 3 commits June 5, 2025 14:17
Signed-off-by: xbasel <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
Signed-off-by: xbasel <[email protected]>
xbasel and others added 2 commits June 10, 2025 10:59
Co-authored-by: Viktor Söderqvist <[email protected]>
Signed-off-by: xbasel <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
@zuiderkwast zuiderkwast merged commit 8e2f355 into valkey-io:unstable Jun 10, 2025
61 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Valkey 9.0 Jun 10, 2025
chzhoo pushed a commit to chzhoo/valkey that referenced this pull request Jun 12, 2025
Allocate database structures lazily to prevent excessive memory usage
when a large number of databases is configured but not actually used.

fixes valkey-io#1597

---------

Signed-off-by: xbasel <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
Signed-off-by: chzhoo <[email protected]>
shanwan1 pushed a commit to shanwan1/valkey that referenced this pull request Jun 13, 2025
Allocate database structures lazily to prevent excessive memory usage
when a large number of databases is configured but not actually used.

fixes valkey-io#1597

---------

Signed-off-by: xbasel <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
Signed-off-by: shanwan1 <[email protected]>
enjoy-binbin added a commit to enjoy-binbin/valkey that referenced this pull request Jul 26, 2025
In valkey-io#1609, we now doing on-demand database allocation instead
of preallocation. And in beginDefragCycle, we should not skip
empty databases if they exist. The defrag still defrags the
internal allocations of the hashtables structs if they exist.

Call chain: defragStageDbKeys -> defragStageKvstoreHelper ->
kvstoreHashtableDefragTables -> hashtableDefragTables.

Signed-off-by: Binbin <[email protected]>
enjoy-binbin added a commit that referenced this pull request Jul 27, 2025
In #1609, we now doing on-demand database allocation instead
of preallocation. And in beginDefragCycle, we should not skip
empty databases if they exist. The defrag still defrags the
internal allocations of the hashtables structs if they exist.

Call chain: defragStageDbKeys -> defragStageKvstoreHelper ->
kvstoreHashtableDefragTables -> hashtableDefragTables.


Signed-off-by: Binbin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance release-notes This issue should get a line item in the release notes run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

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

5 participants