Skip to content

feat(iroh): Make poll_writable precise by using NodeMap::addr_for_send #3266

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

matheus23
Copy link
Member

For now just a PR to look at netsim numbers.

Description

Every send side of a connection gets its own UdpPoller.

If we pass in the Connection::remote_address() to AsyncUdpSocket::create_io_poller in quinn (n0-computer/quinn#57) then we can determine which path we're going to use in try_send via NodeMap::addr_for_send within IoPoller::poll_writable.

This allows us to accurately (ignoring race conditions between try_send and poll_writable) report readiness.

Notes & open questions

Needs changes to quinn's AsyncUdpSocket trait.
It's possible to circumvent them, but it's verrrry annoying to do so.
(The trick would be to wrap calls to quinn::Endpoint::connect_with and quinn::Incoming::accept[_with] with a lock and pre-load the remote address somewhere so that MagicSock::create_io_poller can fetch it back when it's called through connect_with/accept[_with]. I couldn't get that to work easily without panicing 😬 )

Change checklist

  • Self-review.
  • Tests if relevant.

@matheus23 matheus23 self-assigned this Apr 15, 2025
Copy link

github-actions bot commented Apr 15, 2025

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3266/docs/iroh/

Last updated: 2025-05-20T15:12:46Z

Copy link

github-actions bot commented Apr 15, 2025

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 1150c5d

@matheus23
Copy link
Member Author

This PR:

test case throughput_gbps throughput_transfer
iroh 1_to_1 1.38 1.38
iroh 1_to_3 3.24 3.25
iroh 1_to_5 4.98 4.98
iroh 1_to_10 6.52 6.53
iroh 2_to_2 2.71 2.72
iroh 2_to_4 3.93 3.93
iroh 2_to_6 5.68 5.69
iroh 2_to_10 7.92 7.93

Latest PR to main:

test case throughput_gbps throughput_transfer
iroh 1_to_1 1.37 1.37
iroh 1_to_3 4.06 4.07
iroh 1_to_5 6.93 6.94
iroh 1_to_10 12.14 12.16
iroh 2_to_2 3.06 3.07
iroh 2_to_4 5.83 5.84
iroh 2_to_6 9.26 9.27
iroh 2_to_10 13.74 13.76

@matheus23
Copy link
Member Author

I guess it clearly makes a difference? 😅

@n0bot n0bot bot added this to iroh Apr 15, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Apr 15, 2025
@matheus23 matheus23 marked this pull request as draft April 15, 2025 10:11
@matheus23
Copy link
Member Author

Don't think I'll merge this near-term.
Needs more though/figuring out why performance is so much worse.
And I don't want to pollute the pull requests tab, so closing.

@matheus23 matheus23 closed this Apr 29, 2025
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in iroh Apr 29, 2025
@matheus23 matheus23 reopened this May 20, 2025
@matheus23 matheus23 force-pushed the matheus23/poll-writable-improvements branch from 1c22b91 to 19dfdb5 Compare May 20, 2025 14:38
@matheus23
Copy link
Member Author

This PR after rebasing on main:

test case throughput_gbps throughput_transfer
iroh 1_to_1 1.41 2.79
iroh 1_to_3 4.50 9.50
iroh 1_to_5 7.15 14.34
iroh 1_to_10 11.85 20.35
iroh 2_to_2 2.78 5.48
iroh 2_to_4 5.49 10.68
iroh 2_to_6 9.01 19.23
iroh 2_to_10 13.77 26.81

main:

test case throughput_gbps throughput_transfer
iroh 1_to_1 1.45 2.95
iroh 1_to_3 4.58 9.87
iroh 1_to_5 6.59 12.30
iroh 1_to_10 12.27 21.67
iroh 2_to_2 2.59 4.78
iroh 2_to_4 5.89 12.19
iroh 2_to_6 8.43 16.79
iroh 2_to_10 14.02 27.93

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

1 participant