Repository navigation
Conversation
A QWP sender burned ~70% of a core per connection even when idle: the I/O thread polled the socket and the segment ring every 50 us. Kafka Connect users saw 30% CPU on QWP vs 3% on HTTP for the same quiet sink. The I/O thread now blocks until there is something to do: socket readable, a producer published a frame, close(), or a PING is due. The producer wakes it through a native doorbell (eventfd on Linux, kqueue EVFILT_USER on macOS/FreeBSD, a kernel Event on Windows), and only when the thread is parked, so a busy connection never pays a syscall per flush. Ack waiters, the segment manager and the dispatchers are event-driven as well: a waiter in awaitAckedFsn/drain is unparked on every ack advance, terminal and durability latch; the segment manager parks until a rotation, ACK or timer needs it and persists the ack watermark on a 1 ms cadence while ACKs stream; the progress dispatcher thread starts only when a handler is installed. No change to the Sender API or configuration. Idle CPU drops to 0, flush-to-ack latency falls 3-15x and saturation throughput rises ~20%. Above ~25k small flushes per second per connection the client uses 1.1-1.45x the CPU of the polling loop: the I/O thread takes two prompt wakes per flush (doorbell, then ACK). design/tla holds TLA+ models of the two wake protocols.
… in column lookup ensureCapacity() carried the realloc in the same body, which was large enough that the JIT would not inline it into every putLong/putDouble. The check is now one compare against a precomputed limit address with the realloc in grow(). lookupColumn() first compares the column name by identity, which callers passing String constants hit every row. asipc, mock server, 60M rows, 1000 rows/flush, 3 interleaved reps: instructions/row -26..-68, rows/s +1..2% across numeric/symbol/string.
Only recovery reads a frame's CRC, and a memory-backed segment is never
recovered, yet every frame paid a CRC pass over all its bytes: 13% of
the producer's cycles at one row per flush. The CRC field stays, zeroed,
so both backings keep one frame layout; disk segments are unchanged
(inverting the condition fails the recovery tests).
Measured on asipc against the batching mock, producer and I/O thread
pinned to CPUs 2 and 4, 5 interleaved reps, medians:
1 row/flush: rows/s +7.2%, producer instructions/row -4.4%
1000-row numeric: rows/s +2.5% against the baseline's fast mode
(+22.9% against its median: the baseline runs are
bimodal, 28.0-28.8M or 34.6M; this one is steady
at 34.8-36.4M), instructions/row -3.2%
1000-row symbol: +4.3%; 1000-row string: +3.9%
Encode on a local cursor and leave for the general loop at the first non-ASCII char. Writing the appendAddress field per char, interleaved with Unsafe stores, kept the JIT from holding the address in a register. String column (80M rows, 1000 rows/flush, pinned): +8.6% rows/s, producer instructions -9.0%. Other shapes unchanged.
Vect.memcpy copies up to 64 bytes with 8-byte moves instead of the Unsafe.copyMemory stub, and NativeBufferWriter.putBlockOfBytes goes through it. At one row per flush each column is a few bytes, and the stub's call and dispatch cost more than the copy: 7.3% of the producer. One row per flush, pinned: +8.3% rows/s, producer instructions/row -7.5%. 1000-row shapes unchanged.
Contributor
Author
Review (level 3): approve with commentsReviewed HEAD Nothing blocks the merge. Two Moderate findings have runtime evidence on HEAD and base; please address #1 before merging. CriticalNone. Moderate1. A failed rotation followed by close leaks one segment from the pool's shared budget
Path:
Base, same trigger: the spare stayed installed as Limits:
Suggested fix: make Repro output (scratch test: disk mode, 64 KiB segments, shared budget, manifest fsync made to fail)2. With
|
| Setup | Base | HEAD |
|---|---|---|
| tmpfs, unpaced | 3009–3042 MB/s, rotation 24 µs | 2192–2443 MB/s, rotation 338–496 µs |
| tmpfs, 100 MB/s | 0.63 ms/s in rotations | 11.7 ms/s |
| tmpfs, 300 MB/s | 1.8 ms/s | 33.7 ms/s |
| ext4/NVMe | — | equal or better: no spare-wait stalls, lower max latency, about 1.75× unpaced throughput |
- Magnitude: one stall per 4 MiB of encoded data, about 1–3% of producer time at 100–300 MB/s. The stall lands on whichever
flush()triggers the rotation. - Base: the manager thread pre-created a hot spare, so the producer only rebased it and updated the manifest. Nothing rejects tmpfs for
sf_dir. - Optional fix: keep synchronous creation as the fallback, but have the manager pre-create a spare off the producer thread once the active segment is about ¾ full. That keeps the ext4 gain and takes the tmpfs
fallocateout offlush(). - Description: the PR description doesn't mention this design change. Please add it there: the hot spare is gone, segments are created on the producer thread, and the ack watermark is now written by the I/O thread on every ACK.
Minor
None.
Coverage
- Test gate: passes. There is no Critical coverage gap.
- Moderate gap, no tandem: the change touches store-and-forward behaviour (idle wait, reconnect replay, ack waiters), but there is no
questdborquestdb-enterprisePR onperf/qwp-idle-cpu-low-risk(searched withgh pr list --head).- The local tests and the TLA+ models cover the wake protocol.
- A real-server failover/reconnect run against the event-driven loop is missing.
- Please run the Enterprise failover suite against this branch before bumping the submodule.
- CI: GitHub Actions, the macOS Azure leg and the coverage check are green. The Windows and linux-x64 Azure legs were still pending when I checked; the Windows
WSAEventSelectwait code only runs there.
Summary
- Verdict: approve with comments. Please address ci(build): set file encoding to utf-8 for logging #1.
- Findings: 0 Critical, 2 Moderate, 0 Minor. Both are in the diff itself.
- Tradeoff: idle CPU drops to zero and disk-mode throughput on ext4/NVMe goes up. In exchange, rotations are slower when
sf_diris on tmpfs (chore(conf): update JDK version from 17 to 11 and change tag to 1.0.0 #2). - Checked, no defect found:
- the I/O loop's park/publish handshake (
ioParkedagainstpublishStamp) - the doorbell's lifecycle against close
recvDrainedand TLS buffered inputAckWaiters- concurrent writes and syncs of the ack watermark (a torn record only causes duplicates)
- manager wakeups covering every trim case
- all
Vect.memcpycallers (none depends on overlap-safe copies at 64 bytes or less) OffHeapAppendMemory's capacity limit staying consistent- readers of the frame CRC that memory segments now leave at 0
- the store-and-forward invariants: no reconnect time budget, and no transport errors surfaced to the producer
- the I/O loop's park/publish handshake (
- The perf(qwp): event-driven I/O loop, zero CPU when idle #114 review items are fixed on this branch.
A rotation whose manifest update throws hands its new segment back to the SegmentProvisioner, still charged to the SegmentBudget, so a retry can reuse it. SegmentManager.deregister released only SegmentRing.totalSegmentBytes(), which did not count that segment, and SegmentProvisioner.close() closed it without a release. A pooled sender closed after such a flush failure therefore left one segment charged to the pool's shared budget for the life of the pool. SegmentSource gains heldBytes(), which the provisioner reports for its retained segment, and totalSegmentBytes() includes it. Deregister now releases the whole ring, as it did for the hot spare before. Close still does not release, so the charge is returned exactly once. SegmentManagerSharedBudgetTest injects a manifest fsync failure and checks that deregister returns the budget to zero, and that repeated failed rotations and the retry that finally commits charge one segment only.
Contributor
[PR Coverage check]😍 pass : 394 / 435 (90.57%) file detail
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A QWP sender burns ~70% of a core per connection even when idle: the I/O thread polls the socket and the segment ring every 50 µs.
Change
close()runs, or a PING is due. Producers ring a native doorbell (eventfd on Linux, kqueueEVFILT_USERon macOS/FreeBSD, a kernel Event on Windows) only when the thread is parked. Ack waiters, the segment manager and the dispatchers are event-driven too.OffHeapAppendMemory; identity check first in column lookupOffHeapAppendMemory.putUtf8Vect.memcpy)No change to the
SenderAPI or configuration.Numbers
main.putUtf8), 1 row/flush +8.3% (small copies) and +7.2% (CRC skip). They have not been re-measured together on this branch.Testing
mvn -pl core test: 3748 pass on JDK 25; JDK 8 test-compile clean.putUtf8againstString.getBytes(UTF_8);Vect.memcpyover every length 0–160 at every source and destination alignment.