fix(udp): pre-allocate receive buffer #1707

mxinden · 2024-03-02T09:45:31Z

Instead of allocating a new receive buffer on each call to neqo_common::udp::Socket::recv, preallocate a long-lived buffer in Socket::bind and reuse this buffer on each read.

See flamegraphs below. With the patch neqo_common::udp::Socket::recv simply delegates to tokio::net::udp::UdpSocket::try_io instead of spending significant time in memset.

Quick-fix while working on #1693.

Addresses first performance issue in #1690.

Before:

After:

Instead of allocating a new receive buffer on each call to `neqo_common::udp::Socket::recv`, preallocate a long-lived buffer in `Socket::bind` and reuse this buffer on each read.

codecov · 2024-03-02T10:00:08Z

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 92.97%. Comparing base (ad027cf) to head (6d3491e).

Files	Patch %	Lines
neqo-common/src/udp.rs	60.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1707      +/-   ##
==========================================
- Coverage   92.98%   92.97%   -0.01%     
==========================================
  Files         120      120              
  Lines       37340    37342       +2     
==========================================
- Hits        34719    34718       -1     
- Misses       2621     2624       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mxinden · 2024-03-02T18:02:11Z

neqo-common/src/udp.rs

        Ok(Some(Datagram::new(
            meta.addr,
            *local_address,
            meta.ecn.map(|n| IpTos::from(n as u8)).unwrap_or_default(),
            None, // TODO: get the real TTL https://github.com/quinn-rs/quinn/issues/1749
-            &buf[..meta.len],
+            &self.recv_buf[..meta.len],
        )))


Note that each call to Datagram::new allocates the datagram's data in a new Vec.

neqo/neqo-common/src/datagram.rs

Lines 20 to 35 in ad027cf

impl Datagram {

pub fn new<V: Into<Vec<u8>>>(

src: SocketAddr,

dst: SocketAddr,

tos: IpTos,

ttl: Option<u8>,

d: V,

) -> Self {

Self {

src,

dst,

tos,

ttl,

d: d.into(),

}

}

Changing this is a larger undertaking I would like to tackle with #1693.

martinthomson · 2024-03-04T00:10:19Z

neqo-common/src/udp.rs

@@ -31,6 +32,7 @@ impl Socket {
        Ok(Self {
            state: quinn_udp::UdpSocketState::new((&socket).into())?,
            socket: tokio::net::UdpSocket::from_std(socket)?,
+            recv_buf: vec![0; u16::MAX as usize],


This is too large. I get that we don't want to overflow, but unless we are receiving multiple datagrams, we only need 2k or so (a constant might be a good idea for that limit).

Why is this too large? This is about ~43 packets, which is smaller than realistic WAN congestion windows. Are you worried about Fx memory usage of this is per-socket?

Ignore the comment. I was reading this patch on its own and missed the follow-up that includes multiple packets.

I do think that a constant is the right thing to use for this, rather than the (rather arbitrary) u16::MAX.

I do think that a constant is the right thing to use for this, rather than the (rather arbitrary) u16::MAX.

👍 I prepared #1709.

I was reading this patch on its own and missed the follow-up that includes multiple packets.

For others to follow, this is #1708.

There are two server implementations based on neqo: 1. https://github.com/mozilla/neqo/tree/main/neqo-bin/src/server - http3 and http09 implementation - used for manual testing and QUIC Interop 2. https://searchfox.org/mozilla-central/source/netwerk/test/http3server/src/main.rs - used to test Firefox I assume one was once an exact copy of the other. Both implement their own I/O, event loop, ... Since then, the two implementations diverged significantly. Especially (1) saw a lot of improvements in recent months: - mozilla#1564 - mozilla#1569 - mozilla#1578 - mozilla#1581 - mozilla#1604 - mozilla#1612 - mozilla#1676 - mozilla#1692 - mozilla#1707 - mozilla#1708 - mozilla#1727 - mozilla#1753 - mozilla#1756 - mozilla#1766 - mozilla#1772 - mozilla#1786 - mozilla#1787 - mozilla#1788 - mozilla#1794 - mozilla#1806 - mozilla#1808 - mozilla#1848 - mozilla#1866 At this point, bugs in (2) are hard to fix, see e.g. mozilla#1801. This commit merges (2) into (1), thus removing all duplicate logic and having (2) benefit from all the recent improvements to (1).

* refactor(bin): introduce server/http3.rs and server/http09.rs The QUIC Interop Runner requires an http3 and http09 implementation for both client and server. The client code is already structured into an http3 and an http09 implementation since #1727. This commit does the same for the server side, i.e. splits the http3 and http09 implementation into separate Rust modules. * refactor: merge mozilla-central http3 server into neqo-bin There are two server implementations based on neqo: 1. https://github.com/mozilla/neqo/tree/main/neqo-bin/src/server - http3 and http09 implementation - used for manual testing and QUIC Interop 2. https://searchfox.org/mozilla-central/source/netwerk/test/http3server/src/main.rs - used to test Firefox I assume one was once an exact copy of the other. Both implement their own I/O, event loop, ... Since then, the two implementations diverged significantly. Especially (1) saw a lot of improvements in recent months: - #1564 - #1569 - #1578 - #1581 - #1604 - #1612 - #1676 - #1692 - #1707 - #1708 - #1727 - #1753 - #1756 - #1766 - #1772 - #1786 - #1787 - #1788 - #1794 - #1806 - #1808 - #1848 - #1866 At this point, bugs in (2) are hard to fix, see e.g. #1801. This commit merges (2) into (1), thus removing all duplicate logic and having (2) benefit from all the recent improvements to (1). * Move firefox.rs to mozilla-central * Reduce HttpServer trait functions * Extract constructor * Remove unused deps * Remove clap color feature Nice to have. Adds multiple dependencies. Hard to justify for mozilla-central.

fix(udp): pre-allocate receive buffer

6d3491e

Instead of allocating a new receive buffer on each call to `neqo_common::udp::Socket::recv`, preallocate a long-lived buffer in `Socket::bind` and reuse this buffer on each read.

mxinden requested review from KershawChang, martinthomson and larseggert as code owners March 2, 2024 09:45

mxinden commented Mar 2, 2024

View reviewed changes

larseggert approved these changes Mar 3, 2024

View reviewed changes

larseggert added this pull request to the merge queue Mar 3, 2024

Merged via the queue into mozilla:main with commit a2d525b Mar 3, 2024
11 of 13 checks passed

martinthomson reviewed Mar 4, 2024

View reviewed changes

mxinden mentioned this pull request Mar 5, 2024

refactor(udp): define const for receive buffer size #1709

Merged

mxinden mentioned this pull request May 4, 2024

refactor: enable mozilla-central http3server to use neqo-bin #1878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(udp): pre-allocate receive buffer #1707

fix(udp): pre-allocate receive buffer #1707

mxinden commented Mar 2, 2024

codecov bot commented Mar 2, 2024

mxinden Mar 2, 2024

martinthomson Mar 4, 2024

larseggert Mar 4, 2024

martinthomson Mar 4, 2024

mxinden Mar 5, 2024

	impl Datagram {
	pub fn new<V: Into<Vec<u8>>>(
	src: SocketAddr,
	dst: SocketAddr,
	tos: IpTos,
	ttl: Option<u8>,
	d: V,
	) -> Self {
	Self {
	src,
	dst,
	tos,
	ttl,
	d: d.into(),
	}
	}

fix(udp): pre-allocate receive buffer #1707

fix(udp): pre-allocate receive buffer #1707

Conversation

mxinden commented Mar 2, 2024

codecov bot commented Mar 2, 2024

Codecov Report

mxinden Mar 2, 2024

Choose a reason for hiding this comment

martinthomson Mar 4, 2024

Choose a reason for hiding this comment

larseggert Mar 4, 2024

Choose a reason for hiding this comment

martinthomson Mar 4, 2024

Choose a reason for hiding this comment

mxinden Mar 5, 2024

Choose a reason for hiding this comment