Skip to content
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

feat: latency test for restore #3956

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

maan2003
Copy link
Member

@maan2003 maan2003 commented Dec 15, 2023

restore has become really slow, we need to track these.

@maan2003 maan2003 requested a review from a team as a code owner December 15, 2023 17:35
);
// FIXME: should be smaller
assert!(reissue_stats.median < Duration::from_secs(4));
assert!(ln_sends_stats.median < Duration::from_secs(6));
assert!(ln_receives_stats.median < Duration::from_secs(6));
assert!(restore_time < Duration::from_secs(160));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was 83 on my machine, I multiplied with factor of 2 to account for CI slow down.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may or may not work as you could have been at an arbitrary point inside the current session. If you start restoring at the end of a session it will take shorter, if you start at the beginning it will take longer. #3956 (comment)

But let's just see how it does on master and narrow down the allowed time as we optimize restoring.

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (251b16e) 56.97% compared to head (a92c47b) 56.95%.

Files Patch % Lines
devimint/src/main.rs 0.00% 21 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3956      +/-   ##
==========================================
- Coverage   56.97%   56.95%   -0.03%     
==========================================
  Files         193      193              
  Lines       43180    43200      +20     
==========================================
+ Hits        24602    24604       +2     
- Misses      18578    18596      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

devimint/src/main.rs Outdated Show resolved Hide resolved
@dpc
Copy link
Contributor

dpc commented Dec 15, 2023

@maan2003 Are you on latest master?

@dpc
Copy link
Contributor

dpc commented Dec 15, 2023

During Alepbft refactor the prefetching of epochs was removed "for simplicity" and at the time I was suspecting it will lead to poor performance, so that would be my first bet.

https://github.com/fedimint/fedimint/pull/3227/files#r1368997856

@maan2003
Copy link
Member Author

@maan2003 Are you on latest master?

I was testing on 0.2 branch. but now rebased to master.

@elsirion
Copy link
Contributor

There's also a random, constant slowdown: we wait for the session we started recovery in to end so we get all transactions we may have missed (since currently we don't have an API for fetching transactions from a running epoch)

@elsirion
Copy link
Contributor

@maan2003 the restore invocation uses named arguments, that's why CI is failing.

elsirion
elsirion previously approved these changes Dec 18, 2023
);
// FIXME: should be smaller
assert!(reissue_stats.median < Duration::from_secs(4));
assert!(ln_sends_stats.median < Duration::from_secs(6));
assert!(ln_receives_stats.median < Duration::from_secs(6));
assert!(restore_time < Duration::from_secs(160));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may or may not work as you could have been at an arbitrary point inside the current session. If you start restoring at the end of a session it will take shorter, if you start at the beginning it will take longer. #3956 (comment)

But let's just see how it does on master and narrow down the allowed time as we optimize restoring.

@elsirion
Copy link
Contributor

Needs rebase :(

@okjodom okjodom added this pull request to the merge queue Dec 20, 2023
Merged via the queue into fedimint:master with commit b013367 Dec 20, 2023
20 checks passed
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.

None yet

5 participants