perf(qwp): batch WebSocket sends and cut per-frame syscalls by jerrinot · Pull Request #121 · questdb/java-questdb-client

jerrinot · 2026-11-04T17:16:32Z

Tandem: questdb/questdb#8240, questdb/questdb-enterprise#1396

Makes the QWP WebSocket sender much cheaper per frame. Applications that call flush() per row, or run many small flushes, get up to 2x more throughput per connection on a real network and a fraction of the client CPU; applications that flush large batches are unchanged. Wire bytes are identical, only how frames are grouped into TCP writes changes.

What changes

Send first, poll only on backpressure. doSend no longer waits on epoll before every send; it waits only when the socket is full. Two syscalls fewer per frame.
Batch published frames into one send. The I/O loop now sends every frame already published, up to 64 KiB per write, instead of one frame per pass. ACKs are still read between batches, so the fairness window is bounded by the cap, and a frame larger than the cap goes out alone.
One clock read per batch, none when the interval is off. The auto-flush age check no longer runs on the row that was just timestamped (it could never fire there). auto_flush_interval=off is now accepted for ws::/wss:: as an explicit opt-out; the default stays 100 ms. With it off, auto_flush_rows, auto_flush_bytes and explicit flush() still apply, and the sender reads no clock on the row path. Disabling auto-flush entirely (auto_flush=off, disableAutoFlush()) stays rejected on WebSocket.
Compact the receive buffer lazily. The unread tail was memmoved after every parsed frame, which is quadratic in a burst of ACKs. It is now compacted once, before the next recv.

Measured

Client on a Ryzen desktop, server a Rust QWP mock on a Raspberry Pi 5 over 1 GbE (0.17 ms RTT), main vs this branch, 3 interleaved reps:

workload	main	this branch
1 row per `flush()`, saturation	1.55M rows/s, 500 Mbit/s, I/O thread 100%	3.31M rows/s, 948 Mbit/s (line rate), I/O thread 11%
1 row per `flush()`, 10k flushes/s, I/O thread CPU	6.1%	5.3%
1 row per `flush()`, 50k flushes/s, I/O thread CPU	15-19%	13-17%
1000 rows per `flush()`	1.84M rows/s, link-bound	1.84M rows/s, link-bound

Same client against the mock on a 20-core x86 box over 2.5 GbE (0.31 ms RTT), client pinned to one chiplet, 3 interleaved reps:

workload	main	this branch
1 row per `flush()`, saturation	1.62M rows/s, 490 Mbit/s, I/O thread 100%	5.03M rows/s (5.53M with `auto_flush_interval=off`), both client threads ~100% (client-bound, link not saturated)
1 row per `flush()`, 10k flushes/s, I/O thread CPU	9.1%	7.5%
1 row per `flush()`, 50k flushes/s, I/O thread CPU	14.6%	11.9%
1000 rows per `flush()`	4.57M rows/s, link-bound	4.57M rows/s, link-bound

On loopback (no link limit) the per-row saturation case goes from 136k to 2.8M rows/s with client CPU per flush dropping from 14.7 µs to 0.6 µs. The lazy compaction is neutral against a server that acks per read pass and bounds a pathological cost when hundreds of acks arrive in one recv (I/O thread 100% to 2% in that stress).

Notes

Each change has a regression test proven to fail without it; the full JDK 8 suite passes and JDK 25 compiles.
A zero send timeout now makes one attempt before timing out; no caller passes one.
init_buf_size/max_buf_size remain rejected for ws::; the 64 KiB batch cap is a constant, not a knob, because measurements showed batch size is set by how far the producer runs ahead, not by the cap.

doSend() waited on EPOLLOUT before every send(), costing epoll_ctl + epoll_wait per WebSocket frame even when the socket was writable. Try send() first and fall back to ioWait only when the socket is full (send returns ERETRY/0), mirroring recvOrDie's recv-first pattern.

trySendOne sent one WebSocket frame per send() syscall even with many frames already published. Walk all fully published frames up to the send buffer's capacity, encode them back to back (exact-size masked headers), and flush with one doSend. ACK fairness is now bounded by the byte cap rather than by one frame per iteration. Per-frame checks and bookkeeping are unchanged; early exits flush appended frames first so behaviour matches the old path where earlier frames were already sent.

tryParseFrame compacted the unread tail after every parsed frame, so a burst of N small frames in one recv cost N memmoves of a shrinking tail (quadratic in the burst; 82% of the I/O thread's CPU under per-frame server acks). Rewind the cursors only when the buffer is fully consumed and compact a partial tail once, right before the next recv and before the grow check, so the buffer state at every recv is unchanged.

The batch cap followed the send buffer's live capacity, which only ever grows, so one large frame raised the ACK-fairness window for the rest of the connection. Use the default send buffer size as a constant cap; a frame larger than it still goes out alone.

jerrinot · 2026-11-04T17:59:42Z

Review: perf(qwp): batch WebSocket sends and cut per-frame syscalls

Verdict: approve with comments. There are no Critical findings and the test gate passes. I'd like the two Moderate items fixed in this PR, but neither blocks the merge.

Reviewed head 2732e4a1 against base 02ce9b98.

What I verified:

Full suite: the whole core test suite passes on JDK 8 (3797 tests, 0 failures, 0 errors, 10 skipped).
JDK 25: test-compile succeeds.
Regression tests: I re-ran each new test with its production change reverted. Every test named in the PR description fails as claimed. The exception is Moderate 2: one changed line has no test.
Correctness: the batch loop keeps every per-frame check from trySendOne, and they still run in the old order. A failed append or flush always ends with a fresh connection or a stopped loop, so leftover bytes in the send buffer are never sent. The store-and-forward invariants are unchanged: no new transport errors reach the producer, no reconnect time limit, and no acknowledgement advance past an unsent frame.

Moderate

1. A failed batch flush inflates `getTotalFramesReplayed()`

Problem: frames that never reached the wire are counted as "replayed".
Net impact: the public replay metric over-counts by up to one batch per disconnect detected on write. Data and wire bytes are not affected.
Evidence: I ran the same probe on both revisions (test below). The first connection's socket reports a peer reset on its first write, then a reconnect succeeds. Base 02ce9b98 reports framesReplayed=0; head 2732e4a1 reports framesReplayed=8, although none of the 8 frames had been on any wire.

How it happens. snapshotReplayTarget itself is unchanged; it now gets a different input:

trySendBatch advances nextWireSeq as each frame is appended (CursorWebSocketSendLoop.java:4057-4059), before batchFlush() runs (4073).
When the flush throws, fail → connectWalk → snapshotReplayTarget() (3118-3125) computes fsnAtZero + nextWireSeq - 1. That range now includes frames that never left the send buffer.
swapClient keeps the target (3156) because nothing was acked. Every frame of the failed batch is then counted as replayed when it goes out on the new connection (4062-4066, 4081).

What it breaks.

Base behaviour: a failed sendBinary did not advance nextWireSeq, so those frames were not counted.
Documented contract: the getTotalFramesReplayed() Javadoc says "frames re-sent". The snapshotReplayTarget Javadoc says the target is the highest FSN that "completed a send".
Existing test: a comment in InitialConnectAsyncTest.testAsyncMetricsDistinguishInitialSendFromReconnectReplay says such a frame "has never been sent and must not inflate the replay counter". That test passes today because it never reaches the failed-flush path.
Who sees it: anyone using the metric to judge replay volume or connection flapping, after any disconnect that shows up on write (server restart or failover, RST/EPIPE).

Suggested fix.

On a flush or append failure, roll nextWireSeq back by batchFrames and restore the replayTargetFsn saved before the batch, then call fail(t). swapClient re-anchors the wire state afterwards, so the rollback is safe.
Add the test below as a regression test. It fails at 2732e4a1 and passes at 02ce9b98.
One caveat: after a partial write this treats the batch's leading frames as unsent. Base did the same for a failed sendBinary.

Regression test: replay metric after a failed first send

package io.questdb.client.test.cutlass.qwp.client.sf.cursor;

import io.questdb.client.DefaultHttpClientConfiguration;
import io.questdb.client.cutlass.http.client.WebSocketClient;
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine;
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorWebSocketSendLoop;
import io.questdb.client.network.Socket;
import io.questdb.client.network.SocketReadinessWaiter;
import io.questdb.client.std.MemoryTag;
import io.questdb.client.std.Unsafe;
import io.questdb.client.test.tools.TestWebSocketClient;
import org.junit.Assert;
import org.junit.Test;

import java.lang.reflect.Field;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;

// Same trigger at BASE and HEAD: the first connection's socket reports a peer reset
// (send < 0) on its first send; the reconnect factory hands out a healthy client.
// No ACKs are ever delivered. N frames are published before the loop starts.
public class ProbeReplayMetricTest {
    static final AtomicInteger FAILED_SEND_CALLS = new AtomicInteger();
    static final AtomicInteger GOOD_SEND_CALLS = new AtomicInteger();

    @Test
    public void probe() throws Exception {
        final int n = 8;
        try (CursorSendEngine engine = new CursorSendEngine(null, 1 << 16)) {
            long buf = Unsafe.calloc(16, MemoryTag.NATIVE_DEFAULT);
            try {
                for (int i = 0; i < n; i++) {
                    engine.appendBlocking(buf, 16);
                }
            } finally {
                Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT);
            }
            ProbeClient first = new ProbeClient(true);
            CursorWebSocketSendLoop loop = new CursorWebSocketSendLoop(
                    first, engine, 0L, () -> new ProbeClient(false), 10L, 100L);
            try {
                loop.start();
                long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5);
                while (loop.getTotalFramesSent() < n && System.nanoTime() < deadline) {
                    Thread.sleep(1);
                }
                Thread.sleep(200);
                Assert.assertEquals(n, loop.getTotalFramesSent());
                // none of the n frames reached any wire before the reconnect
                Assert.assertEquals(0, loop.getTotalFramesReplayed());
            } finally {
                loop.close();
            }
        }
    }

    static final class ProbeClient extends TestWebSocketClient {
        ProbeClient(boolean failSends) {
            super(DefaultHttpClientConfiguration.INSTANCE, (nf, log) -> new ScriptSocket(failSends));
            try {
                Field f = WebSocketClient.class.getDeclaredField("upgraded");
                f.setAccessible(true);
                f.setBoolean(this, true);
            } catch (ReflectiveOperationException e) {
                throw new AssertionError(e);
            }
        }

        @Override
        protected void ioWait(int timeout, int op) {
        }

        @Override
        protected void setupIoWait() {
        }
    }

    static final class ScriptSocket implements Socket {
        private final boolean failSends;
        private boolean closed;

        ScriptSocket(boolean failSends) {
            this.failSends = failSends;
        }

        @Override
        public void close() {
            closed = true;
        }

        @Override
        public int getFd() {
            return 0;
        }

        @Override
        public boolean isClosed() {
            return closed;
        }

        @Override
        public void of(int fd) {
        }

        @Override
        public int recv(long bufferPtr, int bufferLen) {
            return 0;
        }

        @Override
        public int send(long bufferPtr, int bufferLen) {
            if (failSends) {
                FAILED_SEND_CALLS.incrementAndGet();
                return -1; // peer disconnect (EPIPE / ECONNRESET)
            }
            GOOD_SEND_CALLS.incrementAndGet();
            return bufferLen;
        }

        @Override
        public void startTlsSession(CharSequence peerName, SocketReadinessWaiter waiter) {
            throw new UnsupportedOperationException();
        }

        @Override
        public boolean supportsTls() {
            return false;
        }

        @Override
        public int tlsIO(int readinessFlags) {
            return 0;
        }

        @Override
        public boolean wantsTlsWrite() {
            return false;
        }
    }
}

2. The new compaction call in `receiveFrame` has no test

Problem: nothing fails if the compactRecvBuffer() call this PR added to receiveFrame is removed.
Net impact: a regression on the egress receive path would ship undetected, and the buffer would grow instead of compacting.
Evidence: at 2732e4a1 with WebSocketClient.java:522 deleted, the full core suite still passes (3797 tests, 0 failures). A receiveFrame version of the burst test (below) passes at head and fails with that line deleted (buffer grows 65536 → 131072).

Why it matters.

The lazy compaction now depends on two pre-recv call sites.
testFrameBurstIsParsedInPlace covers only the tryReceiveFrame one (line 708).
The receiveFrame one has no test, and it is the path egress uses (QwpEgressIoThread.java:328).
If it regressed, a partial frame left within 1 KiB of the buffer end would trigger growRecvBuffer() (doubling, never shrinking) instead of a memmove. With maxRecvBufSize defaulting to Integer.MAX_VALUE, native memory would keep growing over long result streams.
This also contradicts the PR body's "each change has a regression test proven to fail without it".

Suggested fix. Make testFrameBurstIsParsedInPlace cover both receive methods, or add this test to WebSocketClientTest (it reuses FrameFeedWebSocketClient and getIntField):

Test: burst through receiveFrame

    @Test
    public void testFrameBurstViaReceiveFrameIsParsedInPlace() throws Exception {
        assertMemoryLeak(() -> {
            final int frameLen = 13;
            final int frameCount = 5100;
            byte[] data = new byte[frameLen * frameCount];
            for (int i = 0; i < frameCount; i++) {
                data[i * frameLen] = (byte) 0x82;
                data[i * frameLen + 1] = 11;
            }
            try (FrameFeedWebSocketClient client = new FrameFeedWebSocketClient(data)) {
                setUpgradedTrue(client);
                final int bufSize = getIntField(client, "recvBufSize");
                final int[] frames = {0};
                WebSocketFrameHandler handler = new WebSocketFrameHandler() {
                    @Override
                    public void onBinaryMessage(long payloadPtr, int payloadLen) {
                        frames[0]++;
                    }

                    @Override
                    public void onClose(int code, String reason) {
                        Assert.fail("unexpected close frame");
                    }
                };
                for (int i = 0; i < frameCount; i++) {
                    Assert.assertTrue(client.receiveFrame(handler, 1000));
                }
                Assert.assertEquals(frameCount, frames[0]);
                Assert.assertEquals(bufSize, getIntField(client, "recvBufSize"));
            }
        });
    }

Minor

CursorWebSocketSendLoop.java:3343-3344 (ensureSentDictCapacity): the comment says "accumulateSentDict runs AFTER the frame's sendBinary". It now runs after the append and before the flush, so the wording needs updating.

Coverage and tandem

Test gate: passes, with one admitted coverage gap (Moderate 2).
Tandem PRs: none exist in questdb or questdb-enterprise. I don't see that as a blocking gap:
- Wire bytes are unchanged: the same header and masking code produces them.
- The OSS server already consumes every complete frame in each read (QwpIngressUpgradeProcessor.java:802, 2425).
- The reconnect and replay logic is unchanged, and the loopback suites that exercise it pass.
- Optionally, run the parent repo's QWP ingestion e2e tests with -P local-client before bumping the client version there.

Summary

Submodules: no pointers changed.
Severity distribution: 0 Critical, 2 Moderate, 1 Minor.
Where the findings sit: Moderate 2 is in the diff. Moderate 1 is outside it: an unchanged callsite, snapshotReplayTarget, now receives a different input because of the change.

sendRow() had just timestamped that row, so its age is zero and the check could never fire; it cost a clock read per flush for per-row producers (~10% of producer throughput at saturation).

Explicit opt-out only; the default interval is unchanged. With the interval off, row-cap and byte-cap auto-flush and explicit flush() still apply, and the sender reads no clock on the row path at all (measured +21% producer throughput for per-row flushers together with the previous commit).

jerrinot · 2026-11-05T17:18:30Z

Review: PR #121 at level 3 (`perf(qwp): batch WebSocket sends and cut per-frame syscalls`)

Verdict: approve. There are no Critical or Moderate findings, and the test gate passes. One Minor item is below.

Reviewed head f643ffb1 against base 02ce9b98.

What I ran

JDK 8: the full core suite passes at head: 3809 tests, 0 failures, 0 errors, 10 skipped.
JDK 25: test-compile succeeds.
Regression tests: I reverted single production hunks and ran the matching tests at head.
- Restoring the poll before every send in doSend fails testSendPollsForWritabilityOnlyOnBackpressure (expected:<0> but was:<1>).
- Removing the compaction call before recv in receiveFrame fails testFrameBurstIsParsedInPlaceWithBlockingReceive: the buffer grows from 65536 to 131072.
- Deleting the early batch break before an orphaned deferred tail in trySendBatch fails AckWaiterSignalTest.testDrainWakesOnOrphanTailRetirement, a test that uses a real socket. It failed 3 of 3 runs with the break deleted and passed 3 of 3 at head; the full suite with the deletion had exactly that one failure.
Earlier review comments: the three items from the review of 2732e4a1 are fixed. Failed batches no longer count toward the replay metric (CursorWebSocketSendLoopReplayMetricsTest). Compaction in receiveFrame now has its own test. The comments about dictionary-mirror ordering are corrected.

Critical

None.

Moderate

None.

Minor

Problem: three new test methods are out of alphabetical order.
Net impact: cosmetic only; no user impact.
Evidence: read from the source at f643ffb1:
- WebSocketClientTest:427: testSendPollsForWritabilityOnlyOnBackpressure comes before testSendCloseFrameDoesNotClobberSendBuffer (437) and testSendPingDoesNotClobberSendBuffer (466).
- CursorWebSocketSendLoopTornDictGuardTest:94: testGuardFiresOnlyAfterFramesAheadOfTheGapAreFlushed comes before testGuardFiresOnGenuineGapAndShipsNoFrame (130).
- AutoFlushIntervalClockTest:85: testIntervalOffReadsNoClock comes before testIntervalOffKeepsRowCapAndExplicitFlush (102).
Suggested fix: move each method to its sorted position.

Coverage map

Test gate: passes, with 0 admitted coverage gaps.
Local tests: every behaviour change has a test that fails when the change regresses: send-first doSend, the batch wire format, the 64 KiB cap, ending the batch early before special frames, the replay-metric rollback, lazy compaction on both receive paths, the clock-read gates and auto_flush=off on WebSocket.
Tandem PRs: none exist in questdb or questdb-enterprise (searched with gh pr list --head perf/ws-send-first in both repos; the PR body has no links). I'm treating both as accepted gaps:
- OSS e2e: the wire bytes are unchanged; only how frames are grouped into TCP writes changes. The OSS server already handles every complete frame in each read (QwpIngressUpgradeProcessor.processWebSocketFrames, the while (pos < bufferEnd) loop at line 2445 in the local questdb checkout). TCP already merges frames into one read on base under load.
- Enterprise (store-and-forward reconnect and replay): the reconnect, replay, NACK and poison-strike logic is unchanged. A failed append or flush always ends with a new client from swapClient, or with the loop stopped. The loopback and fuzz suites that exercise these paths pass.

Summary

Submodules: no pointers changed, so there was nothing to classify.
Where the findings sit: no admitted findings. All out-of-diff callsites came out safe:
- egress query, credit and cancel sends;
- the dictionary catch-up;
- PING, PONG and close-echo frames;
- the upgrade write;
- the Linux, macOS and Windows ioWait;
- TLS send;
- the builder's config paths.
Severity: 0 Critical, 0 Moderate, 1 Minor.
Tradeoffs:
- ACKs are now read once per batch of up to 64 KiB rather than after every frame.
- One send timeout covers the whole batch. With the default timeout of 600 s, a batch only times out against a peer draining slower than about 110 B/s.
- Neither of these is worse in practice than base sending one large frame under a single timeout.

The branch accepted auto_flush=off / disableAutoFlush() on ws:: along with auto_flush_interval=off, because both store the interval as Integer.MAX_VALUE and the WebSocket validation keyed on that value. Fully disabling auto-flush on WebSocket left auto_flush_bytes in force, so auto_flush=off;auto_flush_bytes=N still flushed every N bytes, contrary to the disableAutoFlush() contract. The builder now records disableAutoFlush() in its own flag, and the WebSocket validation rejects that flag again, as main does. Only the time-based trigger can be turned off on WebSocket, with auto_flush_interval=off; auto_flush_rows=off and auto_flush_bytes=off remain the per-trigger opt-outs. The disableAutoFlush() javadoc and the tests that the branch had flipped are restored, and QuestDBBuilderTest pins both sides: auto_flush=off is rejected, also with auto_flush_bytes, while auto_flush_interval=off is accepted alone and with the row trigger off.

jerrinot · 2026-11-05T19:13:35Z

Reviewed PR #121 at level 3, head a2f14d28 against base 02ce9b98.

Verdict: approve. No Critical or Moderate findings were admitted, and the test gate passes. Two Minor test-code items remain.

Critical

None.

Moderate

None.

Minor

1. New test methods are out of alphabetical order

Problem: in three test files, a new method sits before methods that sort ahead of it.
Net impact: cosmetic only; no user impact.
Evidence: method positions in the source at a2f14d28:
- WebSocketClientTest: testSendPollsForWritabilityOnlyOnBackpressure (line 427) comes before testSendCloseFrameDoesNotClobberSendBuffer (437) and testSendPingDoesNotClobberSendBuffer (466).
- CursorWebSocketSendLoopTornDictGuardTest: testGuardFiresOnlyAfterFramesAheadOfTheGapAreFlushed (94) comes before testGuardFiresOnGenuineGapAndShipsNoFrame (130).
- AutoFlushIntervalClockTest: testIntervalOffReadsNoClock (85) comes before testIntervalOffKeepsRowCapAndExplicitFlush (102).
Fix: move each method to its sorted position. The earlier review comment on f643ffb1 flagged the same three, and they are still unfixed.

2. Unused sendBinary override in the IdleWait test stub

Problem: BlockingDoorbellClient.sendBinary(long, int) is never called.
Net impact: misleading test code; no user impact.
Evidence: CursorWebSocketSendLoopIdleWaitTest.java:519-524 at a2f14d28.
- The send loop calls the client only through sendPing() (CursorWebSocketSendLoop:3105) and the two-slice sendBinary (:3687).
- The two-slice overload calls the five-argument overload, not the two-argument one.
- So the override, commented "The one-frame-per-write path: one frame, one write", describes a path the loop no longer has.
Fix: delete the override and its comment.

Coverage map

Test gate: passes, with 0 admitted coverage gaps.
Local tests: every main behaviour change has a local test that pins it:
- doSend sends before it polls.
- Batched frames go out back to back in one write.
- The batch stops at a fixed 64 KiB cap.
- A batch ends early, before the special frame, at a torn dictionary, a segment boundary or an orphaned tail.
- Failed batches are not counted as sent or replayed.
- The receive buffer is compacted only before the next read, in both receive methods.
- The new clock-read rules.
- auto_flush_interval=off is accepted and auto_flush=off stays rejected on WebSocket.
Tandem PRs: both exist, link to each other and to this PR, and are on the matching branch perf/ws-send-first. Their CI was still pending when I reviewed.
- test(qwp): cover batched WebSocket sends against a real server - OSS tandem questdb#8240 pins the client at a2f14d28. It adds real-server tests for per-row flushing, mixed frame sizes, transactions and TLS under batching, plus a test where the client receives bursts of small frames.
- questdb/questdb-enterprise#1396 pins that OSS head (bd12119f). It adds a SQL failover test that flushes every row, so the demote cuts batched writes short; it checks that every row keeps its own symbol and that the counts match.
- No kill -9 Python e2e test is needed: the store-and-forward on-disk format and crash recovery are unchanged.

Summary

Submodules: no pointers changed. The only submodule, zstd, is untouched.
Severity: 0 Critical, 0 Moderate, 2 Minor, all in the diff.
What I checked:
- Callers of the changed code: egress query, credit and cancel sends, the dictionary catch-up, ping/pong/close frames, the upgrade request, epoll/kqueue/select waits, TLS send, egress decoding in place, and both config parsers. All are safe.
- Store-and-forward rules: after a failed append or flush the loop always reconnects on a new client or stops. No transport error reaches the producer, there is no reconnect time limit, and no frame is dropped, reordered, or acked before it is sent.
- Builds: CI passes the JDK 8 build and tests, the JDK 25/26 compile and javadoc checks, and the macOS run.
Tradeoffs:
- ACKs are now read once per batch of up to 64 KiB instead of after every frame.
- One send timeout covers a whole batch.
- Base already had both limits for any single large frame. The ACKs one batch can produce are far smaller than the socket buffers.

BlockingDoorbellClient overrode the one-slice sendBinary to append and flush a single frame, but the send loop no longer calls that overload: data frames go through batchAppendBinary and batchFlush, and the dictionary catch-up uses the two-slice sendBinary. Drop the dead override and its comment.

mtopolnik · 2026-11-05T19:24:32Z

[PR Coverage check]

😍 pass : 108 / 121 (89.26%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java	67	79	84.81%
🔵	io/questdb/client/cutlass/http/client/WebSocketClient.java	16	17	94.12%
🔵	io/questdb/client/cutlass/http/client/WebSocketSendBuffer.java	11	11	100.00%
🔵	io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java	7	7	100.00%
🔵	io/questdb/client/Sender.java	7	7	100.00%

jerrinot · 2026-11-06T08:39:52Z

I reviewed PR #121 at level 3, the full review. Verdict: approve. I found no Critical or Moderate issues, the test gate passes, and only a few cosmetic items remain in the tests.

PR metadata: the title follows Conventional Commits, and the description explains the user impact and cross-links both tandem PRs. The label "QUEUED FOR MERGE" is already set.

Submodules: no submodule pointer moved. The only submodule is zstd, which is untouched. No binary files are committed.

Critical

None.

Moderate

None.

Minor

1. Three new test methods are out of alphabetical order (raised in earlier review rounds and still not fixed)

Problem: new test methods sit before methods that sort ahead of them.
Net impact: cosmetic only; no user impact.
Evidence: read from the source at 1da8ffd4:
- In WebSocketClientTest, testSendPollsForWritabilityOnlyOnBackpressure comes before testSendCloseFrameDoesNotClobberSendBuffer and testSendPingDoesNotClobberSendBuffer.
- In CursorWebSocketSendLoopTornDictGuardTest, testGuardFiresOnlyAfterFramesAheadOfTheGapAreFlushed comes before testGuardFiresOnGenuineGapAndShipsNoFrame.
- In AutoFlushIntervalClockTest, testIntervalOffReadsNoClock comes before testIntervalOffKeepsRowCapAndExplicitFlush.
Fix: move each method to its sorted position.

2. A new boolean field lacks the is/has prefix

Problem: Sender.LineSenderBuilder.autoFlushDisabled (Sender.java:1124) breaks the QuestDB boolean naming rule.
Net impact: cosmetic only. Nearby builder fields (tlsEnabled, transactional) don't follow the rule either.
Evidence: static, from the changed line at 1da8ffd4.
Fix: rename it to isAutoFlushDisabled.

Coverage map

Test gate: passes, with 0 coverage gaps worth reporting.
Local test run: I ran the 14 test classes this change touches on JDK 8 at 1da8ffd4, with the native library rebuilt from source. All passed.

Mutation spot-checks: I broke each of these lines in a scratch worktree, and each time a named test failed:

Line broken	Test that failed
Batch ends before the orphaned tail	`AckWaiterSignalTest.testDrainWakesOnOrphanTailRetirement`
Batch ends before a gap in the symbol dictionary	`TornDictGuardTest.testGuardFiresOnlyAfterFramesAheadOfTheGapAreFlushed`
Batch ends at the end of a sealed segment	Both tests in `RotationRaceTest`
Receive buffer is compacted before reading (`tryReceiveFrame`)	`WebSocketClientTest.testFrameBurstIsParsedInPlace`
Wire sequence is rolled back after a failed flush	Two tests in `ReplayMetricsTest`

Tandem PRs:
- test(qwp): cover batched WebSocket sends against a real server - OSS tandem questdb#8240 and questdb/questdb-enterprise#1396 are both on the matching branch perf/ws-send-first, link to each other and to this PR, and have green CI. The Enterprise CI includes the end-to-end job on linux-amd64.
- The OSS PR pins a2f14d28. The only change between that and the head is a test-only deletion.
- Neither repo contains a WebSocketClient subclass that overrides sendBinary, so moving data frames to batchAppendBinary/batchFlush doesn't silently bypass any test helper there.
- No kill -9 test is needed, because the store-and-forward on-disk format and crash recovery are unchanged.

Summary

Severity: 0 Critical, 0 Moderate, 2 Minor, all in the diff and none from callers outside it.
Store-and-forward rules hold:
- A failed append or flush always ends in a reconnect on a new client, or the loop stops.
- The ACK watermark cannot move past a frame that was appended but not flushed, because ACKs are only read after the batch has been flushed or the client swapped.
- Frames that were appended but not flushed are replayed from ackedFsn+1.
- The reconnect still has no time limit, and no transport error reaches the producer.
Callers outside the diff: I checked these and all are safe:
- every caller of doSend: the upgrade request, ping/pong/close frames, the TLS send path, and egress credit and cancel sends;
- lazy receive-buffer compaction under egress in-place decoding and the SERVER_INFO handoff;
- the dictionary catch-up's use of the shared send buffer;
- every reader of nextWireSeq and dataFrameSentThisConnection;
- both config parsers.
Tradeoffs:
- ACKs are read once per batch of up to 64 KiB, not once per frame.
- One send timeout now covers a whole batch.
- The dictionary mirror can now include frames that never reached the socket. The new server sees exactly what it already saw at base when a sent but unacknowledged frame was lost on the wire, and the Enterprise failover tandem covers this case.

jerrinot added 4 commits November 4, 2026 13:37

jerrinot added 7 commits November 4, 2026 22:44

perf(qwp): skip the auto-flush age check on the first pending row

8506ed5

sendRow() had just timestamped that row, so its age is zero and the check could never fire; it cost a clock read per flush for per-row producers (~10% of producer throughput at saturation).

docs(qwp): disableAutoFlush() javadoc covers the WebSocket transport

05b3106

fix(qwp): exclude failed send batches from replay metrics

9f7defd

test(ws): cover blocking receive buffer compaction

bd77813

test(qwp): cover batch flushing across recovered segments

d1a01c6

docs(qwp): correct dictionary accumulation ordering comments

f643ffb

jovfer mentioned this pull request Nov 5, 2026

perf(qwp): cut the producer cost of a flush on the WebSocket sender #108

Open

jerrinot added the QUEUED FOR MERGE Approved PR in the merge queue. Do not merge master into this PR. label Nov 5, 2026

jerrinot mentioned this pull request Nov 5, 2026

test(qwp): cover batched WebSocket sends against a real server - OSS tandem questdb/questdb#8240

Open

jerrinot marked this pull request as ready for review November 5, 2026 19:14

Navigation Menu

Repository navigation

perf(qwp): batch WebSocket sends and cut per-frame syscalls - #121

perf(qwp): batch WebSocket sends and cut per-frame syscalls#121
jerrinot wants to merge 13 commits into
mainfrom
perf/ws-send-first

jerrinot commented Nov 4, 2026 •

edited

Loading

Uh oh!

jerrinot commented Nov 4, 2026

Uh oh!

jerrinot commented Nov 5, 2026

Uh oh!

jerrinot commented Nov 5, 2026

Uh oh!

mtopolnik commented Nov 5, 2026

Uh oh!

jerrinot commented Nov 6, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jerrinot commented Nov 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes

Measured

Notes

Uh oh!

jerrinot commented Nov 4, 2026

Review: perf(qwp): batch WebSocket sends and cut per-frame syscalls

Moderate

1. A failed batch flush inflates getTotalFramesReplayed()

2. The new compaction call in receiveFrame has no test

Minor

Coverage and tandem

Summary

Uh oh!

jerrinot commented Nov 5, 2026

Review: PR #121 at level 3 (perf(qwp): batch WebSocket sends and cut per-frame syscalls)

Critical

Moderate

Minor

Coverage map

Summary

Uh oh!

jerrinot commented Nov 5, 2026

Critical

Moderate

Minor

Coverage map

Summary

Uh oh!

mtopolnik commented Nov 5, 2026

[PR Coverage check]

file detail

Uh oh!

jerrinot commented Nov 6, 2026

Critical

Moderate

Minor

Coverage map

Summary

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jerrinot commented Nov 4, 2026 •

edited

Loading

1. A failed batch flush inflates `getTotalFramesReplayed()`

2. The new compaction call in `receiveFrame` has no test

Review: PR #121 at level 3 (`perf(qwp): batch WebSocket sends and cut per-frame syscalls`)