Skip to content

fix(storage): don't zero CSR header and length-chunk buffers - #1114

Merged
adsharma merged 1 commit into
LadybugDB:mainfrom
zahariash:fix/csr-header-zeroed-per-scan-thread
Oct 5, 2026
Merged

adsharma merged 1 commit into
LadybugDB:mainfrom
zahariash:fix/csr-header-zeroed-per-scan-thread

Conversation

@zahariash

Copy link
Copy Markdown
Contributor

This work was produced with the help of language models.

Fixes #1110.

Every ScanRelTable worker zeroed a 2 MiB CSR header before it looked for a morsel. Three ColumnChunkFactory::createColumnChunkData calls passed a trailing false meant as initializeToZero, but the signature ends in bool hasNullData = true, bool initializeToZero = true, so it bound to hasNullData and mallocBuffer calloced the UINT64 buffers. The sites: the InMemChunkedCSRHeader constructor (every rel scan state owns one at NODE_GROUP_SIZE capacity, one per worker thread), and the length chunks in CountRelTable (its comment already said /*initializeToZero*/) and RelTable::getDegreeEntries.

Fix. Pass both flags explicitly: false /*hasNullData*/, false /*initializeToZero*/.

Why it is safe. Nothing reads these buffers before writing them, every header getter clamps to numValues, and flushing covers numValues entries, so an unwritten tail reaches neither stats nor disk. A build that fills all three buffers with 0xAB right after allocation passes the full ctest suite.

Measurements. The #1110 reproducer (100 Tag nodes, one hasType each), median of three runs of 200 timed executions, main at 305d80e before → after:

query threads wall ms CPU ms
MATCH (t:Tag)-[:hasType]->(k:Class) WHERE t.name = 'tag7' RETURN count(*) 1 0.607 → 0.424 1.08 → 0.76
16 2.063 → 0.618 12.64 → 1.12
same with a second hop (two rel scans) 16 3.953 → 0.969 29.47 → 1.64
same without the rel pattern 16 0.503 → 0.501 0.91 → 0.91

The repo's LDBC SNB SF1 suite (54 queries): geomean 0.92x at 16 threads and 0.93x at 1 thread; the 23 queries with a relationship pattern are at 0.84x, the 31 node-only queries at 0.98x and 1.00x. Every query returned identical output on both builds.

Tests. EmptyBufferManagerTest.CSRScanStateHeaderIsTwoPlainUInt64Buffers (test/storage/buffer_manager_test.cpp) pins the header a rel scan state carries: two UINT64 buffers of NODE_GROUP_SIZE capacity, no null chunk, the buffer pool charged exactly their size. It also passes on the unpatched tree: mallocBuffer accounts the bytes before it chooses calloc or malloc, so zeroing is invisible to every counter the engine exposes; the measurements above are the evidence it is gone. ctest (make test-build-release): 0 of 2861 failed (23 skipped), 258 disabled by the suite. clang-format 18.1.8 reports no changes.

Kuzu history, write-before-read sites, full table and methodology

kuzu 0.11.3 built the header through ColumnChunk(…, residencyState, false), whose last parameter is initializeToZero. The Segmentation refactor (kuzudb/kuzu#5950) moved the call to the factory, where the same false lands on hasNullData; kuzu master still has that call.

Where each buffer is written before it is read: the scan path resets numValues to 0 and appends the on-disk header through scanCommitted; checkpoint writes with copyFrom or populateCSRLengthInMemOnly + populateStartCSROffsetsFromLength; COPY starts with the std::fill in populateCSRLengthsInternal; the two length chunks are filled by csrLengthColumn->scan before they are summed. getMetadataToFlush and flushBuffer cover numValues entries.

Reproducer setup: against the C API, both builds make benchmark (Release, gcc 13.3), idle 16-core Ryzen 9 6900HS, CHECKPOINT before timing, max_num_threads = 16, lbug_connection_set_max_num_thread_for_exec(N). Rows not shown above:

query threads wall ms CPU ms
MATCH (t:Tag)-[:hasType]->(k:Class) WHERE t.name = 'tag7' RETURN count(*) 4 0.977 → 0.580 2.34 → 1.03
MATCH (t:Tag)-[:hasType]->(k:Class) RETURN count(*) (plans as COUNT_REL_TABLE) 16 0.253 → 0.191 0.44 → 0.38

In a long-running process the freed buffers are reused, so the cost shows up as CPU spent zeroing rather than as page faults; the Python script in #1110 starts a fresh process and goes from 5492 to 19 minor faults per execution at 16 threads (0.21.1, header fix applied).

Benchmark suite: benchmark/queries/ldbc-sf100 on LDBC SNB SF1, 1 warm-up and 5 runs per query, six rounds alternating which build runs first, median of the 30 runs. Largest gains at 16 threads: join/SelectiveTwoHopJoin 3.58 → 1.37 ms, join/q31 1.71 → 0.72 ms, ldbc_snb_ic/q36 6.15 → 3.99 ms (join/q31 1.17 → 0.28 ms at 1 thread). Individual node-only ratios span 0.82x–1.08x at 16 threads, the noise band for single-query ratios here.

Three createColumnChunkData calls passed a trailing false meant as
initializeToZero, but the factory's sixth parameter is hasNullData, so
the UINT64 buffers were calloc'ed: the CSR header of every rel scan
state (per worker thread per SCAN_REL_TABLE), and the length chunks in
CountRelTable and RelTable::getDegreeEntries. Pass both flags
explicitly. Nothing reads these buffers before writing them.

Fixes LadybugDB#1110.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@adsharma

adsharma commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Thank you!

@adsharma
adsharma merged commit 20e26f4 into LadybugDB:main Oct 5, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: every relationship scan zeroes a full-size CSR header for each worker thread

2 participants