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

Allow setting TCP_NODELAY on socket (to save 40ms on connection latency) #3043

Closed
mirek26 opened this issue Feb 9, 2024 · 4 comments · Fixed by #3055
Closed

Allow setting TCP_NODELAY on socket (to save 40ms on connection latency) #3043

mirek26 opened this issue Feb 9, 2024 · 4 comments · Fixed by #3055
Labels
enhancement New feature or request

Comments

@mirek26
Copy link
Contributor

mirek26 commented Feb 9, 2024

Is your feature request related to a problem? Please describe.

We're using sqlx for a latency-sensitive use case and while connection pooling mitigates the problem, connecting to (postgres) database is very slow, causing latency spikes in our service. Running a very basic

sqlx::postgres::PgConnection::connect_with(&options).await

takes consistently 140ms. When we do the same with psycopg2 in Python (which uses libpq), it's consistently 16-20ms. Based on tcp dump (see below) it's pretty clear that 40ms can be attributed to waiting for a delayed tcp ack from database - the client doesn't finish sending data due to Nagle's algorithm.

Describe the solution you'd like

I'd like an option to set TCP_NODELAY option on the socket when it's created, i.e.

let stream = TcpStream::connect((host, port)).await?;
stream.set_nodelay(true)?;

When we add the second line in https://github.com/launchbadge/sqlx/blob/v0.7.3/sqlx-core/src/net/socket/mod.rs#L198, we see a consistent improvement by 40ms. This bring connection latency to ~100ms, which is still bad, but slightly less bad. If people have ideas on how we could improve this further, I'm all ears.

I was able to confirm with strace that pyscopg2 does set TCP_NODELAY by default and it's not even runtime-configurable (it's compile-time configurable in libpq).

I don't understand the code well enough to recommend where this configuration should live. ConnectOptions?

Describe alternatives you've considered

Turn it on without a way to opt-out? Some frameworks do that but seems better to make it configurable

Additional context

This is on sqlx 0.7.3 with tokio, running on debian-bullseye on AWS ECS connecting to Aurora Postgres in the same AWS region. We're seeing the problem with multiple services and multiple databases - the connection latency is consistently bad.

TCP dumps before and after (=with a patch that adds stream.set_nodelay(true)?):
tcpdump-before.txt
tcpdump-after.txt

Good context on TCP_NODELAY is here or here.

@mirek26 mirek26 added the enhancement New feature or request label Feb 9, 2024
@mirek26
Copy link
Contributor Author

mirek26 commented Feb 11, 2024

Update:

  1. This only reproduces with TLS. Both with "tls-native-tls" and "tls-rustls". Without TLS, things are much faster and we don't see any effect of delayed ACKs. This makes some sense because the packet structure with and without TLS is just different.

  2. The numbers above are inaccurate. I made a noob mistake and assumed that db connections are not CPU-heavy.. so I collected those numbers in debug mode. But SASL authentication is very slow on unoptimized build. The real numbers in release mode are

    • before (without TCP_NODELAY): 68ms
    • after (with TCP_NODELAY): 23ms -> ~2.5x improvement

23ms is a pretty reasonable number - there are probably other optimizations to do here but this seems to be the major issue.

@abonander
Copy link
Collaborator

I'd be fine setting TCP_NODELAY by default, since we buffer data internally anyway.

@mirek26
Copy link
Contributor Author

mirek26 commented Feb 11, 2024

@abonander thanks for looking - with internal buffering, do you think this should be configurable at all? I'm happy to prep a PR if we agree on what to do here: (a) always set it - very easy, not sure if there are cases where you'd not want this (b) add it to PgConnectOptions/MySqlConnectOptions and set via WithSocket

@abonander
Copy link
Collaborator

Libpq hardcodes it, that's sufficient precedent for me: https://github.com/postgres/postgres/blob/e70abd67c3e6155fe8e853c4e29255578d9cf48d/src/backend/libpq/pqcomm.c#L743

There's no real benefit to Nagle's algorithm if the application does its own buffering so I don't see a need for a toggle. IMO, NODELAY should be the default nowadays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants