-
Notifications
You must be signed in to change notification settings - Fork 171
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
Conversation
@flub I have fixed formatting issues. Could you please help to approve the CI workflow again? Thank you very much! |
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.
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.
iroh-net/src/magicsock.rs
Outdated
let LocalAddresses { | ||
regular: mut ips, | ||
loopback, | ||
} = LocalAddresses::new(); |
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.
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.
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 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.
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.
Thanks for these detailed numbers, it's great to see these written down!
See #2332 for the flaky test this found :( |
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.
clippy is unhappy, otherwise looks good
Co-authored-by: Floris Bruynooghe <[email protected]>
Thank you for your patient guidance. |
Thanks for your contributions! Finding and fixing where those delays are from is great! |
## 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. --------- Co-authored-by: Floris Bruynooghe <[email protected]>
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.