perf(vector,fileservice): coalesce checksummed ReadAt + batch UnionBatch varlena materialization - #25013
Merged
Merged
Conversation
The on-disk FileWithChecksum format interleaves a 4-byte CRC32 before every 2044-byte content block, so the old ReadAt issued one tiny pread per block — ~12k syscalls for a 24MB range. Pull up to 128 KiB of contiguous blocks per underlying ReadAt into a pooled scratch buffer, then verify CRCs and de-interleave the payloads in memory. 128 KiB is the measured knee (NVMe MDTS-aligned); throughput is flat from ~64 KiB to 2 MiB. No on-disk format change; WriteAt unchanged. +40% QPS on cold/cache-miss vector-index reads; ~1.6x on the isolated read benchmark. Adds TestFileWithChecksumReadAtCoalesce (random reads across block sizes, multi-chunk, past-EOF) and BenchmarkFileWithChecksumReadAt (coalesced vs per-block). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The columnar scan/union hot paths materialized varlena (vector/string) columns one row at a time via BuildVarlenaFromVarlena, which a CPU profile showed to be ~50% of a full table scan (runtime.memmove + incremental mpool realloc churn). Three optimizations, all gated to safe cases and verified equivalent to the per-row path: 1. UnionBatch full-append fast path (offset==0, cnt==w.length, no nulls/grouping — the block-scan materialization case): replace the per-row loop with two big memmoves (whole source area + whole header array) plus an unsafe offset rebase of the non-inline headers. ~3x on f32 full table scans. 2. Area pre-grow (pregrowVarlenaArea): unionT and UnionBatch's general null/flag branches now reserve v.area capacity once (one mpool realloc, length preserved) instead of growing per row. ~1.9x on the per-row union path. 3. Const-broadcast doubling fill (fillSlice / broadcastFixed): UnionMulti, unionT, UnionBatch const branches and appendMultiFixed now broadcast a value across a batch with O(log n) memmoves instead of n scalar stores. ~2.1x on the fill op. mpool ownership (Grow/Grow2, offHeap) and null/grouping bookkeeping are preserved. Adds equivalence + microbenchmark tests; full vector suite passes with -race. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g rows The fast path previously required no nulls and no grouping, so nullable varlena columns fell back to the per-row path. Drop that restriction: the two memmoves are null-agnostic (a null row's content isn't in w.area, and its header is never read), so after copying the area + headers and rebasing offsets, just propagate w's null and grouping bitmaps (shifted by oldLen) via Foreach and zero the null rows' copied headers so no rebased big-header offset lingers as a dangling reference into v.area. Now nullable varlena materialization gets the same ~3x as the no-null case. Adds TestUnionBatchNullFastPath cross-checking values + nsp + gsp against per-row UnionOne (non-empty target, grouping bits, all-null edge); suite passes with -race. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The UnionBatch full-append varlena fast path (from the null/grouping extension)
propagated w's null/grouping bitmaps with w.nsp.Foreach / w.gsp.Foreach, which
walk EVERY set bit in the underlying bitmap with no upper bound. The per-row
path it replaces only consults [0,cnt) via Contains, so the two diverge whenever
w carries stale bits at index >= w.length — a normal reused state, since
SetLength shrinks v.length without clearing nsp/gsp and vectors are pooled.
For a stale nsp bit i >= cnt, `vCol[oldLen+int(i)] = types.Varlena{}` indexes the
oldLen+cnt-length slice out of range -> panic. For a stale gsp bit it sets a
phantom grouping bit at oldLen+i beyond the appended range. (The predecessor
commit was safe: its fast path was gated on nsp/gsp EmptyByFlag, so any stray
bit fell through to the per-row path.)
Fix: skip indices >= cnt in both Foreach callbacks, matching the per-row path.
Keeps the two-memmove fast path; only bitmap propagation changes.
Found by an adversarial self-review-to-break pass; the shipped
TestUnionBatchNullFastPath only sets bits within length so it missed this.
Adds TestUnionBatchFastPathStaleBitmapBits (stale nsp/gsp bits past SetLength,
cross-checked vs per-row UnionOne + phantom-bit assertions) — it panics on the
unbounded code and passes with the bound.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
requested changes
Jun 16, 2026
XuPeng-SH
left a comment
XuPeng-SH
left a comment
Contributor
There was a problem hiding this comment.
I found a substantive issue in the new varlena pre-grow logic.
In pkg/container/vector/vector.go, the new pregrowVarlenaArea sizing loops inspect wCol[...] before checking whether the source row is null. For varlen vectors, null appends do not overwrite the slot, and reused vectors can retain stale non-inline headers in null positions. That means a null-heavy append can reserve area based on dead old payload, triggering large unnecessary mp.Grow calls or allocation failures on inputs that the actual union path would mostly skip.
Please exclude null rows from the pre-grow total and only inspect headers for rows that will actually be copied.
fengttt
approved these changes
Jun 16, 2026
XuPeng-SH
approved these changes
Jun 17, 2026
XuPeng-SH
left a comment
XuPeng-SH
left a comment
Contributor
There was a problem hiding this comment.
🎯 Multi-Angle Review Summary
Comprehensive review completed across 6 dimensions: logic correctness, concurrency safety, error handling, test coverage, unhappy paths, and performance verification.
✅ Overall Assessment: APPROVE
Strengths:
- 🎯 Critical bug already fixed: Stale bitmap bits issue (commit
7a3d8352) - 📝 Exceptional test coverage: 485 new test lines (1.6:1 test:code ratio)
- 🔍 Explicit regression guards:
TestUnionBatchFastPathStaleBitmapBits+TestUnionPregrowSkipsNullRows - ⚡ Measured perf gains: syscall reduction (~60×), batch speedup (~3×)
- 🛡️ Equivalence oracles: All fast paths cross-checked vs known-correct
UnionOne
🔴 Critical Issues (Already Fixed)
Issue #1: Stale Bitmap Bits Panic [FIXED in 7a3d8352]
Root cause: Vector.SetLength(n) only shrinks length, does not clear nsp/gsp bitmaps. Reused vectors carry stale bits at index >= length.
Impact:
nspstale bit → panic onvCol[oldLen+i]out-of-boundsgspstale bit → phantom grouping bit leaks
Fix applied:
w.nsp.Foreach(func(i uint64) bool {
if i < ucnt { // ✅ Bound to [0,cnt)
nulls.Add(&v.nsp, base+i)
vCol[oldLen+int(i)] = types.Varlena{}
}
return true
})✅ Verified correct — matches per-row UnionOne semantics. Excellent self-review by author.
⚠️ Non-Blocking Suggestions
1. Document Vector.SetLength stale state semantics
Current code assumes reuse behavior but lacks doc comment. Suggest adding:
// SetLength shrinks or grows the logical length without clearing
// nsp, gsp, or data/area past the new length. Reused vectors may
// carry stale state at index >= length; consumers MUST NOT assume
// bits or data beyond [0,length) are initialized.2. Document UnionBatch concurrency safety
The unsafe pointer walk is not atomic. Suggest clarifying:
// UnionBatch is NOT safe for concurrent use. Caller must serialize
// all calls that mutate or read v during this operation.3. Clarify _ReadCoalesceSize rationale
Comment claims "matches NVMe MDTS" but typical MDTS is 512KB–2MB. Suggest:
// 128 KiB is the empirical knee: measured syscall reduction saturates
// here (~1.6× throughput) while keeping pooled scratch small. Larger
// sizes (tested up to 2 MiB) show <6% additional gain.🧪 Test Coverage Analysis
Unhappy paths tested:
- ✅ Stale bitmap bits (
>= length) → regression guard - ✅ Null rows with stale non-inline headers → pregrow skip
- ✅ EOF reads past file end → 3000 random windows
- ✅ Block size > pool buffer → one-off alloc fallback
- ✅ Mixed inline/non-inline varlenas →
baseOff != 0rebase
Cross-validation:
- All fast paths checked against
UnionOne(equivalence oracle) - Benchmarks quantify old vs new (coalesced: perblock, doubling: scalar)
Metrics:
- 485 new test lines (377 vector + 108 fileservice)
- ~300 production lines changed
- 1.6:1 test:code ratio 🟢 Excellent
🚀 Performance Verification
| Optimization | Claim | Verified |
|---|---|---|
| ReadAt syscall reduction | ~12k → ~190 for 24MB | ✅ Math checks out (24MB/128KB = 192) |
| Varlena full-append | ~3× f32 table scan | ✅ Benchmark included |
| Const-broadcast fill | ~2.1× speedup | ✅ BenchmarkConstBroadcastFill |
| Cold vector index QPS | +40% | ℹ️ Claim credible (syscall overhead reduced) |
🔍 Unhappy Path Deep Dive
Edge case #1: Stale bits → FIXED + regression test
Edge case #2: Null pregrow over-reserve → Skips null rows correctly + test
Edge case #3: EOF at block boundary → Returns (n, io.EOF) per io.ReaderAt contract
Edge case #4: Large block size → Graceful fallback to one-off alloc
Edge case #5: Concurrent union → Not documented (suggest doc comment)
📊 Code Quality
- No breaking changes: FileWithChecksum semantics preserved
- No on-disk format change: Transparent optimization
- Mpool semantics preserved: Pregrow uses
mp.Grow2to stay mpool-tracked - Type safety: Generics + unsafe confined to offset rebase (well-scoped)
🎯 Verdict
APPROVE — This is a model PR for performance optimization:
- Self-identified bug fixed before review
- Comprehensive test coverage including regression guards
- Equivalence oracles prove correctness
- Measurable, significant performance gains
- All unhappy paths tested
The only issues are documentation suggestions (non-blocking).
Recommendation to author: Consider the 3 doc comment additions above to help future maintainers understand the reuse contract and concurrency constraints.
Reviewed dimensions: logic, concurrency, errors, tests, unhappy paths, performance
Test coverage: 485 lines | Test:Code ratio: 1.6:1 | Regression guards: 2
Contributor
Merge Queue Status
This pull request spent 2 hours 16 minutes 17 seconds in the queue, including 1 hour 12 minutes 12 seconds running CI. Required conditions to merge
|
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
4 participants
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.
What type of PR is this?
Which issue(s) this PR fixes:
issue #25012
What this PR does / why we need it:
Brings three perf optimizations to the vector and fileservice hot paths, plus a correctness fix found while reviewing them.
11a537d8perf(fileservice)FileWithChecksum.ReadAtcoalesces up to 128 KiB of contiguous blocks per underlying read (pooled scratch), then verifiesWriteAtuntouched.6b0bfef9perf(vector)UnionBatchfull-append fast path (twomemmoves + unsafe offset rebase),pregrowVarlenaArea,fillSlice/broadcastFixed).171c9354perf(vector)7a3d8352fix(vector)[0,cnt)— stalensp/gspbits ≥w.lengthcaused an out-of-range panic / phantomAnd the perf-numbers table, if you want it too:
ReadAt(24 MB range)UnionBatchfull-append