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

fix(iroh-net): fix extra delay #2330

Merged
merged 7 commits into from
May 29, 2024
Merged

Conversation

zh522130
Copy link
Contributor

@zh522130 zh522130 commented May 28, 2024

Description

Fix for #2328: Enumerating network devices is slow
on windows and causes delays in the magicsock actor. This makes sure that this operation
does not block the remainder of the actor.

Also bumps netdev to v0.27.0 , to fix #2327.

@zh522130
Copy link
Contributor Author

@flub I have fixed formatting issues. Could you please help to approve the CI workflow again? Thank you very much!

Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

This is a fairly reasonable way to go about this I guess. The structure of the magicsocket is not great here, but this is just living with that and improving it is a whole other project.

let LocalAddresses {
regular: mut ips,
loopback,
} = LocalAddresses::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

This can still block the executor this is being spawned on. Could you wrap the new() call into tokio::task::spawn_blocking()? And also leave a comment that on windows this introduces a 10-20ms of delay so that we know why.

Copy link
Contributor Author

@zh522130 zh522130 May 29, 2024

Choose a reason for hiding this comment

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

I have made the modifications as per your suggestion and conducted some tests on the execution time of the latest version of the netdev::get_interfaces() function. conducting 30 tests for each scenario. The results are as follows:

windows(Lan + wifi off):min: 9ms, max: 28ms
windows(Lan + wifi connects to mobile hotspot ):min: 24ms, max: 1174ms

macos (Hackintosh X86): min: 1ms, max: 6ms

linux (7840hs + lan): min: 100us, max: 502us
linux (7840hs + 1lan connect + 3lan disconnect): min: 100us, max: 1092us
linux (2core cpu server + lan): min: 100us, max: 960us

The time taken for the function call may be influenced by the platform, the number of network interfaces, and CPU performance (such as on embedded devices or server). Therefore, in the comments, I did not limit my annotations to just Windows. The test results show that the Windows platform indeed takes the most time. Analyzing the source code of netdev for the Windows platform confirms that the outcome aligns with expectations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for these detailed numbers, it's great to see these written down!

@flub
Copy link
Contributor

flub commented May 28, 2024

See #2332 for the flaky test this found :(

@zh522130 zh522130 requested a review from flub May 29, 2024 02:19
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

clippy is unhappy, otherwise looks good

iroh-net/src/magicsock.rs Outdated Show resolved Hide resolved
zh522130 and others added 2 commits May 29, 2024 15:20
Co-authored-by: Floris Bruynooghe <flub@devork.be>
@zh522130
Copy link
Contributor Author

clippy is unhappy, otherwise looks good

Thank you for your patient guidance.

@zh522130 zh522130 requested a review from flub May 29, 2024 07:36
@flub flub added this pull request to the merge queue May 29, 2024
@flub
Copy link
Contributor

flub commented May 29, 2024

Thank you for your patient guidance.

Thanks for your contributions! Finding and fixing where those delays are from is great!

Merged via the queue into n0-computer:main with commit 77f92ef May 29, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

More delays when the network interface is down on Windows .
2 participants