RajaVinoth96 opened a new pull request, #20263: URL: https://github.com/apache/pinot/pull/20263
Suggested labels: `bugfix`, `performance` ## Summary This builds on #19577 by @cypherean (durable flush on segment write paths, issue #19576). Their commit `fcf8dca` is included unchanged, and this PR adds two commits on top: 1. **Fixes for review comments on #19577**, including both of @tarun11Mavani's comments. 2. **`BenchmarkSegmentCreation`**, a JMH benchmark for end-to-end segment creation, plus measurements of what the added syncs cost. If #19577 merges first, this rebases down to just those two commits. I'm also happy to fold them into #19577 if @cypherean prefers that. ## Changes on top of #19577 | Change | Why | |---|---| | **`CLPForwardIndexCreatorV1/V2.close()`**: always delete the intermediate `.clp.tmp` directory, using a nested `try`/`finally` (as suggested inline) | #19577 made `forceMappedBuffers` and `syncAndClose` able to throw, so a failed final sync left the temp directory on disk. | | **`MmapMemory.flush()`**: skip read-only, empty and closed mappings | `PinotDataBuffer.close()` flushes every buffer, so segment unload was msyncing read-only `columns.psf` mappings, and #19577 makes a failed msync throw. Read-write mappings keep the page-aligned msync and still fail loudly. This matches the existing `_flushable` guard in `PinotByteBuffer`. | | **`BitmapInvertedIndexWriter.close()`**: unmap only in the `finally` block, with no second force | `cleanBitmapBuffer()` forced the buffer again inside `finally`. If the first force failed, the second one could throw too, which replaced the original exception and skipped `freeBuffer`. Resizing still forces the old window before unmapping it. The file fsync now runs in the `try` block for both owned and borrowed channels, and the `finally` block only closes and unmaps. | | **`BaseChunkForwardIndexWriter` / `VarByteChunkForwardIndexWriterV4.close()`**: release the file, compressor, direct buffers and scratch file in `finally`; fsync only once every write has succeeded | A failed fsync skipped `_chunkCompressor.close()`, which leaked the native memory held by gzip's `Deflater`, and V4's buffer and scratch-file cleanup. | | **`MmapMemoryTest`**: tests for the read-only, empty and closed cases, and for a read-write flush at an unaligned offset | | Benchmarks: `pinot-perf/.../BenchmarkSegmentCreation.java` builds a full segment with `SegmentIndexCreationDriverImpl`. The table has 25 columns: 10 dictionary-encoded INTs (5 with inverted indexes), 2 range-indexed INTs, 5 raw LONGs, 5 dictionary-encoded STRINGs (2 inverted), 2 raw STRING payload columns and 1 JSON column with a JSON index. Rows come from a seeded generator, so every pass and every run sees identical data. The benchmark has two parameters, `_numRows` (500k or 2M) and `_rawPayloadLength` (64 or 512 bytes), which give segments from 123 MB to 2.1 GB. A byte counter reports the final segment size. ## Measured cost of the syncs All runs compare the PR's base (`8e454ba`) with #19577 plus this PR. They ran on GitHub's `ubuntu-latest` runners in my fork (4 vCPUs, 15 GB RAM, ext4, Temurin 25.0.5). The workflow and scripts are on [`bench/durable-flush`](https://github.com/RajaVinoth96/pinot/tree/bench/durable-flush/.github), and each run's raw output is attached to it as the `bench-results` artifact. **1. Writer microbenchmarks** ([run](https://github.com/RajaVinoth96/pinot/actions/runs/43667214291)). Each writer runs on its own and syncs immediately, which is the worst case. - `BenchmarkRawForwardIndexWriter`, with 130 to 750 MB files: **+23 to 97%**. The extra time works out to about 1.0 to 1.35 GB/s of data, so it is proportional to the bytes written. - `BenchmarkOffheapBitmapInvertedIndexCreator` with 10M docs: **+65 to 92%** for round-robin values (large bitmaps) and +6 to 13% for sorted values. **2. End-to-end segment creation** ([run](https://github.com/RajaVinoth96/pinot/actions/runs/43703991000)). Two alternating rounds per side, with 2 warmup and 5 measured iterations. | Rows × payload | Segment size | Change | |---|---|---| | 500k × 64 B | 123 MB | +0.3% | | 500k × 512 B | 555 MB | −1.3% | | 2M × 64 B | 454 MB | −2.3% | | 2M × 512 B | 2.1 GB | −1.2% | None of these differences is distinguishable from noise on this runner. **3. Time spent inside sync calls**, measured with `strace -f -w -c` ([run](https://github.com/RajaVinoth96/pinot/actions/runs/43720045228)). This is more reliable than comparing build times. | Segment | Base: sync wait per build | This PR: sync wait per build | |---|---|---| | 123 MB | 0.14 s | 0.18 s | | 2.1 GB | 4.3 s (almost all msync) | 9.8 s (+5.5 s from new fsyncs) | On this runner a raw `force()` costs about 0.3 ms per call. Data is absorbed at several GB/s while the burst lasts, then falls to about **450 MB/s sustained**. The runner in run 1 appeared to be faster, so the cost depends heavily on the disk. **4. Every individual sync call**, traced with `strace -T -y` ([run](https://github.com/RajaVinoth96/pinot/actions/runs/43726287136)), for one 2.1 GB build: - **19 fsyncs per build**, up from 1 on the base, with **0 failures**. The median call takes **0.27 ms**. - **Two calls account for 5.27 s, 99.7% of the fsync time**: `t0.sv.raw.fwd` and `t1.sv.raw.fwd`, the roughly 1 GB raw string forward indexes. - **Every fsync lands on a file under the build's `tmp-*` directory.** With the default v3 format, the v1 per-column files are copied into `v3/columns.psf` and deleted, and the `*.json.idx.tmp` scratch files are also deleted. No fsync touched `v3/`. - `v3/columns.psf` is made durable by msync when its buffers close. Buffers up to 2 GB use `PinotByteBuffer`/`MappedByteBuffer.force()`, so this was already the case before #19577. On macOS the same benchmarks show much smaller costs, because `fsync` there doesn't wait for the drive cache (`F_FULLFSYNC` does). I'd treat the Mac numbers as a lower bound and haven't used them for the conclusions. Caveats: - Shared runners are noisy, and disk throughput varies between VMs. - The per-call trace is a single cold build per configuration. - In the strace runs, the base build always ran first. - I couldn't test on production-like storage. EBS gp3 at its default 125 MB/s is roughly 3.6× slower than this runner. By extrapolation, not measurement, a 2 GB segment build would wait an extra ~15 to 20 s there. ## Options for reviewers **Option A: drop the msync on each resize in `BitmapInvertedIndexWriter`.** In full builds the msync time didn't increase (3.8 s on this PR versus 4.7 s on the base). I suggest leaving this as is. **Option B: where the syncs happen on the build path.** With the default v3 format, the new per-writer fsyncs make temporary files durable just before they are deleted. Meanwhile the files that persist, `index_map`, `metadata.properties`, `creation.meta` and the `v3/` directory entries, are still never synced. That was true before #19577 as well. Some options: 1. **Keep the per-writer sync.** It is simple and correct. The cost is negligible for typical segments and roughly bytes ÷ disk throughput for large raw columns. 2. **Skip the sync for writer output that will be copied into `columns.psf`.** This needs a flag from the creator to the writers. Then sync the final `v3/` files and directory once when the build finishes. The protection for persisted data stays the same, without the temp-file cost. 3. **Keep the per-writer sync and also add the final-file and directory sync**, to close the remaining metadata gap. One argument for keeping the temp-file syncs: if background writeback of a v1 file fails and the kernel then evicts those pages before the v3 copy reads them, the copy reads stale bytes, and nothing reports an error. An fsync before the copy would surface that failure. I'd value your view on whether that case justifies the cost, or whether a targeted sync just before the copy (or verifying the copied bytes) would be better. I have a directory-sync change for the download path (`untarDurably`, segment moves, `cleanupRemovedIndices`) ready on a local branch. #19577 deliberately excludes rename and directory durability, so I left it out of this PR and can open it separately if wanted. ## Testing - Spotless, `license:format`, checkstyle and `license:check` on `pinot-segment-spi`, `pinot-segment-local` and `pinot-perf`. - 1,457 tests pass: `MmapMemoryTest`, `PinotDataBufferFlushAndCloseTest`, `UnsafeMemoryPinotDataBufferTest`, `BitmapInvertedIndexWriterTest`, the CLP V1/V2 creator tests, the V4/V5/V3 chunk writer tests, `FixedByteChunkSVForwardIndexTest` and `SingleFileIndexDirectoryTest`. - The tests don't simulate a failing `force()`/`msync`, so the new cleanup-on-failure paths are covered by review rather than by tests. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
