-
Notifications
You must be signed in to change notification settings - Fork 407
Add a simple send-funds benchmark in channelmanager #860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a simple send-funds benchmark in channelmanager #860
Conversation
While its not necessarily a common operation on a running node, `get_our_node_id()` is used incredibly heavily in tests, and there is no reason to not eat the extra ~64 bytes to just cache it.
Codecov Report
@@ Coverage Diff @@
## main #860 +/- ##
==========================================
+ Coverage 90.60% 90.62% +0.02%
==========================================
Files 51 51
Lines 27193 27195 +2
==========================================
+ Hits 24637 24646 +9
+ Misses 2556 2549 -7
Continue to review full report at Codecov.
|
Our naive persister...not so much, its currently showing around 100ms for a full back-and-forth send, at least after some time as the |
Note that if you swap to flushing the |
If you go even further and cut out the duplicate fsync and file rename for ChannelMonitorUpdate writes we get down to 19ms-per-two-sends, which is much more reasonable. Branch is at https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=shortlog;h=refs/heads/2021-03-optimize-chanmon-persist if anyone wants to go implement the reading logic for it so we can take it https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=shortlog;h=refs/heads/2021-03-optimize-chanmon-persist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it might make sense to also make a slightly more complicated benchmark with one routing node in between?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve mod CI warning :)
use test::Bencher; | ||
|
||
struct NodeHolder<'a, P: Persist<InMemorySigner>> { | ||
node: &'a ChannelManager<InMemorySigner, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be curious to compare this to an Arc
'd version (i.e. using arcs instead of refs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be astounded if it made a difference - we aren't actually cloning the Arc
s anywhere that I know of here and our runtime is dominated by cryptographic operations. Non-Arc objects are nice mostly because we'd like to eventually support non-threaded applications, so we don't need trait implementations to implement Sync+Send
.
1424b28
to
8a9f0b8
Compare
Only difference from previous ACKs is fixed the compile warning:
|
Hmm, there should be almost no difference there, forwarding vs initial send should be completely dwarfed by cryptographic operations. If you don't mind, I'd prefer to just take this as-is and we can follow up with more benchmarking if we think its merited later. |
I saw https://twitter.com/bottlepay/status/1376918214249218049 and was curious, so I benchmarked sends.
We're doing pretty good - on my local machine somewhere in the neighborhood of 5-6ms for two full back-and-forth sends, ignoring all IO latency.