Reducing EndpointDriver state lock contention: split incoming accept around the endpoint lock

[stablebits]

Accept API currently hold the endpoint lock across TLS session setup, Connection::new(), and Connection::handle_first_packet(). Under a high rate of new connections, that stalls the endpoint driver whenever it encounters a new Initial, which in turn cuts throughput for already-established traffic.

This change splits server-side accept into three phases:

First, claim the Incoming and reserve the routing/CID state under the endpoint lock.
Second, run the expensive TLS session creation and first-packet handling without the lock.
Third, reacquire the lock briefly to either activate the connection and replay buffered Initial/0-RTT datagrams, or clean up and send an Initial-close response on failure.

That transient state is made explicit in quinn-proto with an Accepting connection state. Packets that arrive during the split window are routed to Accepting and buffered until finalization, so retransmitted Initials and 0-RTT packets are preserved.

f939eb0 is included for completeness (test results below), but should be dropped.

Is this approach acceptable? If no, what could be a better alternative?

Test results:

    $ NEW_CONN_RATE=1000 ./bench/run_contention_compare.sh
    throughput: connections=1 streams_per_connection=1 stream_size=1600M stream_runs=1
    new-conn: rate=... workers=16 duration_secs=30 warmup_secs=2

The numbers below are for 500 and 1000 new connections per second.

baseline

    baseline throughput: 384.12 MiB/s (elapsed 4.165s)
    mixed throughput:    209.43 MiB/s (elapsed 7.640s)
    mixed new-conn rate: 500.00 /s
    degradation:         45.48%

    baseline throughput: 377.83 MiB/s (elapsed 4.235s)
    mixed throughput:    43.00 MiB/s (elapsed 37.212s)
    mixed new-conn rate: 1000.00 /s
    degradation:         88.62%

with this change

    baseline throughput: 382.34 MiB/s (elapsed 4.185s)
    mixed throughput:    307.89 MiB/s (elapsed 5.197s)
    mixed new-conn rate: 500.00 /s
    degradation:         19.47%

    baseline throughput: 391.27 MiB/s (elapsed 4.089s)
    mixed throughput:    290.59 MiB/s (elapsed 5.506s)
    mixed new-conn rate: 1000.00 /s
    degradation:         25.73%

[djc]

Splitting up a lock makes sense to me in principle but it feels like this adds a lot of code to do that, more than I'd intuitively expect.

Also, can you better quantify which (more granular) parts of the acceptance process are taking up time?

[stablebits]

Also, can you better quantify which (more granular) parts of the acceptance process are taking up time?

The overhead comes from Connection::handle_first_packet() (this, along with Connection::new(), is what runs without holding the lock in finish_without_endpoint() now).

So on the server side (app's accept()), handle_first_packet() (-> process_decrypted_packet() -> process_early_payload() -> (read_crypto() && write_crypto()) is where rustls processes the TLS handshake.

I don't have a full profile at hand. But this is a screenshot that I used some time ago for internal discussions:

[stablebits]

Splitting up a lock makes sense to me in principle but it feels like this adds a lot of code to do that, more than I'd intuitively expect.

@djc the relevant core changes are basically:

So like ~350 lines of code. Lots of changes are boilerplate code around a new Accepting type. Some changes are due to renaming/refactoring, and in principle could be avoided. Changes in bench/ are to be discarded (as I mentioned). Then there are some new tests. I'll try to reduce the diff more (btw., there are multiple commits introducing changes one by one).

Do you have any specific recommendations?

[stablebits]

Reduced the scope of changes:

  Core code: 7 files, +425 / −115 (net +310 lines)
  Tests: 2 files, +362 / −2 (net +360 lines)

[djc]

So like ~350 lines of code. Lots of changes are boilerplate code around a new Accepting type. Some changes are due to renaming/refactoring, and in principle could be avoided. Changes in bench/ are to be discarded (as I mentioned). Then there are some new tests. I'll try to reduce the diff more (btw., there are multiple commits introducing changes one by one).

Looking specifically at the second commit (quinn-proto: split server-side accept around endpoint lock), it introduces what I consider a substantial amount of new code, a lot of which seems repetitive. Suggest splitting that up into smaller changes?

[stablebits]

Looking specifically at the second commit (quinn-proto: split server-side accept around endpoint lock), it introduces what I consider a substantial amount of new code, a lot of which seems repetitive. Suggest splitting that up into smaller changes?

Thanks for the review. Took another pass at the second commit. Removed/simplified some things. The 2nd commit is now 336+/130−.

On splitting the commit further

Happy to do this if it helps review, but the smaller commits will probably not stand on their own — each only makes sense because the lock-split commit follows. Still worth pursuing?

The two new types

• AcceptReservation (5 fields, Copy): the persistent identity of an in-flight accept — CIDs, init-CID, buffer-slot index, addresses. Created under the lock in start_accept(); consumed under the lock again in finish_accept() / finish_accept_error() to either register the connection or free the routing state. It bridges the lock-released window so retransmitted Initials and 0-RTT keep landing in the right buffer.
• Accepting (14 fields): the off-lock work package. Inputs to TLS / Connection::new() / handle_first_packet(). Each field is captured once under the lock and used once after release. Consumed in the middle phase; doesn't reappear under the lock.

The flow:

     start_accept()            finish_without_endpoint()        finish_accept()
     [LOCK HELD]               [NO LOCK]                        [LOCK HELD]
     ─────────────────────     ──────────────────────────────   ─────────────
     build Accepting {         consume Accepting               consume Accepted
       reservation,    ──────► run TLS, Connection::new,  ───► register conn,
       14 other            │   handle_first_packet            remove_accept_reservation(reservation)
       fields             ───►                                 (frees CIDs, buffer slot)
     }                    │   on Ok → Accepted {
                          │     reservation,  ◄── same one
                          │     conn,
                          │   }
                          │   on Err → AcceptingError {
                          │     reservation,  ◄── same one
                          │     ...
                          │   }                              finish_accept_error()
                          │                                  [LOCK HELD]
                          │                                  ─────────────
                          └─────────────────────────────────► consume AcceptingError
                                                              remove_accept_reservation(reservation)

Three phases (start_accept() / finish_without_endpoint() / finish_accept[_error]()) are the lock split.

All internal: methods are doc(hidden), state types only re-export under __internal_split_accept, not semver. Endpoint::accept() is unchanged for direct users.

[stablebits]

Looking specifically at the second commit (quinn-proto: split server-side accept around endpoint lock), it introduces what I consider a substantial amount of new code, a lot of which seems repetitive. Suggest splitting that up into smaller changes?

Thanks for the review. Took another pass at the second commit. Removed/simplified some things. The 2nd commit is now 336+/130−.

On splitting the commit further

Happy to do this if it helps review, but the smaller commits will probably not stand on their own — each only makes sense because the lock-split commit follows. Still worth pursuing?

I've split the 2nd commit into smaller ones. The largest (main) one is now 259+/64− (was 336+/130−). Happy to reorder/squash differently if it helps.

[djc]

Have reviewed up to this commit, the previous commits looked fine. I want a different commit history for this:

• Extract the logic that previously lived in add_connection() into a separate method register_connection(). The new method should live below the existing one, since it is called by it and we follow top-down ordering.
• Move logic from insert_conn() into register_connection().
• Move the self.index.insert_initial() call.

[stablebits]

Thanks for the feedback! Split it into 3 separate commits starting with 885c1fc

[djc]

Sorry for the delay...

[djc]

Okay, I got to "inline insert_conn into register_connection". For these changes, I want to split into (1) a commit that moves the code and (2) a commit that changes the code.

(This is basically the same for all the changes you're submitting -- please self-review.)

In order to make progress here, maybe submit a separate PR with the commits before this one, so we can get it merged? This is pretty hard to digest.

[stablebits]

Thanks! The first part is here #2771