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

refactor(networking): wait for all futures together in connectToNodes #1471

Merged
merged 2 commits into from
Jan 9, 2023

Conversation

alrevuelta
Copy link
Contributor

  • Instead of awaiting on each dialPeer individually, this pr triggers all dials, stores the futures in an array and then waits for the futures to be completed. This substantially decreases the time it takes for a nwaku node to reach a reasonable number of connections.
  • In several runs, attempting to connect 12 peers with connectToNodes I could verify that the time it tool to connect to all of them decreased from 43/44 seconds to 4/5 seconds. Note that it was tested against our fleets

@status-im-auto
Copy link
Collaborator

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
44a8d88 #1 2022-12-16 23:07:51 ~20 min macos 📄log

Copy link
Contributor

@LNSD LNSD left a comment

Choose a reason for hiding this comment

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

LGTM ✅

waku/v2/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks - small change with big effect. Agree with stylistic consistency of await allFutures(...)

@alrevuelta alrevuelta merged commit 4eb9375 into master Jan 9, 2023
@alrevuelta alrevuelta deleted the connect-await-together branch January 9, 2023 20:45
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.

4 participants