feat: Implement RelayDatagramsQueue

[matheus23]

Based on #2986

Description

Replaces RelayRecvReceiver and RelayRecvSender with the (clonable & shared) RelayDatagramsQueue.
This queue contains a ConcurrentQueue (from the smol library concurrent-queue) and an AtomicWaker.
It should only be polled from one task.
If polled from multiple tasks, then tasks will overwrite each other's wakers.

Unfortunately we can't make it use &mut Self in poll_recv because quinn expects the AsyncUdpSockets poll_recv interface to be &self.

This (un)fortunately doesn't have an effect on performance for me.

(The benchmark is completely broken half the time for some reason, but when it runs it produces normal numbers:)

$ DEV_RELAY_ONLY=true cargo run -p iroh-net-bench --release --features=local-relay -- iroh --with-relay --download-size=100M
      │  Throughput   │ Duration 
──────┼───────────────┼──────────
 AVG  │   55.05 MiB/s │     1.82s
 P0   │   55.03 MiB/s │     1.82s
 P10  │   55.06 MiB/s │     1.82s
 P50  │   55.06 MiB/s │     1.82s
 P90  │   55.06 MiB/s │     1.82s
 P100 │   55.06 MiB/s │     1.82s

And basically exactly the same times for the PR this is based on.

Breaking Changes

Notes & open questions

Todo

• Rename variables to e.g. relay_datagrams_queue instead of relay_recv_sender or relay_recv_channel etc.
• Add documentation about multiple tasks polling, etc.

Change checklist

• Self-review.
• Documentation updates following the style guide, if relevant.
• Tests if relevant.
• All breaking changes documented.

[github-actions]

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

Last updated: 2024-12-04T14:15:20Z

[github-actions]

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: b8d633f

[flub]

FWIW the Flag examples in AtomicWaker don't bother with removing this waker if it is no longer needed. I really don't know anymore how much this matters. It depends on what suprious wakeups might happen or how concurrently this is called I guess.

This should be reasonably not-concurrently called IIUC, but then I don't understand why I had things not waking up in the broken version.

[matheus23]

Well, yeah, not removing the waker here means you're generating another spurious wakeup AFAIU. Not terrible, but also not great.

[flub]

Will it result in spurious wakeups though? You're yielding an item instead of pending. So the task does not suspend and the runtime might(?)/should(?) now drop this waker and create a new one for the next poll?

[flub]

Of course custom poll functions could mean some other poll decides to still return pending for other reasons. In that case you'll get spurious wakeups I guess.

[matheus23]

Yes it's exactly that case you describe. If another poll fn decides to return pending for other reasons, then there's no need to wake up the future if there's another item in the queue. The only reason to wake up the future is for the other poll fn's reasons.

[flub]

I'm a bit surprised that you joined the sender & receiver back into a single object, after complaining on my PR that they were like this. But... I don't mind.

Good naming though!

[matheus23]

😂 Sorry about that.

It's not that I want the sender & receiver to be separate necessarily.
It's just that sticking both a tokio::mpsc::Sender and tokio::mpsc::Receiver into the same object reads weird to me.

[flub]

Good naming though!

I take this part back. This is only the RelayRecvDatagramsQueue. We'll probably wan a send version too. No big deal though.

[matheus23]

Good naming though!

I take this part back. This is only the RelayRecvDatagramsQueue. We'll probably wan a send version too. No big deal though.

What if we re-use the same implementation for the send side 👀 (and only rename the variable names to relay_recv_queue, not the struct name)?