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

perf: don't allocate in UDP send & recv path #2093

Draft
wants to merge 89 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
89 commits
Select commit Hold shift + click to select a range
5d847ee
perf: don't allocate in UDP send & recv path
mxinden Sep 8, 2024
b334e84
Merge Encoder impl blocks
mxinden Sep 9, 2024
2db53a2
Fix tests
mxinden Sep 10, 2024
9fef795
clippy
mxinden Sep 10, 2024
995c499
fix some, ignore some
mxinden Sep 14, 2024
1c653de
Always run bench
mxinden Sep 14, 2024
c05bc64
First process_input then process_http3
mxinden Sep 14, 2024
08eba9d
Revert "fix some, ignore some"
mxinden Sep 14, 2024
ae112c8
Remove process_multiple_input
mxinden Sep 14, 2024
828da75
Cleanup classic process fn delegating to process_x_2
mxinden Sep 14, 2024
dfa33b2
Consolidate process functions
mxinden Sep 14, 2024
763b391
Rename process to process_alloc
mxinden Sep 14, 2024
8dee7b3
Rename process_into to process
mxinden Sep 14, 2024
5576875
New TODO
mxinden Sep 14, 2024
b9457bb
Copy only for Datagram &[u8]
mxinden Sep 14, 2024
94d1a68
Fix more tests
mxinden Sep 14, 2024
e2d1452
remove all public process_output
mxinden Sep 14, 2024
2e2a76e
Merge branch 'main' of https://github.com/mozilla/neqo into send-recv…
mxinden Sep 14, 2024
1947d33
Remove process_2
mxinden Sep 14, 2024
8ea56b2
Intra doc links
mxinden Sep 14, 2024
3df6660
Thread local receive buffer
mxinden Sep 15, 2024
52dfa91
Cleanup UdpSocket::recv_inner
mxinden Sep 15, 2024
8699209
Revert "Thread local receive buffer"
mxinden Sep 15, 2024
1b9259c
Fix fuzzing
mxinden Sep 15, 2024
07c2b3b
Reduce diff
mxinden Sep 15, 2024
14a9643
Runner::new
mxinden Sep 15, 2024
f0855e1
Cleanup server
mxinden Sep 15, 2024
936ea2b
Cleanup datagram.rs
mxinden Sep 15, 2024
19a82cd
Rename new_with_buffer to new
mxinden Sep 15, 2024
55adc20
simplify codec.rs
mxinden Sep 15, 2024
1cee426
Remove encode_into
mxinden Sep 15, 2024
e9dd74d
Document segment_size
mxinden Sep 15, 2024
99a323e
Cleanup datagram.rs
mxinden Sep 15, 2024
147df66
Address separate write buffer TODO
mxinden Sep 15, 2024
3928c62
Fix fuzz
mxinden Sep 15, 2024
34d904e
Fix fuzzing
mxinden Sep 15, 2024
7f6ca94
Address TODO
mxinden Sep 15, 2024
bd325fe
Test previous client behaviour
mxinden Sep 15, 2024
28f9b0a
Minor changes
mxinden Sep 15, 2024
7cae54f
Remove outdated TODO
mxinden Sep 15, 2024
b03f8d4
build_vn test
mxinden Sep 15, 2024
2003c84
Rename process to process_into_buffer
mxinden Sep 19, 2024
38179ef
Rename process_alloc to process
mxinden Sep 19, 2024
fbccdf2
Merge branch 'main' of https://github.com/mozilla/neqo into send-recv…
mxinden Sep 19, 2024
24e0cbd
Fix docs
mxinden Sep 19, 2024
6deaef8
Rename write_buffer to out
mxinden Sep 19, 2024
5244fc1
Re-introduce process_output
mxinden Sep 19, 2024
15eb2f8
Address minor TODOs
mxinden Sep 25, 2024
ffc3708
Encode update frame directly
mxinden Sep 25, 2024
e5bc0e2
Document panic
mxinden Sep 25, 2024
eb09a9a
Remove outdated clippy allow
mxinden Sep 25, 2024
5ab4e01
allow too_many_arguments in build_packet_header
mxinden Sep 25, 2024
08c49e6
Fix build_insufficient_space
mxinden Sep 25, 2024
2b0103a
Fix build_two
mxinden Sep 25, 2024
c254319
Merge branch 'main' of https://github.com/mozilla/neqo into send-recv…
mxinden Sep 25, 2024
1bd20da
Make PacketBuilder limit an Option<usize>
mxinden Sep 27, 2024
8137e73
No unsafe in recv_inner
mxinden Sep 27, 2024
166ae86
Minor TODOs
mxinden Sep 27, 2024
93c1fa5
Update NLL borrow-issue comment
mxinden Sep 27, 2024
3a134d6
Simplify server.rs
mxinden Sep 27, 2024
61565b4
Polonius workflow
mxinden Sep 27, 2024
f65dde5
Remove duplicate runs-on
mxinden Sep 27, 2024
c6f58dc
newline
mxinden Sep 27, 2024
55e650f
Duplicate only
mxinden Sep 27, 2024
92c48de
Fix diff
mxinden Sep 27, 2024
a4d1ef2
test(udp): fix recv_buf initialization
mxinden Sep 27, 2024
75c1de2
Clippy
mxinden Sep 27, 2024
1ce5455
Update diff
mxinden Sep 27, 2024
91fbc6b
Use git
mxinden Sep 30, 2024
f675eb9
Reduce diff
mxinden Sep 30, 2024
d0916a8
clippy
mxinden Sep 30, 2024
bf807b8
Add Datagram segment unit tests
mxinden Sep 30, 2024
760aade
Remove Http3Client::process_input
mxinden Sep 30, 2024
589b8b4
Reduce diff
mxinden Sep 30, 2024
30593e3
Improve docs
mxinden Sep 30, 2024
5ad1f35
Copy segment size when saving datagram
mxinden Sep 30, 2024
136a8c4
Line break comment
mxinden Sep 30, 2024
b070374
Use Datagram::num_segments
mxinden Sep 30, 2024
55219b8
Revert "Remove Http3Client::process_input"
mxinden Sep 30, 2024
e247d47
Mark process_input for testing only
mxinden Sep 30, 2024
6b30ee5
Fix GRO test
mxinden Sep 30, 2024
f57d1b2
Remove reference to process_input
mxinden Sep 30, 2024
f866a2a
Merge branch 'main' of https://github.com/mozilla/neqo into send-recv…
mxinden Oct 2, 2024
24ba2d4
%s/_inx/_index
mxinden Oct 3, 2024
fe6ed10
Return error and do debug_assert on non-empty send buf
mxinden Oct 4, 2024
6aa438d
%s/with/in
mxinden Oct 4, 2024
364f5bd
Remove impl Copy for Datagram
mxinden Oct 4, 2024
f5b2d7f
Introduce BorrowedDatagram
mxinden Oct 5, 2024
bbb93cc
Encoder encode into &mut [u8] instead of &mut Vec<u8>
mxinden Oct 6, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions .github/workflows/polonius.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
diff --git a/neqo-http3/src/server.rs b/neqo-http3/src/server.rs
index 9b73f2c7..d6cd39a2 100644
--- a/neqo-http3/src/server.rs
+++ b/neqo-http3/src/server.rs
@@ -120,12 +120,7 @@ impl Http3Server {
out: &'a mut Vec<u8>,
) -> Output<&'a [u8]> {
qtrace!([self], "Process.");
- let mut output = self.server.process_into_buffer(
- dgram,
- now,
- // See .github/workflows/polonius.yml.
- unsafe { &mut *std::ptr::from_mut(out) },
- );
+ let mut output = self.server.process_into_buffer(dgram, now, out);

self.process_http3(now);

diff --git a/neqo-transport/src/server.rs b/neqo-transport/src/server.rs
index 2446f8d3..25ceb2b0 100644
--- a/neqo-transport/src/server.rs
+++ b/neqo-transport/src/server.rs
@@ -445,12 +445,7 @@ impl Server {
let mut callback = None;

for connection in &mut self.connections {
- match connection.borrow_mut().process_into_buffer(
- None,
- now,
- // See .github/workflows/polonius.yml.
- unsafe { &mut *std::ptr::from_mut(out) },
- ) {
+ match connection.borrow_mut().process_into_buffer(None, now, out) {
Output::None => {}
d @ Output::Datagram(_) => return d,
Output::Callback(next) => match callback {
@@ -491,12 +486,7 @@ impl Server {

#[allow(clippy::option_if_let_else)]
let output = if let Some(dgram) = dgram {
- self.process_input(
- dgram,
- now,
- // See .github/workflows/polonius.yml.
- unsafe { &mut *std::ptr::from_mut(out) },
- )
+ self.process_input(dgram, now, out)
} else {
Output::None
};
61 changes: 61 additions & 0 deletions .github/workflows/polonius.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# Rustc by default uses the NLL borrow-checker. There are multiple cases where
# NLL is too restrictive. An upcoming improvement to Rust's borrow-checker,
# adressing these shortcomings, is Polonius. It ships with Nightly behind a
# flag.
#
# This workflow first removes the workarounds necessary to please NLL and then
# runs with Polonius to ensure each workaround only fixes the false-positive of
# NLL and doesn't mask an actually undefined behavior.
Comment on lines +6 to +8
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat unconventional. Still, I think statically proving unsafe code to be safe is worth it. As always, open for alternative suggestions.

#
# See also:
# - <https://blog.rust-lang.org/inside-rust/2023/10/06/polonius-update.html>
# - <https://github.com/rust-lang/rust/issues/54663>

name: Polonius
on:
push:
branches: ["main"]
paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"]
pull_request:
branches: ["main"]
paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"]
merge_group:
workflow_dispatch:
env:
CARGO_TERM_COLOR: always
RUST_BACKTRACE: 1

concurrency:
group: ${{ github.workflow }}-${{ github.ref_name }}
cancel-in-progress: true

permissions:
contents: read

jobs:
polonius:
strategy:
fail-fast: false
defaults:
run:
shell: bash
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
- uses: ./.github/actions/rust
with:
version: nightly
token: ${{ secrets.GITHUB_TOKEN }}

- id: nss-version
run: echo "minimum=$(cat neqo-crypto/min_version.txt)" >> "$GITHUB_OUTPUT"

- uses: ./.github/actions/nss
with:
minimum-version: ${{ steps.nss-version.outputs.minimum }}

- name: Apply patch, removing `unsafe` workarounds.
run: git apply .github/workflows/polonius.diff
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really the best place to keep this diff. Put it in the tree.


- run: RUSTFLAGS="-Z polonius" cargo +nightly check
8 changes: 5 additions & 3 deletions fuzz/fuzz_targets/client_initial.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ fuzz_target!(|data: &[u8]| {
let (aead, hp) = initial_aead_and_hp(d_cid, Role::Client);
let (_, pn) = remove_header_protection(&hp, header, payload);

let mut payload_enc = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE);
let mut out = Vec::with_capacity(MIN_INITIAL_PACKET_SIZE);
let mut payload_enc = Encoder::new(&mut out);
payload_enc.encode(data); // Add fuzzed data.

// Make a new header with a 1 byte packet number length.
let mut header_enc = Encoder::new();
let mut out = Vec::new();
let mut header_enc = Encoder::new(&mut out);
header_enc
.encode_byte(0xc0) // Initial with 1 byte packet number.
.encode_uint(4, Version::default().wire_version())
Expand Down Expand Up @@ -57,7 +59,7 @@ fuzz_target!(|data: &[u8]| {
&mut ciphertext,
(header_enc.len() - 1)..header_enc.len(),
);
let fuzzed_ci = Datagram::new(ci.source(), ci.destination(), ci.tos(), ciphertext);
let fuzzed_ci = Datagram::new(ci.source(), ci.destination(), ci.tos(), ciphertext, None);

let mut server = default_server();
let _response = server.process(Some(&fuzzed_ci), now());
Expand Down
8 changes: 5 additions & 3 deletions fuzz/fuzz_targets/server_initial.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@ fuzz_target!(|data: &[u8]| {
let (aead, hp) = initial_aead_and_hp(d_cid, Role::Server);
let (_, pn) = remove_header_protection(&hp, header, payload);

let mut payload_enc = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE);
let mut out = Vec::with_capacity(MIN_INITIAL_PACKET_SIZE);
let mut payload_enc = Encoder::new(&mut out);
payload_enc.encode(data); // Add fuzzed data.

// Make a new header with a 1 byte packet number length.
let mut header_enc = Encoder::new();
let mut out = Vec::new();
let mut header_enc = Encoder::new(&mut out);
header_enc
.encode_byte(0xc0) // Initial with 1 byte packet number.
.encode_uint(4, Version::default().wire_version())
Expand Down Expand Up @@ -63,7 +65,7 @@ fuzz_target!(|data: &[u8]| {
&mut ciphertext,
(header_enc.len() - 1)..header_enc.len(),
);
let fuzzed_si = Datagram::new(si.source(), si.destination(), si.tos(), ciphertext);
let fuzzed_si = Datagram::new(si.source(), si.destination(), si.tos(), ciphertext, None);
let _response = client.process(Some(&fuzzed_si), now());
});

Expand Down
18 changes: 8 additions & 10 deletions neqo-bin/src/client/http09.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use std::{
time::Instant,
};

use neqo_common::{event::Provider, qdebug, qinfo, qwarn, Datagram};
use neqo_common::{event::Provider, qdebug, qinfo, qwarn, BorrowedDatagram};
use neqo_crypto::{AuthenticationStatus, ResumptionToken};
use neqo_transport::{
CloseReason, Connection, ConnectionEvent, EmptyConnectionIdGenerator, Error, Output, State,
Expand Down Expand Up @@ -177,15 +177,13 @@ impl TryFrom<&State> for CloseState {
}

impl super::Client for Connection {
fn process_output(&mut self, now: Instant) -> Output {
self.process_output(now)
}

fn process_multiple_input<'a, I>(&mut self, dgrams: I, now: Instant)
where
I: IntoIterator<Item = &'a Datagram>,
{
self.process_multiple_input(dgrams, now);
fn process_into_buffer<'a>(
&mut self,
input: Option<BorrowedDatagram>,
now: Instant,
out: &'a mut Vec<u8>,
) -> Output<BorrowedDatagram<'a>> {
self.process_into_buffer(input, now, out)
}

fn close<S>(&mut self, now: Instant, app_error: neqo_transport::AppError, msg: S)
Expand Down
18 changes: 8 additions & 10 deletions neqo-bin/src/client/http3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use std::{
time::Instant,
};

use neqo_common::{event::Provider, hex, qdebug, qinfo, qwarn, Datagram, Header};
use neqo_common::{event::Provider, hex, qdebug, qinfo, qwarn, BorrowedDatagram, Header};
use neqo_crypto::{AuthenticationStatus, ResumptionToken};
use neqo_http3::{Error, Http3Client, Http3ClientEvent, Http3Parameters, Http3State, Priority};
use neqo_transport::{
Expand Down Expand Up @@ -128,15 +128,13 @@ impl super::Client for Http3Client {
self.state().try_into()
}

fn process_output(&mut self, now: Instant) -> Output {
self.process_output(now)
}

fn process_multiple_input<'a, I>(&mut self, dgrams: I, now: Instant)
where
I: IntoIterator<Item = &'a Datagram>,
{
self.process_multiple_input(dgrams, now);
fn process_into_buffer<'a>(
&mut self,
input: Option<BorrowedDatagram>,
now: Instant,
out: &'a mut Vec<u8>,
) -> Output<BorrowedDatagram<'a>> {
self.process_into_buffer(input, now, out)
}

fn close<S>(&mut self, now: Instant, app_error: AppError, msg: S)
Expand Down
94 changes: 51 additions & 43 deletions neqo-bin/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use futures::{
future::{select, Either},
FutureExt, TryFutureExt,
};
use neqo_common::{qdebug, qerror, qinfo, qlog::NeqoQlog, qwarn, Datagram, Role};
use neqo_common::{qdebug, qerror, qinfo, qlog::NeqoQlog, qwarn, BorrowedDatagram, Role};
use neqo_crypto::{
constants::{TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256},
init, Cipher, ResumptionToken,
Expand Down Expand Up @@ -372,10 +372,12 @@ enum CloseState {

/// Network client, e.g. [`neqo_transport::Connection`] or [`neqo_http3::Http3Client`].
trait Client {
fn process_output(&mut self, now: Instant) -> Output;
fn process_multiple_input<'a, I>(&mut self, dgrams: I, now: Instant)
where
I: IntoIterator<Item = &'a Datagram>;
fn process_into_buffer<'a>(
&mut self,
input: Option<BorrowedDatagram>,
now: Instant,
out: &'a mut Vec<u8>,
) -> Output<BorrowedDatagram<'a>>;
fn has_events(&self) -> bool;
fn close<S>(&mut self, now: Instant, app_error: AppError, msg: S)
where
Expand All @@ -391,13 +393,34 @@ struct Runner<'a, H: Handler> {
handler: H,
timeout: Option<Pin<Box<Sleep>>>,
args: &'a Args,
recv_buf: Vec<u8>,
send_buf: Vec<u8>,
}

impl<'a, H: Handler> Runner<'a, H> {
fn new(
local_addr: SocketAddr,
socket: &'a mut crate::udp::Socket,
client: H::Client,
handler: H,
args: &'a Args,
) -> Self {
Self {
local_addr,
socket,
client,
handler,
args,
timeout: None,
recv_buf: vec![0; neqo_udp::RECV_BUF_SIZE],
send_buf: Vec::new(),
}
}

async fn run(mut self) -> Res<Option<ResumptionToken>> {
loop {
let handler_done = self.handler.handle(&mut self.client)?;
self.process_output().await?;
self.process(false).await?;
if self.client.has_events() {
continue;
}
Expand All @@ -418,7 +441,7 @@ impl<'a, H: Handler> Runner<'a, H> {
}

match ready(self.socket, self.timeout.as_mut()).await? {
Ready::Socket => self.process_multiple_input().await?,
Ready::Socket => self.process(true).await?,
Ready::Timeout => {
self.timeout = None;
}
Expand All @@ -432,37 +455,36 @@ impl<'a, H: Handler> Runner<'a, H> {
Ok(self.handler.take_token())
}

async fn process_output(&mut self) -> Result<(), io::Error> {
async fn process(&mut self, mut should_read: bool) -> Result<(), io::Error> {
loop {
match self.client.process_output(Instant::now()) {
let dgram = should_read
.then(|| self.socket.recv(&self.local_addr, &mut self.recv_buf))
.transpose()?
.flatten();
should_read = dgram.is_some();

match self
.client
.process_into_buffer(dgram, Instant::now(), &mut self.send_buf)
{
Output::Datagram(dgram) => {
self.socket.writable().await?;
self.socket.send(&dgram)?;
self.socket.send(dgram)?;
self.send_buf.clear();
continue;
}
Output::Callback(new_timeout) => {
qdebug!("Setting timeout of {:?}", new_timeout);
self.timeout = Some(Box::pin(tokio::time::sleep(new_timeout)));
break;
}
Output::None => {
qdebug!("Output::None");
break;
}
}
}

Ok(())
}

async fn process_multiple_input(&mut self) -> Res<()> {
loop {
let dgrams = self.socket.recv(&self.local_addr)?;
if dgrams.is_empty() {
if !should_read {
break;
}
self.client
.process_multiple_input(dgrams.iter(), Instant::now());
self.process_output().await?;
}

Ok(())
Expand Down Expand Up @@ -572,32 +594,18 @@ pub async fn client(mut args: Args) -> Res<()> {

let handler = http09::Handler::new(to_request, &args);

Runner {
args: &args,
client,
handler,
local_addr: real_local,
socket: &mut socket,
timeout: None,
}
.run()
.await?
Runner::new(real_local, &mut socket, client, handler, &args)
.run()
.await?
} else {
let client = http3::create_client(&args, real_local, remote_addr, &hostname, token)
.expect("failed to create client");

let handler = http3::Handler::new(to_request, &args);

Runner {
args: &args,
client,
handler,
local_addr: real_local,
socket: &mut socket,
timeout: None,
}
.run()
.await?
Runner::new(real_local, &mut socket, client, handler, &args)
.run()
.await?
};
}
}
Expand Down
Loading