Parallelize ChannelMonitor loading from async KVStores
#4147
Conversation
|
🎉 This PR is now ready for review! |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4147 +/- ##
==========================================
- Coverage 89.32% 89.28% -0.05%
==========================================
Files 180 180
Lines 139132 139244 +112
Branches 139132 139244 +112
==========================================
+ Hits 124281 124325 +44
- Misses 12227 12283 +56
- Partials 2624 2636 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TheBlueMatt
force-pushed
the
2025-10-parallel-reads
branch
from
4a773c9 to
31fbd16
Compare
|
This doesn't actually work. monitor loading is (at least for a semi-local disk) CPU-bound, so we really need to spawn each monitor load task rather than having one task. |
TheBlueMatt
force-pushed
the
2025-10-parallel-reads
branch
3 times, most recently
from
64412a5 to
2c807b9
Compare
TheBlueMatt
force-pushed
the
2025-10-parallel-reads
branch
from
2c807b9 to
0a2547a
Compare
|
This requires GATs which requires an MSRV bump (to 1.64), but we're planning on doing that soon anyway. |
TheBlueMatt
force-pushed
the
2025-10-parallel-reads
branch
from
0a2547a to
a4a778c
Compare
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
TheBlueMatt
force-pushed
the
2025-10-parallel-reads
branch
from
a4a778c to
3346cba
Compare
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 12th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 13th Reminder Hey @jkczyz! This PR has been waiting for your review. |
TheBlueMatt
force-pushed
the
2025-10-parallel-reads
branch
2 times, most recently
from
ba55b75 to
5e45e0e
Compare
|
Rebased on #4175, which avoids some further conflicts. |
TheBlueMatt
force-pushed
the
2025-10-parallel-reads
branch
from
5e45e0e to
0503f6b
Compare
|
Marking draft until the parent is marged. |
Reading `ChannelMonitor`s on startup is one of the slowest parts of
LDK initialization. Now that we have an async `KVStore`, there's no
need for that, we can simply paralellize their loading, which we do
here.
Sadly, because Rust futures are pretty unergonomic, we have to add
some `unsafe {}` here, but arguing its fine is relatively
straightforward.
`tokio::spawn` can be use both to spawn a forever-running background task or to spawn a task which gets `poll`ed independently and eventually returns a result which the callsite wants. In LDK, we have only ever needed the first, and thus didn't bother defining a return type for `FutureSpawner::spawn`. However, in the next commit we'll start using `FutureSpawner` in a context where we actually do want the spawned future's result. Thus, here, we add a result output to `FutureSpawner::spawn`, mirroring the `tokio::spawn` API.
`MonitorUpdatingPersister::read_all_channel_monitors_with_updates` was made to do the IO operations in parallel in a previous commit, however in practice this doesn't provide material parallelism for large routing nodes. Because deserializing `ChannelMonitor`s is the bulk of the work (when IO operations are sufficiently fast), we end up blocked in single-threaded work nearly the entire time. Here, we add an alternative option - a new `read_all_channel_monitors_with_updates_parallel` method which uses the `FutureSpawner` to cause the deserialization operations to proceed in parallel.
When reading `ChannelMonitor`s from a `MonitorUpdatingPersister` on startup, we have to make sure to load any `ChannelMonitorUpdate`s and re-apply them as well. For users of async persistence who don't have any `ChannelMonitorUpdate`s (e.g. because they set `maximum_pending_updates` to 0 or, in the future, we avoid persisting updates for small `ChannelMonitor`s), this means two round-trips to the storage backend, one to load the `ChannelMonitor` and one to try to read the next `ChannelMonitorUpdate` only to have it fail. Instead, here, we use `KVStore::list` to fetch the list of stored `ChannelMonitorUpdate`s, which for async `KVStore` users allows us to parallelize the list of update fetching and the `ChannelMonitor` loading itself. Then we know exactly when to stop reading `ChannelMonitorUpdate`s, including reading none if there are none to read. This also avoids relying on `KVStore::read` correctly returning `NotFound` in order to correctly discover when to stop reading `ChannelMonitorUpdate`s.
When reading `ChannelMonitor`s from a `MonitorUpdatingPersister` on startup, we have to make sure to load any `ChannelMonitorUpdate`s and re-apply them as well. Now that we know which `ChannelMonitorUpdate`s to load from `list`ing the entries from the `KVStore` we can parallelize the reads themselves, which we do here. Now, loading all `ChannelMonitor`s from an async `KVStore` requires only three full RTTs - one to list the set of `ChannelMonitor`s, one to both fetch the `ChanelMonitor` and list the set of `ChannelMonitorUpdate`s, and one to fetch all the `ChannelMonitorUpdate`s (with the last one skipped when there are no `ChannelMonitorUpdate`s to read).
TheBlueMatt
force-pushed
the
2025-10-parallel-reads
branch
from
0503f6b to
15706ef
Compare
|
Okay this is ready now. |
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
2 participants
Based on
#4146#4175just cause I don't want to resolve conflicts.