[jthomas43]

delayed ack: ack every other packet. flush any pending ack during libuv's check phase

[marcus-pousette-hp]

This is cool! I benched it and I saw great perf gain on loopback. I got 22% throughput and 20% less CPU cost.

There are some thing that need to be fixed/considered though to land this, see comments inline and below.

1. If the peer's END arrives before its last data packet, the packet that fills the gap now takes the delayed-ack path. If we send data in the same tick (e.g. send_packets at the end of process_packet), the cancel in _send_packet stops the check. See the test below.

bool ack_now = seq != stream->ack || udx_sack_tree_min(&stream->sack_tree) != NULL; // before process_data_packet
...
if (stream->pending_acks >= UDX_MAX_PENDING_ACKS || type & UDX_HEADER_END || ack_now) {

test/stream-ooo-end-eof.c

.#include "../include/udx.h"
#include "../src/endian.h"
#include <assert.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>

// EOF must be delivered once the gap before the peer's END is filled, even when
// the packet that fills it also acks our data and we send more in the same tick.

uv_loop_t loop;
udx_t udx;

udx_socket_t sock;
udx_socket_t peer;
udx_stream_t stream;

struct sockaddr_in addr;
struct sockaddr_in peer_addr;

udx_stream_write_t *req;
uv_timer_t timer;

char data[65536];
bool gap_filled = false;
bool eof = false;

static void
peer_send (int type, uint32_t seq, uint32_t ack) {
struct {
 uint8_t magic, version, type, data_offset;
 uint32_t id, rwnd, seq, ack;
 char data[1];
} pkt = {0xff, 1, type, 0, udx__swap_uint32_if_be(1), udx__swap_uint32_if_be(0x100000), udx__swap_uint32_if_be(seq), udx__swap_uint32_if_be(ack), {'x'}};

uv_buf_t buf = uv_buf_init((char *) &pkt, sizeof(pkt));
int e = uv_udp_try_send(&peer.uv_udp, &buf, 1, (struct sockaddr *) &addr);
assert(e == sizeof(pkt));
}

static void
on_timeout (uv_timer_t *t) {
if (!gap_filled) {
 gap_filled = true;
 peer_send(UDX_HEADER_DATA, 0, 2); // fill the gap and ack 2 of our packets
 uv_timer_start(&timer, on_timeout, 500, 0);
 return;
}

uv_close((uv_handle_t *) &timer, NULL);
udx_stream_destroy(&stream);
}

static void
on_peer_recv (udx_socket_t *socket, ssize_t read_len, const uv_buf_t *buf, const struct sockaddr *from) {
if (read_len < UDX_HEADER_SIZE || gap_filled || uv_is_active((uv_handle_t *) &timer)) return;
if (buf->base[2] & UDX_HEADER_SACK) uv_timer_start(&timer, on_timeout, 10, 0); // END was sacked
}

static void
on_read (udx_stream_t *s, ssize_t read_len, const uv_buf_t *buf) {
if (read_len == UV_EOF) eof = true;
}

static void
on_close (udx_stream_t *s, int status) {
udx_socket_close(&sock);
udx_socket_close(&peer);
}

int
main () {
int e;

uv_loop_init(&loop);
uv_timer_init(&loop, &timer);

e = udx_init(&loop, &udx, NULL);
assert(e == 0);

e = udx_socket_init(&udx, &sock, NULL);
assert(e == 0);

e = udx_socket_init(&udx, &peer, NULL);
assert(e == 0);

uv_ip4_addr("127.0.0.1", 18141, &addr);
e = udx_socket_bind(&sock, (struct sockaddr *) &addr, 0);
assert(e == 0);

uv_ip4_addr("127.0.0.1", 18142, &peer_addr);
e = udx_socket_bind(&peer, (struct sockaddr *) &peer_addr, 0);
assert(e == 0);

e = udx_stream_init(&udx, &stream, 1, on_close, NULL);
assert(e == 0);

e = udx_stream_connect(&stream, &sock, 2, (struct sockaddr *) &peer_addr);
assert(e == 0);

e = udx_stream_read_start(&stream, on_read);
assert(e == 0);

e = udx_socket_recv_start(&peer, on_peer_recv);
assert(e == 0);

req = malloc(udx_stream_write_sizeof(1));
uv_buf_t buf = uv_buf_init(data, sizeof(data));
udx_stream_write(req, &stream, &buf, 1, NULL); // more than the initial window

peer_send(UDX_HEADER_END, 1, 0); // data seq 0 is "lost"

e = uv_run(&loop, UV_RUN_DEFAULT);
assert(e == 0);

e = uv_loop_close(&loop);
assert(e == 0);

free(req);

assert(eof);

return 0;
}

2. Tradeoff: ack delay vs UDX_TLP_MAX_ACK_DELAY. An alone packet's ack now waits until the whole read batch (up to 32 datagrams) has been processed, but the sender only allows 2 ms for the receiver to ack. When the receiver is busy, especially with several streams on one socket, that can cause unneeded resends. Can we first reason about whether this is a problem in practice? If it is, I see three options: send the pending ack as soon as the next packet in the batch is for a different stream, raise the allowance ,, or accept some extra resends.

This review was co-aided with AI

[jthomas43]

If the peer's END arrives before its last data packet, the packet that fills the gap now takes the delayed-ack path. If we send data in the same tick (e.g. send_packets at the end of process_packet), the cancel in _send_packet stops the check. See the test below.

Pushed a change: once the end packet is received, we will ack every packet - the END itself, or any gap packets that arrive. I also added the proposed test.

I'm realizing now that you proposed a fix too - yours would ack every packet as long as there is any gap if I understand it, END included, so it will work too. TCP disables delayed ACK in the presence of SACK and reordering, I don't think it's necessary for us since we don't really delay the ACK for 25+ms like TCP implementations do.

   if (type & UDX_HEADER_DATA_OR_END) {
-    if (stream->pending_acks >= UDX_MAX_PENDING_ACKS || type & UDX_HEADER_END) {
+    if (stream->pending_acks >= UDX_MAX_PENDING_ACKS || stream->status & UDX_STREAM_ENDING_REMOTE) {
       send_ack(stream);

I think this is OK to ensure that we don't break users, but ultimately we should deliver the EOF to the user immediately after the final read. It's hard to tell if this quirk is just an accident or is maintained because some downstream code inadvertently depends on it. Needs investigation.

Tradeoff: ack delay vs UDX_TLP_MAX_ACK_DELAY. An alone packet's ack now waits until the whole read batch (up to 32 datagrams) has been processed, but the sender only allows 2 ms for the receiver to ack. When the receiver is busy, especially with several streams on one socket, that can cause unneeded resends. Can we first reason about whether this is a problem in practice? If it is, I see three options: send the pending ack as soon as the next packet in the batch is for a different stream, raise the allowance ,, or accept some extra resends.

I think 2ms is OK to budget for the cost of waiting for the delayed ack. I don't think the cost of being wrong is very high: just an unnecessary TLP on a mostly idle connection (idle since it had only one packet to put in flight). I think it's OK, but 5ms would be OK too.

[marcus-pousette-hp]

Nice, the last point I made seem to be fixed! Two additional points, see comments below. First one is blocking (unless there is some miss-understading by me). I also reran benches before and after last review and the performance gain is the same.

Co-reviewed with AI

[marcus-pousette-hp]

After a uv_run(UV_RUN_NOWAIT) that reads a lone packet, the ack should already be sent, or uv_backend_timeout() should be 0. Electron (>= 42.9.3) and bare_poll() hosts sleep for that timeout. This prepare only runs on the next uv_run and isn't counted in it, so the ack waits until something else wakes the app, and the sender resends. I think this test should pass. It fails on this branch and passes on main and the previous verison of this PR abbec2b:

#include "../include/udx.h"
#include <assert.h>
#include <stdbool.h>
#include <stdlib.h>

// bare_poll() and Electron run uv_run(UV_RUN_NOWAIT), then sleep for uv_backend_timeout().
// After the run that reads a lone packet, its ack must be sent or the timeout must be 0.

uv_loop_t loop;
udx_t udx;
struct sockaddr_in addr;
udx_socket_t sock;

udx_stream_t astream;
udx_stream_t bstream;

udx_stream_write_t *req;

bool read_called = false;

void
on_read (udx_stream_t *handle, ssize_t read_len, const uv_buf_t *buf) {
  read_called = true;
}

int
main () {
  int e;

  req = malloc(udx_stream_write_sizeof(1));

  uv_loop_init(&loop);

  e = udx_init(&loop, &udx, NULL);
  assert(e == 0);

  e = udx_socket_init(&udx, &sock, NULL);
  assert(e == 0);

  uv_ip4_addr("127.0.0.1", 18151, &addr);
  e = udx_socket_bind(&sock, (struct sockaddr *) &addr, 0);
  assert(e == 0);

  e = udx_stream_init(&udx, &astream, 1, NULL, NULL);
  assert(e == 0);

  e = udx_stream_init(&udx, &bstream, 2, NULL, NULL);
  assert(e == 0);

  e = udx_stream_read_start(&astream, on_read);
  assert(e == 0);

  e = udx_stream_connect(&astream, &sock, 2, (struct sockaddr *) &addr);
  assert(e == 0);

  e = udx_stream_connect(&bstream, &sock, 1, (struct sockaddr *) &addr);
  assert(e == 0);

  uv_buf_t buf = uv_buf_init("a", 1);
  udx_stream_write(req, &bstream, &buf, 1, NULL);

  while (!read_called) {
    uv_run(&loop, UV_RUN_NOWAIT);
  }

  assert(astream.packets_tx == 1 || uv_backend_timeout(&loop) == 0);

  udx_stream_destroy(&astream);
  udx_stream_destroy(&bstream);
  udx_socket_close(&sock);

  uv_run(&loop, UV_RUN_DEFAULT);

  free(req);

  return 0;
}

[marcus-pousette-hp]

I think 2ms is OK to budget for the cost of waiting for the delayed ack. I don't think the cost of being wrong is very high: just an unnecessary TLP on a mostly idle connection (idle since it had only one packet to put in flight). I think it's OK, but 5ms would be OK too.

My first comment was only about a lone packet, and there I agree 2 ms is enough. The bigger case turned out to be two packets in flight (after reviewing this again): the allowance is only added when one packet is in flight, so with two the PTO is just 2*srtt (2 ms on a LAN). If the two packets land in different read batches, the first one's ack waits until the receiver has worked through its batch. With about a millisecond of work behind it, two-thirds to nearly all of these flights got a spurious TLP in some simulated runs I did. 5 ms would not help either here, since the allowance is not added there.

This failure mode is kind of narrow, but what would you think be a good way to work around this?

[jthomas43]

After a uv_run(UV_RUN_NOWAIT) that reads a lone packet, the ack should already be sent, or uv_backend_timeout() should be 0. Electron (>= 42.9.3) and bare_poll() hosts sleep for that timeout.

OK, uv_check won't work on windows and uv_prepare won't work on UV_RUN_NOWAIT hosts. I've reworked the code to use a uv_check with a dummy uv_idle to force the poll timeout to zero, which should handle both cases. It also changes the pending (data) packet to use the same uv_check / uv_idle pair. I've added the test too.

If the two packets land in different read batches, the first one's ack waits until the receiver has worked through its batch. With about a millisecond of work behind it, two-thirds to nearly all of these flights got a spurious TLP in some simulated runs I did.

We could just add the 2ms in all circumstances, it's not the RFC but it looks like Linux does it. They have a huge allowance if a single packet is in flight (since they have an actual delayed ack timer ), but they also have a 2ms allowance for multiple packets in flight for when the rtt+jitter is close to the clock granularity. Let me know and I'll add it in.