Skip to content

Commit 3314d98

Browse files
glasstigerclaude
andcommitted
Drop redundant symbol-dict catch-up entry index
sendDictCatchUp is the only reader of the sent-dictionary entry-ends index, and it walks entries strictly in order, so the index bought nothing: it added an O(n) rebuild pass per capped reconnect and kept N*4 bytes of native memory alive for the connection's whole life. Replace it with a running pointer that derives each entry's byte span from its own [len varint][utf8] framing in a single pass. Removing the index also deletes rebuildSentDictEntryIndex, whose bare LineSenderException on a malformed mirror unwound into connectLoop as a transient and reconnect-livelocked instead of failing loud. The packing loop's inline bound check now latches a terminal via recordFatal (like the sibling ensureSentDictCapacity), so a corrupt mirror stops the loop cleanly. Add testCorruptCatchUpMirrorLatchesTerminalNotLivelock to pin that terminal, and drop the now-obsolete entry-index assertions from the split catch-up test (renamed to testSplitCatchUpStagesOnlyPrefixAcrossReconnects). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 1699c0a commit 3314d98

2 files changed

Lines changed: 72 additions & 111 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java

Lines changed: 28 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,6 @@ public final class CursorWebSocketSendLoop implements QuietCloseable {
192192
// workload. The guard exists so that pathological growth fails loudly instead
193193
// of overflowing the int capacity math into a heap-corrupting copyMemory.
194194
private static final int MAX_SENT_DICT_BYTES = Integer.MAX_VALUE - 8;
195-
private static final int MAX_SENT_DICT_ENTRIES = MAX_SENT_DICT_BYTES / Integer.BYTES;
196195
/**
197196
* Throttle "reconnect attempt N failed" WARN logs to one per 5 s.
198197
*/
@@ -308,12 +307,6 @@ public final class CursorWebSocketSendLoop implements QuietCloseable {
308307
// Any growth first performs a copy-on-write; only owned buffers are freed.
309308
private boolean sentDictBytesOwned;
310309
private int sentDictCount;
311-
// Relative byte end of every [len varint][utf8] entry in sentDictBytesAddr.
312-
// The mirror is parsed once as it is built/recovered; reconnect catch-up then
313-
// reads fixed-width offsets instead of decoding every length varint again.
314-
private long sentDictEntryEndsAddr;
315-
private int sentDictEntryEndsCapacity;
316-
private int sentDictIndexedCount;
317310
// True when replay frames can start above dictionary id zero and therefore
318311
// depend on a catch-up on a fresh connection. Delta-enabled live engines
319312
// always have this dependency. A recovered delta slot whose dictionary
@@ -326,7 +319,6 @@ public final class CursorWebSocketSendLoop implements QuietCloseable {
326319
// second WebSocket payload slice, avoiding a full-dictionary staging copy.
327320
private long catchUpFrameAddr;
328321
private int catchUpFrameCapacity;
329-
private int catchUpEntryIndexBuildCount;
330322
private int catchUpFrameGrowthCount;
331323
// Orphan-policy cap-gap attempts -- catch-ups that reached a node and found an entry
332324
// too large for its batch cap -- with no intervening successful catch-up or unrelated
@@ -2484,15 +2476,13 @@ private void accumulateSentDict(long payloadAddr, int payloadLen, int deltaStart
24842476
if (deltaCount <= 0 || deltaStart > sentDictCount || deltaEnd <= sentDictCount) {
24852477
return;
24862478
}
2487-
// Deliberately does NOT touch the entry-ends index. Only sendDictCatchUp reads
2488-
// it, so maintaining it here would put a store per new symbol on the per-frame
2489-
// I/O path -- and on the workload this feature targets (a new symbol per ROW)
2490-
// that is a store per row, plus 4 bytes per symbol retained for the connection's
2491-
// whole life, to serve a reconnect that may never happen. sendDictCatchUp calls
2492-
// ensureSentDictEntryIndex(), which notices sentDictIndexedCount has fallen
2493-
// behind sentDictCount and rebuilds; that O(n) walk runs at most once per
2494-
// reconnect and is dwarfed by shipping the same n bytes over the wire. A
2495-
// connection that never reconnects now never allocates the index at all.
2479+
// Keeps no per-entry offset index: the mirror stores each entry as
2480+
// [len varint][utf8], so the reconnect catch-up (sendDictCatchUp) walks it with
2481+
// a running pointer on demand. Maintaining an index here would put a store per
2482+
// new symbol on the per-frame I/O path -- and on the workload this feature
2483+
// targets (a new symbol per ROW) that is a store per row -- to serve a reconnect
2484+
// that may never happen; the catch-up's single walk is dwarfed by shipping the
2485+
// same n bytes over the wire.
24962486
//
24972487
// Walk past the already-held prefix [deltaStart, sentDictCount), then copy
24982488
// the new tail [sentDictCount, deltaEnd).
@@ -2507,7 +2497,7 @@ private void accumulateSentDict(long payloadAddr, int payloadLen, int deltaStart
25072497
long regionStart = p;
25082498
long newCount = deltaEnd - sentDictCount;
25092499
// Walk the new tail only to find where it ends; the entry boundaries are not
2510-
// recorded here (see above -- the catch-up rebuilds them on demand).
2500+
// recorded here (see above -- the catch-up re-walks the mirror when it needs them).
25112501
for (long i = 0; i < newCount; i++) {
25122502
long len = readVarintAt(p, limit);
25132503
p = varintEnd + len;
@@ -2574,76 +2564,11 @@ private void releaseSentDictBytes() {
25742564
if (sentDictBytesAddr != 0 && sentDictBytesOwned) {
25752565
Unsafe.free(sentDictBytesAddr, sentDictBytesCapacity, MemoryTag.NATIVE_DEFAULT);
25762566
}
2577-
if (sentDictEntryEndsAddr != 0L) {
2578-
Unsafe.free(
2579-
sentDictEntryEndsAddr,
2580-
sentDictEntryEndsCapacity * Integer.BYTES,
2581-
MemoryTag.NATIVE_DEFAULT);
2582-
}
25832567
sentDictBytesAddr = 0;
25842568
sentDictBytesCapacity = 0;
25852569
sentDictBytesLen = 0;
25862570
sentDictBytesOwned = false;
25872571
sentDictCount = 0;
2588-
sentDictEntryEndsAddr = 0L;
2589-
sentDictEntryEndsCapacity = 0;
2590-
sentDictIndexedCount = 0;
2591-
}
2592-
2593-
private void ensureSentDictEntryEndsCapacity(long required) {
2594-
if (required <= sentDictEntryEndsCapacity) {
2595-
return;
2596-
}
2597-
if (required > MAX_SENT_DICT_ENTRIES) {
2598-
LineSenderException err = new LineSenderException(
2599-
"symbol dictionary entry index exceeds the maximum size [required="
2600-
+ required + ", max=" + MAX_SENT_DICT_ENTRIES + ']');
2601-
recordFatal(err);
2602-
throw err;
2603-
}
2604-
long newCapacity = Math.max(
2605-
(long) sentDictEntryEndsCapacity * 2L,
2606-
Math.max(1024L, required));
2607-
if (newCapacity > MAX_SENT_DICT_ENTRIES) {
2608-
newCapacity = MAX_SENT_DICT_ENTRIES;
2609-
}
2610-
sentDictEntryEndsAddr = Unsafe.realloc(
2611-
sentDictEntryEndsAddr,
2612-
sentDictEntryEndsCapacity * Integer.BYTES,
2613-
(int) newCapacity * Integer.BYTES,
2614-
MemoryTag.NATIVE_DEFAULT);
2615-
sentDictEntryEndsCapacity = (int) newCapacity;
2616-
}
2617-
2618-
private void ensureSentDictEntryIndex() {
2619-
if (sentDictIndexedCount != sentDictCount) {
2620-
rebuildSentDictEntryIndex();
2621-
}
2622-
}
2623-
2624-
private void rebuildSentDictEntryIndex() {
2625-
ensureSentDictEntryEndsCapacity(sentDictCount);
2626-
long p = sentDictBytesAddr;
2627-
long limit = sentDictBytesAddr + sentDictBytesLen;
2628-
for (int i = 0; i < sentDictCount; i++) {
2629-
long len = readVarintAt(p, limit);
2630-
p = varintEnd + len;
2631-
if (p > limit) {
2632-
throw new LineSenderException(
2633-
"invalid symbol dictionary mirror while building the entry index [entry="
2634-
+ i + ", count=" + sentDictCount + ']');
2635-
}
2636-
Unsafe.getUnsafe().putInt(
2637-
sentDictEntryEndsAddr + (long) i * Integer.BYTES,
2638-
(int) (p - sentDictBytesAddr));
2639-
}
2640-
if (p != limit) {
2641-
throw new LineSenderException(
2642-
"invalid symbol dictionary mirror length while building the entry index [indexed="
2643-
+ (p - sentDictBytesAddr) + ", length=" + sentDictBytesLen + ']');
2644-
}
2645-
sentDictIndexedCount = sentDictCount;
2646-
catchUpEntryIndexBuildCount++;
26472572
}
26482573

26492574
private long readVarintAt(long p, long limit) {
@@ -2693,7 +2618,6 @@ private int sendDictCatchUp() {
26932618
resetCatchUpCapGapEpisode();
26942619
return 1;
26952620
}
2696-
ensureSentDictEntryIndex();
26972621
// The frame ceiling a catch-up chunk must not exceed: the server's
26982622
// advertised cap, or -- when the server advertises none (cap <= 0) --
26992623
// MAX_SENT_DICT_BYTES so sendCatchUpChunk's int frameLen (HEADER_SIZE +
@@ -2717,15 +2641,27 @@ private int sendDictCatchUp() {
27172641
long chunkStartAddr = sentDictBytesAddr;
27182642
int chunkSymbols = 0;
27192643
long chunkBytes = 0;
2644+
// Walk the mirror once with a running pointer, deriving each entry's byte span
2645+
// from its own [len varint][utf8] framing -- no separate offset index is kept
2646+
// (see accumulateSentDict). A mirror built from CRC-validated frames is always
2647+
// well-formed, so entryEnd can exceed the limit only under memory corruption;
2648+
// latch a terminal via recordFatal rather than let a bare throw unwind into
2649+
// connectLoop as a transient and reconnect-livelock.
2650+
long entryPtr = sentDictBytesAddr;
2651+
long mirrorLimit = sentDictBytesAddr + sentDictBytesLen;
27202652
for (int entryId = 0; entryId < sentDictCount; entryId++) {
2721-
int entryStartOffset = entryId == 0
2722-
? 0
2723-
: Unsafe.getUnsafe().getInt(
2724-
sentDictEntryEndsAddr + (long) (entryId - 1) * Integer.BYTES);
2725-
int entryEndOffset = Unsafe.getUnsafe().getInt(
2726-
sentDictEntryEndsAddr + (long) entryId * Integer.BYTES);
2727-
long entryStart = sentDictBytesAddr + entryStartOffset;
2728-
long entryBytes = entryEndOffset - entryStartOffset;
2653+
long entryStart = entryPtr;
2654+
long entryLen = readVarintAt(entryPtr, mirrorLimit);
2655+
long entryEnd = varintEnd + entryLen;
2656+
if (entryEnd > mirrorLimit) {
2657+
LineSenderException err = new LineSenderException(
2658+
"invalid symbol dictionary mirror during catch-up [entry="
2659+
+ entryId + ", count=" + sentDictCount + ']');
2660+
recordFatal(err);
2661+
throw new CatchUpSendException(err);
2662+
}
2663+
entryPtr = entryEnd;
2664+
long entryBytes = entryEnd - entryStart;
27292665
// The exact table-less frame sendCatchUpChunk would build for THIS entry
27302666
// alone: header + deltaStart varint (the entry's own global id) +
27312667
// deltaCount varint (1) + the entry bytes. A cap gap exists only when even
@@ -2895,11 +2831,6 @@ private void sendCatchUpChunk(int deltaStart, int deltaCount, long symbolsAddr,
28952831
totalFramesSent.incrementAndGet();
28962832
}
28972833

2898-
@TestOnly
2899-
public int catchUpEntryIndexBuildCount() {
2900-
return catchUpEntryIndexBuildCount;
2901-
}
2902-
29032834
@TestOnly
29042835
public int catchUpFrameGrowthCount() {
29052836
return catchUpFrameGrowthCount;

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopCatchUpAlignmentTest.java

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -160,19 +160,13 @@ public void testSplitCatchUpFramesAcksDoNotAdvanceTrimWatermark() throws Excepti
160160
}
161161

162162
@Test
163-
public void testSplitCatchUpReusesEntryIndexAndStagesOnlyPrefixAcrossReconnects() throws Exception {
163+
public void testSplitCatchUpStagesOnlyPrefixAcrossReconnects() throws Exception {
164164
TestUtils.assertMemoryLeak(() -> {
165165
CatchUpCapturingClient client = new CatchUpCapturingClient(3_100);
166166
try (CursorSendEngine engine = newEngine()) {
167167
CursorWebSocketSendLoop loop = newLoop(engine, client);
168168
try {
169169
seedMirror(loop, TestUtils.repeat("x", 3_000), TestUtils.repeat("y", 3_000));
170-
// Building the mirror must NOT index it: the index serves only the
171-
// catch-up, so paying for it on the per-frame send path -- and
172-
// retaining it -- would tax every connection for a reconnect that
173-
// may never come.
174-
assertEquals("accumulating the mirror must not build the entry index",
175-
0, loop.catchUpEntryIndexBuildCount());
176170

177171
invokeSetWireBaselineWithCatchUp(loop, 0L);
178172
assertEquals("the small cap must split the dictionary", 2, client.framesSent);
@@ -186,15 +180,11 @@ public void testSplitCatchUpReusesEntryIndexAndStagesOnlyPrefixAcrossReconnects(
186180
assertEquals("the larger cap must combine the dictionary", 3, client.framesSent);
187181
assertEquals("combining symbols must not grow the prefix-only buffer",
188182
1, loop.catchUpFrameGrowthCount());
189-
assertEquals("reconnect must reuse cached entry ends instead of reparsing",
190-
1, loop.catchUpEntryIndexBuildCount());
191183

192184
invokeSetWireBaselineWithCatchUp(loop, 0L);
193185
assertEquals("the next reconnect sends one combined frame", 4, client.framesSent);
194186
assertEquals("the prefix buffer must be reused across reconnects",
195187
1, loop.catchUpFrameGrowthCount());
196-
assertEquals("later reconnects must still reuse the entry index",
197-
1, loop.catchUpEntryIndexBuildCount());
198188
} finally {
199189
// assertMemoryLeak verifies that close releases the retained buffer.
200190
loop.close();
@@ -303,6 +293,47 @@ public void testCatchUpChunkFrameSizeOverflowFailsLoud() throws Exception {
303293
});
304294
}
305295

296+
@Test
297+
public void testCorruptCatchUpMirrorLatchesTerminalNotLivelock() throws Exception {
298+
// A mirror whose [len varint][utf8] framing disagrees with sentDictCount can
299+
// only arise from memory corruption -- it is built from CRC-validated frames.
300+
// The split catch-up's running-pointer walk must latch a terminal (recordFatal,
301+
// surfaced by checkError) rather than let a bare throw unwind into connectLoop
302+
// and reconnect-livelock. A non-zero cap forces the packing loop that walks it.
303+
TestUtils.assertMemoryLeak(() -> {
304+
CatchUpCapturingClient client = new CatchUpCapturingClient(64);
305+
try (CursorSendEngine engine = newEngine()) {
306+
CursorWebSocketSendLoop loop = newLoop(engine, client);
307+
try {
308+
// Entry 0 = [len=1]['a'] (2 bytes); entry 1 = [len=100] with no bytes
309+
// following (a truncated tail), so the second entry's end runs past the
310+
// buffer while sentDictCount = 2 claims both. loop.close() frees addr.
311+
long addr = Unsafe.malloc(3, MemoryTag.NATIVE_DEFAULT);
312+
long p = writeVarint(addr, 1);
313+
Unsafe.getUnsafe().putByte(p, (byte) 'a');
314+
writeVarint(addr + 2, 100);
315+
loop.seedSentDictMirrorForTest(addr, 3, 2);
316+
317+
try {
318+
invokeSetWireBaselineWithCatchUp(loop, 0L);
319+
fail("a corrupt mirror must raise CatchUpSendException");
320+
} catch (RuntimeException e) {
321+
assertEquals("CatchUpSendException", e.getClass().getSimpleName());
322+
}
323+
try {
324+
loop.checkError();
325+
fail("a corrupt catch-up mirror must latch a terminal, not livelock");
326+
} catch (LineSenderException terminal) {
327+
assertTrue("terminal must name the corrupt mirror: " + terminal.getMessage(),
328+
terminal.getMessage().contains("invalid symbol dictionary mirror during catch-up"));
329+
}
330+
} finally {
331+
loop.close();
332+
}
333+
}
334+
});
335+
}
336+
306337
@Test
307338
public void testForegroundCatchUpCapGapRetriesPastOrphanBudget() throws Exception {
308339
// The foreground policy must never accrue or exhaust the orphan drainer's
@@ -919,9 +950,8 @@ private static void seedMirror(CursorWebSocketSendLoop loop, String... symbols)
919950
}
920951
}
921952
loop.seedSentDictMirrorForTest(addr, total, symbols.length);
922-
// Leave sentDictIndexedCount at 0: the mirror carries its own entry lengths, and
923-
// the entry-ends index is built lazily by the catch-up. Pre-building it here
924-
// would hide that from every test that seeds this way.
953+
// The mirror carries its own entry lengths ([len varint][utf8]); the catch-up
954+
// walks them with a running pointer on demand, so nothing else to seed here.
925955
}
926956

927957
private static int varintSize(long value) {

0 commit comments

Comments
 (0)