perf(clients): coalesce acks for buffered publishes into one write; fast ack encoder - #552

What

A burst of QoS 1 publishes from one client now gets its PUBACKs in a few writes instead of one write(2) per message. Also:

• PUBACK/PUBREC/PUBREL/PUBCOMP encoding is about 60× cheaper in the common case (no Reason String, no User Properties).
• ReadPacket drops a redundant second copy of every inbound packet.

A single publish with nothing behind it is still acked immediately by its own write. No exported API or config changes.

Why

processPublish writes the PUBACK synchronously through WritePacket, so every QoS 1 message costs one encode and one write(2). WritePacket already has a write buffer (outbuf), but it only holds bytes while WriteLoop's outbound queue is non-empty. A client that only publishes has an empty queue, so every ack goes straight to the socket.

Production profiling of a broker with ~1,000 publishing clients (QoS 1, MQTT 5) showed this clearly:

• write syscalls were ~13% of all CPU.
• ~99% of WritePacket time came from processPublish, i.e. one PUBACK per message.
• PubackEncode was ~16% of WritePacket. Most of that was Properties.Encode walking every property kind through nested map lookups, for a packet type that can carry only two properties.

Fix

Coalesce acks while more input is already buffered. After decoding a packet, Client.Read checks whether the next packet is already complete in its bufio.Reader. This is a Peek over bytes already buffered, so it does no I/O. While that is true, success acks are appended to outbuf under the existing client lock instead of being written. They go through a new unexported writeAck, which covers:

• PUBACK/PUBREC from processPublish
• PUBREL from processPubrec
• PUBCOMP from processPubrel

outbuf is flushed when any of these happens:

Write deadlines are unchanged. OnPacketSent fires when the bytes are buffered, which is what already happened when WritePacket buffered behind a non-empty outbound queue.

Fast ack encoder. These four packet types can only carry Reason String and User Properties. Both are suppressed when the client disallowed problem info. When neither will be written, the packet is a fixed 2–3 byte body, so it is now written directly (~620 ns → ~10 ns). Anything else falls through to the existing encoder, unchanged. TestEncodePubAckRelRecCompFastPathMatchesGeneral compares the two paths byte for byte over 1,200 combinations of type, protocol version, reason code, properties, DisallowProblemInfo and MaxSize.

ReadPacket allocated p fresh for every packet and then copied it into another fresh slice before decoding. The copy bought no isolation, so it is removed (−1 allocation and one memmove per inbound packet).

A note on QoS 1

In processPublish the inbound QoS 1 inflight entry is deleted right after the PUBACK is handed to writeAck. With this change the PUBACK may still be in outbuf at that point, for at most the coalescing window (1 ms, or until the burst ends). If the connection dies inside that window, the client never sees the PUBACK and resends. The broker has already delivered the message, so it is delivered twice. QoS 1 permits this ("at least once"), and it could already happen when a PUBACK was lost after write(2) returned. The window is wider, though, so it seemed worth stating. QoS 2 is unaffected: its PUBREC entry stays inflight until PUBREL.

Benchmarks

ack_coalesce_bench_test.go drives the broker through a real TCP listener on loopback. Each client keeps 32 QoS 1 publishes in flight, one conn.Write per publish, like a client library would. It counts the broker's conn.Write calls through a wrapped net.Listener. main vs this PR, interleaved, benchstat n=8, 32-thread host:

go test -run '^$' -bench QoS1 -benchtime 100000x .

All changes are p ≤ 0.016. allocs/op is 11 → 10 in every case. The idle row is the one that must not regress: with nothing buffered behind a publish, its PUBACK still goes out in its own write, immediately. The small latency gain there comes from the cheaper encoder. The OnPublish rows use a hook that busy-waits 20 µs to stand in for real per-message work.

Tests

Real TCP (ack_coalesce_tcp_test.go):

Unit (ack_coalesce_test.go, over net.Pipe):

• a slow reader: held bytes are capped at ClientNetWriteBufferSize before the writer blocks, and nothing is lost or reordered once it reads.
• held acks are flushed in order, in one write, by another packet.
• close while acks are held.
• Read flushes held acks when the burst ends in a handler error.
• a table for nextPacketBuffered: zero-length packet, multi-byte length, partial length, malformed length.

The assertions are on bytes and write counts, not elapsed time.

Each guard was checked by mutation: removing the DISCONNECT flush, the delay bound, the Read exit flush, or the partial-packet check makes the matching test fail.

The PR also fixes the TestServerProcessPublishACLCheckDeny flake. It asserted cl.Closed() before wg.Wait(), but Stop closes the conn (ending the test's ReadAll) before it marks the client closed. With the assertion after wg.Wait() it passes 200/200 under -race.

Commands run:

go test -race ./...                                                                              # all packages pass
go test -race -count=5 .                                                                         # pass
go test -race -cpu 1,2,8 -count=30 -run 'TestAckCoalesce|TestWriteAck|TestReadFlushes|TestNextPacketBuffered|TestServerProcessPublishACLCheckDeny$' .   # pass
go test -race -count=200 -run 'TestServerProcessPublishACLCheckDeny$' .                          # 200/200

Related

Independent of #507, #508, #549, #550 and #551. It merges cleanly with each of them (checked with git merge-tree), so they can land in any order.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KH4BjF1aiSybX9GxM8GXBR