Functionize additional macro code in channelmanager.rs
#4224
Conversation
|
👋 Thanks for assigning @joostjager as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4224 +/- ##
==========================================
- Coverage 89.34% 89.33% -0.01%
==========================================
Files 180 180
Lines 138341 138469 +128
Branches 138341 138469 +128
==========================================
+ Hits 123596 123706 +110
- Misses 12123 12137 +14
- Partials 2622 2626 +4
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:
|
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Not just a build time improvement, but also a reduction of the friction in code navigation and debugging!
lightning/src/ln/channelmanager.rs
Outdated
| }} | ||
| } | ||
|
|
||
| fn convert_channel_err_internal<Close: FnOnce(ClosureReason, &str) -> (ShutdownResult, Option<(msgs::ChannelUpdate, NodeId, NodeId)>)>( |
There was a problem hiding this comment.
Comments explaining this function and the one below would be welcome.
There was a problem hiding this comment.
What do you feel like is missing clarity?
There was a problem hiding this comment.
More details about what the function is doing. Of course if you know what it does, you can see that reflected in the code. But if you are new to the code, a bit of explanation is helpful. Why it is needed, what it does, how to use it.
There was a problem hiding this comment.
I'm really not at all clear what I could possibly add with a comment here - the method takes a ChannelError and returns a MsgHandleErrInternal, doing a conversion, like the name says. The return value is maybe non-obvious, but its also an _internal method that exists to do the thing convert_channel_err!() says it is doing. I could document it with just a doc comment saying "does the work for convert_channel_err!(), but I'm not convinced that's actually improving readability :)
lightning/src/ln/channelmanager.rs
Outdated
| (false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Abort(reason), chan_id)) | ||
| }, | ||
| ChannelError::Close((msg, reason)) => { | ||
| let (shutdown_res, chan_update) = close(reason, &msg); |
There was a problem hiding this comment.
Can't this be a function without a side effect, just call the close logic one level up?
There was a problem hiding this comment.
Not immediately obvious to me how to do this cleanly - we need the shutdown_res to build the MsgHandleErrInternal which we want to do here.
There was a problem hiding this comment.
Ok. Just asking, but I see the point.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
TheBlueMatt
force-pushed
the
2025-11-functionize-chan-close
branch
from
5ca95be to
8f96af9
Compare
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Macro deletion, it's good :)
lightning/src/ln/channelmanager.rs
Outdated
| (false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Abort(reason), chan_id)) | ||
| }, | ||
| ChannelError::Close((msg, reason)) => { | ||
| let (shutdown_res, chan_update) = close(reason, &msg); |
There was a problem hiding this comment.
Ok. Just asking, but I see the point.
The `handle_monitor_update_completion` macro is called from a number of places in `channelmanager.rs` and dumps quite a bit of code into each callsite. Here we take the unlocked half of `handle_monitor_update_completion` and move it into a function in `ChannelManager`. As a result, building the `lightning` crate tests after a single-line `println` change in `channelmanager.rs` was reduced by around a full second (out of ~28.5 originally) on my machine using rustc 1.85.0. Memory usage of the `expand_crate` step went from +554MB to +548MB in the same test.
In the next commit we'll functionize some of the `ChannelError`-handling logic, requiring it have access to `handle_new_monitor_update_locked_actions_handled_by_caller!` which thus needs to be defined above it.
`convert_channel_err` is used extensively in `channelmanager.rs` (often indirectly via `try_channel_entry`) and generates nontrivial code (especially once you include `locked_close_channel`). Here we take the `FUNDED` cases of it and move them to an internal function reducing the generated code size. On the same machine as described two commits ago, this further reduces build times by about another second.
`convert_channel_err` is used extensively in `channelmanager.rs` (often indirectly via `try_channel_entry`) and generates nontrivial code (especially once you include `locked_close_channel`). Here we take the `UNFUNDED` case of it and move them to an internal function reducing the generated code size. On the same machine as described three commits ago, this further reduces build times from the previous commit by about another second.
Now that `convert_channel_err` is primarily in two functions, `locked_close_channel` is just a dumb macro that has two forms, each only called once. Instead, drop it and inline its contents into `convert_*_channel_err`.
TheBlueMatt
force-pushed
the
2025-11-functionize-chan-close
branch
from
8f96af9 to
7cea2e9
Compare
|
Squashed and rebased, fixing the commit message. |
elnosh
left a comment
elnosh
left a comment
There was a problem hiding this comment.
was reviewing before the rebase but LGTM
4 participants
On my machine this decreases the build step of trivial-change
lightning-crate test re-builds by around 10%