-
-
Notifications
You must be signed in to change notification settings - Fork 416
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
feat: Faster UDP/IO on Apple platforms #1993
Conversation
We found there wasn't much performance benefit, and was considerable difficulty taking advantage of, |
Bench on
Bench with this PR:
Surprised that |
Those tests tend to be extremely noisy, as the huge variance suggests. A targeted quinn-udp benchmark might be more useful. |
We've also found on neqo that multi-packet RX without multi-packet TX has limited benefits, since the RX batch size will be very small. |
I added |
How about using the https://github.com/quinn-rs/quinn/blob/main/quinn-udp/benches/throughput.rs |
With @mxinden's benchmark. Baseline:
Only
Both
Both sendmsg_x and recvmsg_x with
|
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.
LGTM.
Please squash all of the changes into a single commit?
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.
Impressive results. Great to see the MacOS _x
syscalls work for QUIC UDP IO.
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.
Can we enable real GSO/GRO using these interfaces?
No. They are the equivalent of the |
Are you waiting on anything from me on this? |
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.
quinn-udp/benches/throughput.rs
will need more changes to still support non-apple platform. @larseggert I believe we will need to either run it multi-threaded, or use some kind of executor, e.g. tokio
. I can prepare a commit in the next couple of days. Sorry for missing this in earlier reviews.
Changes itself look good to me.
@Ralith can you do another round on this one? |
Once @mxinden's fix to the bench is in, I will rebase and squash this PR. |
Hey guys 👋, is there any chance Apple won't approve apps that are using those private syscalls in the App Store? They are notorious for doing so and even de-listing apps for using anything "undocumented". See 2.5.1 here: https://developer.apple.com/app-store/review/guidelines/ See one of such cases here: https://9to5mac.com/2019/11/04/electron-app-rejections/ How they will find out? Apple employs automated tools to scan apps for the usage of private APIs. If |
The use of the private syscalls is now behind a non-default feature. |
@larseggert should we add a big fat warning saying that if you enable this flag you will violate Apple ToS so it's only should be enabled if app is not distributed via App Store (or notarized for EU)? |
This uses Apple's private sendmsg_x and recvmsg_x system calls for multi-packet UDP I/O.
de8f280
to
d9e8564
Compare
Shouldn't the |
Should be fine I believe:
|
Currently we use `quinn-udp` `v0.5.4`. `quinn-udp` `v0.5.5` fixes [`recvmmsg` calls on Android x86](quinn-rs/quinn#1966). `quinn-udp` `v0.5.6` adds [experimental multi-message support on Apple platforms](quinn-rs/quinn#1993) and [fixes an unnecessary `windows-sys` version restriction](quinn-rs/quinn#2021). While not strictly necessary, given that our current version specification (i.e. `version = "0.5.4"`) already allows users to use Neqo with `quinn-udp` `v0.5.6`, this commit updates to `quinn-udp` `v0.5.6` anyways, thus making sure CI tests with latest version. In case mozilla#2208 lands, future compatible version updates would touch the `Cargo.lock` file, not `Cargo.toml`.
Currently we use `quinn-udp` `v0.5.4`. `quinn-udp` `v0.5.5` fixes [`recvmmsg` calls on Android x86](quinn-rs/quinn#1966). `quinn-udp` `v0.5.6` adds [experimental multi-message support on Apple platforms](quinn-rs/quinn#1993) and [fixes an unnecessary `windows-sys` version restriction](quinn-rs/quinn#2021). While not strictly necessary, given that our current version specification (i.e. `version = "0.5.4"`) already allows users to use Neqo with `quinn-udp` `v0.5.6`, this commit updates to `quinn-udp` `v0.5.6` anyways, thus making sure CI tests with latest version. In case #2208 lands, future compatible version updates would touch the `Cargo.lock` file, not `Cargo.toml`.
Currently we use `quinn-udp` `v0.5.4`. `quinn-udp` `v0.5.5` fixes [`recvmmsg` calls on Android x86](quinn-rs/quinn#1966). `quinn-udp` `v0.5.6` adds [experimental multi-message support on Apple platforms](quinn-rs/quinn#1993) and [fixes an unnecessary `windows-sys` version restriction](quinn-rs/quinn#2021). While not strictly necessary, given that our current version specification (i.e. `version = "0.5.4"`) already allows users to use Neqo with `quinn-udp` `v0.5.6`, this commit updates to `quinn-udp` `v0.5.6` anyways, thus making sure CI tests with latest version. In case #2208 lands, future compatible version updates would touch the `Cargo.lock` file, not `Cargo.toml`.
Currently we use `quinn-udp` `v0.5.4`. `quinn-udp` `v0.5.5` fixes [`recvmmsg` calls on Android x86](quinn-rs/quinn#1966). `quinn-udp` `v0.5.6` adds [experimental multi-message support on Apple platforms](quinn-rs/quinn#1993) and [fixes an unnecessary `windows-sys` version restriction](quinn-rs/quinn#2021). While not strictly necessary, given that our current version specification (i.e. `version = "0.5.4"`) already allows users to use Neqo with `quinn-udp` `v0.5.6`, this commit updates to `quinn-udp` `v0.5.6` anyways, thus making sure CI tests with latest version. In case #2208 lands, future compatible version updates would touch the `Cargo.lock` file, not `Cargo.toml`.
Currently we use `quinn-udp` `v0.5.4`. `quinn-udp` `v0.5.5` fixes [`recvmmsg` calls on Android x86](quinn-rs/quinn#1966). `quinn-udp` `v0.5.6` adds [experimental multi-message support on Apple platforms](quinn-rs/quinn#1993) and [fixes an unnecessary `windows-sys` version restriction](quinn-rs/quinn#2021). While not strictly necessary, given that our current version specification (i.e. `version = "0.5.4"`) already allows users to use Neqo with `quinn-udp` `v0.5.6`, this commit updates to `quinn-udp` `v0.5.6` anyways, thus making sure CI tests with latest version. In case #2208 lands, future compatible version updates would touch the `Cargo.lock` file, not `Cargo.toml`. Co-authored-by: Lars Eggert <[email protected]>
Currently we use `quinn-udp` `v0.5.4`. `quinn-udp` `v0.5.5` fixes [`recvmmsg` calls on Android x86](quinn-rs/quinn#1966). `quinn-udp` `v0.5.6` adds [experimental multi-message support on Apple platforms](quinn-rs/quinn#1993) and [fixes an unnecessary `windows-sys` version restriction](quinn-rs/quinn#2021). While not strictly necessary, given that our current version specification (i.e. `version = "0.5.4"`) already allows users to use Neqo with `quinn-udp` `v0.5.6`, this commit updates to `quinn-udp` `v0.5.6` anyways, thus making sure CI tests with latest version. In case #2208 lands, future compatible version updates would touch the `Cargo.lock` file, not `Cargo.toml`. Co-authored-by: Lars Eggert <[email protected]>
Currently we use `quinn-udp` `v0.5.4`. `quinn-udp` `v0.5.5` fixes [`recvmmsg` calls on Android x86](quinn-rs/quinn#1966). `quinn-udp` `v0.5.6` adds [experimental multi-message support on Apple platforms](quinn-rs/quinn#1993) and [fixes an unnecessary `windows-sys` version restriction](quinn-rs/quinn#2021). While not strictly necessary, given that our current version specification (i.e. `version = "0.5.4"`) already allows users to use Neqo with `quinn-udp` `v0.5.6`, this commit updates to `quinn-udp` `v0.5.6` anyways, thus making sure CI tests with latest version. In case #2208 lands, future compatible version updates would touch the `Cargo.lock` file, not `Cargo.toml`. Co-authored-by: Lars Eggert <[email protected]>
io::ErrorKind::Interrupted => { | ||
// Retry the transmission | ||
} |
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.
Given that this is not executed in a loop
, it doesn't actually retry, but instead drop the packet to be sent.
We do retry on Linux:
Lines 311 to 314 in 2edf192
io::ErrorKind::Interrupted => { | |
// Retry the transmission | |
continue; | |
} |
But not on the BSDs or the slow Apple:
Lines 433 to 435 in 2edf192
io::ErrorKind::Interrupted => { | |
// Retry the transmission | |
} |
I don't recall whether we discussed this. @larseggert was this by design?
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 think this was an oversight. It would be good to have identical behavior.
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.
Agreed. @larseggert I will follow-up with a pull request.
With quinn-rs#2017, the concrete `send` implementations per platform are supposed to propagate `io::Error`s. Those errors are then eiter logged and dropped in `UdpSocketState::send` or further propagated in `UdpSocketState::try_send`. The `fast_apple` `send` implementation added in quinn-rs#1993 does not follow this pattern. This commit adjusts the `fast_apple` implementation accordingly.
With #2017, the concrete `send` implementations per platform are supposed to propagate `io::Error`s. Those errors are then eiter logged and dropped in `UdpSocketState::send` or further propagated in `UdpSocketState::try_send`. The `fast_apple` `send` implementation added in #1993 does not follow this pattern. This commit adjusts the `fast_apple` implementation accordingly.
This uses Apple's private
sendmsg_x
andrecvmsg_x
system calls for multi-packet UDP I/O.CC @mxinden