Repository navigation
perf(cpp): enlarge the TsFile write stream page size to 64 KiB - #1087
Merged
Merged
Conversation
Pending bytes are drained to the file one page at a time, so WRITE_STREAM_PAGE_SIZE is both the write() granularity and the per-flush allocation unit. At 512 bytes a single chunk flush issued dozens of tiny write() syscalls and allocated a fresh page chain, which showed up as ~27% of the write path in a sampled profile. Raising it to 64 KiB keeps a chunk's data in one or two pages. On a 3.2M-row / 1000-device write the time drops from 0.60s to 0.45s (~25%) with an identical output size.
The I/O writer drains pending bytes one 64 KiB stream page at a time, so a chunk larger than one page is written as a chain of write() calls. Write INT64 series of several sizes around the page boundary (one page, just under, just over, and multiple pages) and read every value back. This checks that the page chain is flushed in order and that a chunk ending on a page boundary is not truncated.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1087 +/- ##
===========================================
+ Coverage 66.51% 66.57% +0.06%
===========================================
Files 778 782 +4
Lines 56577 56709 +132
Branches 9105 9134 +29
===========================================
+ Hits 37633 37756 +123
- Misses 17291 17295 +4
- Partials 1653 1658 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review follow-up: the integration test claimed to cover a chunk ending exactly on a page boundary, but the per-chunk metadata overhead means no row count in it lands on a multiple of the page size, and ByteStream::Consumer's exact-multiple branch had no test at all. Add a Consumer test that drains byte streams of 0, one byte less than a page, exactly one/two/three pages, and page multiples plus a partial tail, asserting both the per-buffer lengths and the reassembled bytes. Correct the integration test's comment to say what it actually covers.
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
WRITE_STREAM_PAGE_SIZEis a header-defined in-class integral constant. Usingstatic constexpr… · Newcommonnamespace is closed, while earlier tests in this file… · Newcommonnamespace is closed, while earlier tests in this file… · New
What changed in this PR
Improves TsFile write performance by increasing the TsFileIOWriter write-stream page size so flushes issue fewer write() syscalls and allocate fewer pages, while adding targeted tests for page-draining behavior.
Changes:
- Increase
WRITE_STREAM_PAGE_SIZEfrom 512 B to 64 KiB inTsFileIOWriter. - Add a
ByteStream::Consumertest to cover exact page-multiple draining and partial tails. - Add an end-to-end writer/reader test intended to exercise multi-page chunk flushing.
| File | Description |
|---|---|
| cpp/src/file/tsfile_io_writer.h | Grow write-stream page size to 64 KiB and document rationale. |
| cpp/test/common/allocator/byte_stream_test.cc | Add coverage for page-by-page draining, including exact-multiple boundaries. |
| cpp/test/writer/tsfile_writer_test.cc | Add a writer/reader test intended to validate correct flushing across multiple pages. |
Review effort: Lite
Follow-up to review comments: declare WRITE_STREAM_PAGE_SIZE as constexpr, keep the Consumer test inside namespace common, and assert that the largest case still exceeds one write-stream page so the multi-page drain cannot silently stop being exercised.
The test binds the constant to a reference (EXPECT_LT), which odr-uses it; before C++17 a static constexpr data member then needs a definition, and the Linux/LTO builds failed to link. Add the definition.
Parallel CTest processes can seed the name generator in the same clock tick; one then removes the file another is opening (E_ALREADY_EXIST at the fixture's open, observed as SEGFAULT / test failure on CI). Append pid and a per-process counter, as the writer tests already do.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the stream page-size increase.
No open issues 🎉
4 resolved since last review
WRITE_STREAM_PAGE_SIZEis a header-defined in-class integral constant. Usingstatic constexpr…commonnamespace is closed, while earlier tests in this file…commonnamespace is closed, while earlier tests in this file…
Review effort: Balanced
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
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.
Problem
TsFileIOWriterdrains its pending bytes to the file one page at a time, soWRITE_STREAM_PAGE_SIZE(512 bytes, markedFIXME) is both thewrite()granularity and the per-flush allocation unit. A single chunk flush therefore issued dozens of tinywrite()syscalls and allocated/freed a fresh page chain; in a sampled profile thewritesyscall alone was ~27% of the write path.Change
Raise the page size to 64 KiB. A chunk's pending data now fits in one or two pages.
Benchmark
3.2M rows / 1000 devices / 4 columns, single thread, alternating dylib A/B:
~25% faster with identical output size.
Testing
Full C++ suite: 1454 tests passed. No file-format change.