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

Driver: Split receive into its own feature #141

Merged
merged 2 commits into from
Aug 1, 2022

Conversation

FelixMcFelix
Copy link
Member

Adds the "receive" feature, which is disabled by default. When this is disabled, the UDP receive task is not compiled and not run, and as an optimisation the UDP receive buffer size is set to 0. All related events are also removed.

Closes #131.

Adds the "receive" feature, which is disabled by default. When this is disabled, the UDP receive task is not compiled and not run, and as an optimisation the UDP receive buffer size is set to 0. All related events are also removed.

Closes serenity-rs#131.
@GnomedDev
Copy link
Member

I know it would probably be a bit of a hassle, but performance could be further improved by getting rid of the udp_tx task entirely when receive is disabled, eg: GnomedDev@d4b5d87

@FelixMcFelix
Copy link
Member Author

I originally had a slight ideological issue with "putting a UDP send in sync code", but thinking on the packet cycle structure this is probably fine. If you step into the cycle a bit, you get send -> mix -> wait for (20 - x) ms -> .... Blocking on one send needs to overrun around 19.7ms to actually hurt the next packet, which shouldn't happen unless you're sending more traffic than the OS can serve (and the next frame will try to catch up).

The current UDP Tx design has some benefits (never blocks sender, unbounded), but costs a Vec alloc and ~300B memcpy every cycle. I put some thought into alternatives without a Vec like a shared buffer with ~1500B slots, but with a single slot you'll block on the same circumstances, and with m slots the sender still needs to keep its own sleep loop to prevent having m-1 20ms frames of output delay (i.e., by filling up the slots to send on). For the extra complexity I'm not sure it's worth it. This is a longwinded way of saying that returning to this UDP send method is probably a valid tradeoff -- low send overhead, can block but low risk, no added delay, no writing sync<->async shared buffers.

The only thing about the patch you link is that it also drops sending UDP keepalives -- I suppose you can get away without them if you know you're running on dedicated infra, but they're generally there to make sure that NAT entries don't vanish during silent periods for folks running { songbird, Discord itself } from a home machine. I could just stick them after packet sends to keep the sleep logic simple.

@FelixMcFelix
Copy link
Member Author

FelixMcFelix commented Jul 31, 2022

Quick test on "receive" suggests that a non-blocking std::net::UdpSocket for Tx should be okay, so try_clone etc. is a valid way to remove the udp_tx task in all cases, I'll tidy up shortly, have integrated keepalives too.

@FelixMcFelix FelixMcFelix merged commit 7560963 into serenity-rs:next Aug 1, 2022
@FelixMcFelix FelixMcFelix added enhancement New feature or request driver Relates to the driver or one of its sub-tasks. events Relates to driver event handling/generation. breaking Will either cause existing code to fail to compile, or cause substantial behaviour changes labels Aug 1, 2022
@FelixMcFelix FelixMcFelix added this to the v0.4.0: Nightingale milestone Aug 1, 2022
@FelixMcFelix FelixMcFelix deleted the voice-rx-feature branch August 8, 2022 13:23
@FelixMcFelix FelixMcFelix mentioned this pull request Aug 9, 2022
3 tasks
FelixMcFelix added a commit to FelixMcFelix/songbird that referenced this pull request Nov 20, 2023
Adds the "receive" feature, which is disabled by default. When this is disabled, the UDP receive task is not compiled and not run, and as an optimisation the UDP receive buffer size is set to 0. All related events are also removed.

This also removes the UDP Tx task, and moves packet and keepalive sends back into the mixer thread. This allows us to entirely remove channels and various allocations between the mixer and an async task created only for sending data (i.e., fewer memcopies).

If "receive" is enabled, UDP sends are now non-blocking due to technical constraints -- failure to send is non-fatal, but *will* drop affected packets. Given that blocking on a UDP send indicates that the OS cannot clear send buffers fast enough, this should alleviate OS load.

Closes serenity-rs#131.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Will either cause existing code to fail to compile, or cause substantial behaviour changes driver Relates to the driver or one of its sub-tasks. enhancement New feature or request events Relates to driver event handling/generation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants