Parallelize ChannelMonitor loading from async KVStores#4147
Parallelize ChannelMonitor loading from async KVStores#4147TheBlueMatt merged 5 commits intolightningdevkit:mainfrom
ChannelMonitor loading from async KVStores#4147Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
![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 86.63% 86.59% -0.04%
==========================================
Files 158 158
Lines 102066 102179 +113
Branches 102066 102179 +113
==========================================
+ Hits 88422 88485 +63
- Misses 11233 11278 +45
- Partials 2411 2416 +5
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
82cbb76 to
8873d1c
Compare
|
Squashed and pushed with only reverting the change to make |
TheBlueMatt
force-pushed
the
2025-10-parallel-reads
branch
from
8873d1c to
5444e76
Compare
|
Oops, no, it actually has to be |
TheBlueMatt
force-pushed
the
2025-10-parallel-reads
branch
from
5444e76 to
b47ea70
Compare
|
Also silenced clippy: $ git diff-tree -U1 5444e7613 986a3caa8
diff --git a/lightning/src/util/native_async.rs b/lightning/src/util/native_async.rs
index 32c6049f3b..0c380f2b1d 100644
--- a/lightning/src/util/native_async.rs
+++ b/lightning/src/util/native_async.rs
@@ -82,3 +82,10 @@ impl<O> Clone for FutureQueueCompletion<O> {
fn clone(&self) -> Self {
- Self(self.0.clone())
+ #[cfg(all(test, feature = "std"))]
+ {
+ Self(Arc::clone(&self.0))
+ }
+ #[cfg(all(test, not(feature = "std")))]
+ {
+ Self(Rc::clone(&self.0))
+ }
} |
TheBlueMatt
force-pushed
the
2025-10-parallel-reads
branch
from
b47ea70 to
986a3ca
Compare
|
✅ Added second reviewer: @wpaulino |
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
986a3ca to
6ef22ba
Compare
|
Rebased to fix a trivial conflict and addressed the spelling nit from @wpaulino |
|
Changes since @wpaulino's ack were incredibly trivial, so just landing. |
4 participants
Based on
#4146#4175just cause I don't want to resolve conflicts.