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

Stop doing thousands of time checks each time we connect to a peer #3106

Merged
merged 8 commits into from
Dec 3, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Nov 26, 2021

Motivation

Zebra checks the current time 4 times per address book peer, every time we connect to a new peer. If the address book has thousands of peers, it can do hundreds of thousands of time system calls every second.

This is unexpected work in sprint 23, but we might need it to make CI more reliable.

Scope

This PR only changes how often Zebra checks the time.
Other changes and cleanups are out of scope.

Solution

Time checks:

  • Stop checking the time of every address book peer for each connection attempt
    • Stop sorting all the address book peers for each connection attempt
  • Remove redundant peer time checks within the address book
  • Stop checking the time 3 times for each address book update
  • Stop checking the time multiple times in each address book method

Bug fixes:

  • Use saturating time operations to avoid panics

This seems like a bug in Rust or the OS, we really shouldn't be getting earlier times when we remove some time calls. It might also be a multithreading memory consistency issue.

Review

This PR is a blocker for #1873, so we should review it soon.

Reviewer Checklist

  • Code implements Specs and Designs
  • Existing tests pass

Follow Up Work

This PR makes it more obvious when we're using std::time::Instant instead of tokio::time::Instant. We should fix that in ticket #2258.

@teor2345 teor2345 added C-bug Category: This is a bug P-Low I-slow Problems with performance or responsiveness I-integration-fail Continuous integration fails, including build and test failures A-network Area: Network protocol updates or fixes labels Nov 26, 2021
@teor2345 teor2345 added this to the 2021 Sprint 23 milestone Nov 26, 2021
@teor2345 teor2345 self-assigned this Nov 26, 2021
@teor2345 teor2345 requested review from conradoplg and jvff and removed request for conradoplg November 30, 2021 03:41
@mpguerra mpguerra removed this from the 2021 Sprint 23 milestone Nov 30, 2021
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks good, but there seems to be an issue with the usage of BTreeMap

zebra-network/src/address_book.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 added P-Medium and removed P-Low labels Nov 30, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Nov 30, 2021

Bumping this up to medium, because it's required for #1873 in this sprint.

@teor2345 teor2345 marked this pull request as draft November 30, 2021 20:54
@teor2345 teor2345 added the A-dependencies Area: Dependency file updates label Dec 2, 2021
@teor2345 teor2345 force-pushed the addr-book-time-perf branch from c58e02e to 99fd439 Compare December 2, 2021 02:39
@teor2345 teor2345 force-pushed the addr-book-time-perf branch from 99fd439 to 64e5d1c Compare December 2, 2021 02:45
@teor2345 teor2345 marked this pull request as ready for review December 2, 2021 02:46
@teor2345 teor2345 requested a review from conradoplg December 2, 2021 03:17
@teor2345 teor2345 enabled auto-merge (squash) December 2, 2021 04:59
@teor2345
Copy link
Contributor Author

teor2345 commented Dec 2, 2021

Looks good, but there seems to be an issue with the usage of BTreeMap

I went with OrderedMap, and added some peer order tests.

Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks good!

@teor2345 teor2345 merged commit 4d608d3 into main Dec 3, 2021
@teor2345 teor2345 deleted the addr-book-time-perf branch December 3, 2021 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-network Area: Network protocol updates or fixes C-bug Category: This is a bug I-integration-fail Continuous integration fails, including build and test failures I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants