Skip to content

Avoid heap-allocating background processor futures#4733

Open
Abeeujah wants to merge 1 commit into
lightningdevkit:mainfrom
Abeeujah:drop-box
Open

Avoid heap-allocating background processor futures#4733
Abeeujah wants to merge 1 commit into
lightningdevkit:mainfrom
Abeeujah:drop-box

Conversation

@Abeeujah

Copy link
Copy Markdown
Contributor
The persistence loop in process_events_async boxed each task future
(ChannelManager, network graph, scorer, sweeper, LiquidityManager)
before handing it to the Joiner, incurring a heap allocation per task
on every iteration. Box::pin was only needed to satisfy the Joiner's
Unpin bound when our MSRV predated core::pin::pin!.

Now that the MSRV is 1.75, replace Box::pin with core::pin::pin! so the
futures live on the stack. Pin<&mut F> is Unpin, so the Joiner bounds
still hold. To pin every future before the Joiner borrows it, build all
five uniformly at the end of the iteration: run the synchronous timer
checks and side effects first, capture their outcomes in flags, and
gate each future's work behind those flags. Futures whose work is
skipped resolve to Ok(()) immediately, preserving the previous behavior
where an unset Joiner slot was treated as Ready(Ok(())).

The eager single poll of the ChannelManager future and the early
loop-exit (break) semantics on the scorer/sweeper timers are unchanged.

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.
@ldk-reviews-bot

ldk-reviews-bot commented Jun 20, 2026

Copy link
Copy Markdown

I've assigned @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-claude-review-bot

Copy link
Copy Markdown
Collaborator

No functional issues found.

I reviewed the refactor carefully against the original behavior:

  • Side-effect ordering preserved: the synchronous timer checks (archive, prune + remove_stale_channels_and_tracking_with_time, scorer time_passed, sweeper) all run in the same order before any future is polled. Since futures are lazy, building them after the sync work (rather than interleaved) does not change behavior.
  • Skipped-work semantics preserved: every future is now unconditionally set into the Joiner, but each gates its work behind a flag (needs_cm_persist, network_graph_to_persist, persist_scorer, persist_sweeper, LM presence) and resolves to Ok(()) when skipped — matching the old "unset slot → Ready(Ok(()))" behavior.
  • Eager CM poll preserved: cm_fut is still polled once with the dummy waker; Ready is stashed via set_a_res, Pending moves the future into the Joiner via set_a. No double-flush, no double-encode (encode()/flush() remain gated behind needs_cm_persist inside the async block).
  • break semantics preserved: scorer/sweeper Some(true) => break still exit before the join, and None no longer persists (equivalent to the old None => {} leaving the slot unset).
  • Drop order: pending_monitor_writes is still captured after get_and_clear_needs_persistence() and declared before the futures, so the borrow/drop-order invariant holds.
  • Pin<&mut F> is Unpin, so the Joiner bounds are satisfied; type inference for the conditionally-set a slot is still resolved from the set_a(cm_fut) call.

Cross-cutting note (not blocking): the scorer_fut binding (around lib.rs:1294) is split across lines in a way that differs from the sibling let network_graph_fut = core::pin::pin!(...) style; running cargo fmt would likely collapse it, so double-check CI's rustfmt check passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants