zeroshade commented on PR #1469: URL: https://github.com/apache/arrow-go/pull/1469#issuecomment-6690538424
@kszucs following up on the write overhead with CDC enabled. I profiled it and prototyped some changes to `chunker.go` that keep the page boundaries bit-identical. `TestChunksStableBoundaries` and the full `pqarrow` CDC suite pass with `-tags assert` and `-tags assert,noasm`, and `-race` passes for `cdc`. **Where the time goes** (pprof, Apple M4): in `BenchmarkChunks` almost everything is `Chunks`. For required int64 roughly 35% is hashing and ~55% is per-value overhead of the general loop (level checks, width dispatch, `needNewChunk`, offset bookkeeping). Strings also pay a non-inlinable closure call per value. **Changes that helped:** 1. **Skip the min-size window instead of walking it.** Values that only fill `MinChunkSize` are never hashed and can't end a chunk, so they only need their sizes added. Required fixed-width columns skip arithmetically, required binary/string columns binary-search the offsets, and nullable/nested fixed-width columns use a tight size-only loop. 2. **Specialized loops** for required 8-byte values and required binary/string values, reading the data and offsets buffers directly with no level handling or per-value closure. 3. **Split the hash chain per word.** For a word, the hash after byte j is `hash<<(j+1) + p_j`, where `p_j` rolls the word's own bytes from zero and doesn't depend on `hash`. The loop-carried chain becomes one shift-add per word instead of eight. `hash&mask == 0` is checked as `(hash>>shift) - 1` having its top bit set, OR-ed over the word, which avoids a compare per byte. On its own this was only ~6%; most of the gain is from 1 and 2. `BenchmarkChunks`: | Input | PR | Prototype | Speedup | | --- | ---: | ---: | ---: | | required int64 | 2335 MB/s | 4358 MB/s | 1.87x | | nullable int64 | 1886 MB/s | 2140 MB/s | 1.13x | | nested list<int32> | 650 MB/s | 784 MB/s | 1.21x | | required string | 2059 MB/s | 3391 MB/s | 1.65x | `BenchmarkWriteContentDefinedChunking` with CDC on (median of 5 interleaved runs): | Column | PR | Prototype | Change | | --- | ---: | ---: | ---: | | int64 | 5.13 ms | 3.52 ms | -31% | | string | 21.65 ms | 18.63 ms | -14% | **SIMD:** I don't think it's worth it here. The gear hash has to stay as is for compatibility with C++ and arrow-rs, and vectorizing it needs 64-bit gathers. The FAST'25 VectorCDC paper ([Udayashankar et al.](https://www.usenix.org/system/files/fast25-udayashankar.pdf)) measured only ~1.2x for AVX-512 Gear and no gain for FastCDC, because splitting hashing from boundary detection gives up min-size skipping. NEON has no 64-bit gather, so arm64 wouldn't benefit at all. **Other ideas, not prototyped:** - For strings, roll the contiguous value bytes as one stream and only stop at matches (~1 in 32k bytes with the defaults) or the max size, which would remove the remaining per-value cost. - The same specialization for required 4-byte and flat nullable 8-byte columns. Nullable matters since Arrow fields default to nullable. - Not CDC-specific: `PooledBufferWriter.Reset` and `poolBuffer.Release` `memset` the whole buffer capacity on every page flush. That was ~13% of the string write profile, and CDC makes it more visible because it produces more pages. None of this blocks the PR. Happy to share the patch (~200 lines, `chunker.go` only) if you'd like to fold it in here, or I can send it as a follow-up after this merges. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
