Avoid heap-allocating background processor futures#4733
Conversation
Abeeujah
commented
Jun 20, 2026
Abeeujah
commented
|
I've assigned @valentinewallace as a reviewer! |
|
I've re-verified the current file against the diff and my prior review. The code is unchanged and my previous analysis holds. No issues found. All previously verified concerns remain correctly handled:
The only outstanding note (non-blocking, already raised in a prior pass) is the non-standard line break on |
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
| // Captured by the ChannelManager persistence future below. Only meaningful when | ||
| // `needs_cm_persist` is set, declared before the futures so it outlives them | ||
| // (drop order). | ||
| let pending_monitor_writes = |
There was a problem hiding this comment.
This can stay in the if/future and avoid the if.
| let mut waker = dummy_waker(); | ||
| let mut ctx = task::Context::from_waker(&mut waker); | ||
| match core::pin::Pin::new(&mut fut).poll(&mut ctx) { | ||
| task::Poll::Ready(res) => futures.set_a_res(res), |
There was a problem hiding this comment.
I don't believe there's any reason to change this, it would probably be simpler to keep the code structured the way it was.
There was a problem hiding this comment.
I've simplified it, although, I couldn't fully restore the original now that the futures are stack-pinned, Joiner holds Pin<&mut> references into them and so must be declared after all of them.
Meaning it doesn't exist yet at the eager-poll site, so the outcome has to be stashed and fed in once the Joiner is constructed. Open to a cleaner approach if you have any recommendations.
There was a problem hiding this comment.
Ah, I had only tested a smaller patch and was hoping it would move up, but at least this reduces patch size marginally:
diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index b2b0c8d9b2..f2b3cdd183 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -1122,2 +1122,4 @@ where
+ let mut futures = Joiner::new();
+
let needs_cm_persist = channel_manager.get_cm().get_and_clear_needs_persistence();
@@ -1149,3 +1151,2 @@ where
// outcome and feed it into the `Joiner` once it is constructed.
- let mut cm_persist_res = None;
if needs_cm_persist {
@@ -1156,4 +1157,5 @@ where
let mut ctx = task::Context::from_waker(&mut waker);
- if let task::Poll::Ready(res) = cm_fut.as_mut().poll(&mut ctx) {
- cm_persist_res = Some(res);
+ match cm_fut.as_mut().poll(&mut ctx) {
+ task::Poll::Ready(res) => futures.set_a_res(res),
+ task::Poll::Pending => futures.set_a(cm_fut),
}
@@ -1283,2 +1285,4 @@ where
});
+ futures.set_b(network_graph_fut);
+
let scorer_fut =
@@ -1302,2 +1306,4 @@ where
});
+ futures.set_c(scorer_fut);
+
let sweeper_fut = core::pin::pin!(async {
@@ -1310,2 +1316,4 @@ where
});
+ futures.set_d(sweeper_fut);
+
let lm_fut = core::pin::pin!(async {
@@ -1329,12 +1337,2 @@ where
});
-
- let mut futures = Joiner::new();
- match cm_persist_res {
- Some(res) => futures.set_a_res(res),
- None if needs_cm_persist => futures.set_a(cm_fut),
- None => {},
- }
- futures.set_b(network_graph_fut);
- futures.set_c(scorer_fut);
- futures.set_d(sweeper_fut);
futures.set_e(lm_fut);There was a problem hiding this comment.
Yeah, thanks, this works 👍
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4733 +/- ##
==========================================
+ Coverage 84.55% 86.95% +2.39%
==========================================
Files 137 161 +24
Lines 77617 111645 +34028
Branches 77617 111645 +34028
==========================================
+ Hits 65627 97076 +31449
- Misses 9948 12061 +2113
- Partials 2042 2508 +466
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Abeeujah
force-pushed
the
drop-box
branch
2 times, most recently
from
16d7eef to
f91e367
Compare
|
🔔 2nd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
Replace Box::pin with core::pin::pin! in process_events_async now that MSRV is 1.75. This eliminates a heap allocation per task on every loop iteration by pinning the futures directly to the stack. To satisfy lifetime and Joiner bounds, the loop logic was refactored to run synchronous timer checks first, using flags to conditionally execute the stack-pinned futures. Existing eager polling and early-break semantics are preserved.
5 participants
