Skip to content

perf(storage): reuse pooled buffer in recompaction writes - #245

Merged
ovaistariq merged 3 commits into
mainfrom
perfloop-pr-open-p714bgyjdv
Sep 9, 2026
Merged

ovaistariq merged 3 commits into
mainfrom
perfloop-pr-open-p714bgyjdv

Conversation

@perfloop

@perfloop perfloop Bot commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

The change

Segment.WriteEntry keeps the source io.WriterTo fast path and uses the acquired pooled buffer for readers without it. This prevents the destination file's ReaderFrom fallback from allocating another copy buffer for SectionReader inputs used by recompaction, while retaining the segment lock, rollback, short-write handling, byte-count validation, and accounting updates.

The eviction protection check now retries the second batch of order-key writes and marks setup inconclusive if any write fails. Its wait no longer requires one ocachecli list call to observe all 30 keys before checking the settled LRU/FIFO survivor set.

Testing

  • make test-storage TESTRUN='WriteEntry\|TestSegmentRecompaction_WithFragmentation\|TestSegmentRecompaction_IncompletePassKeepsOldSegment'
  • make test-e2e-eviction
  • make lint
  • make fmt-check
  • make license-check

Workload: ordinary timer-driven recompaction of two fragmented segments with 768 live 256 KiB entries at the supported default one-minute interval, with allocation as the outcome metric

Metric Before After Change
recompaction-gc-alloc-bytes/op 88640676 61053600 31.1% lower

Workload: synchronous 64 KiB SectionReader payload copied through Segment.WriteEntry to an *os.File destination, repeated 10000 times

Metric Before After Change
allocs/op 3 2 33.3% lower
B/op 32916 62 99.8% lower

Checks: 6 passed.


Generated by Perfloop. Measurements and checks.


Note

Medium Risk
Changes the segment write hot path for non-WriterTo readers; behavior is guarded by new tests and benchmarks but affects all SectionReader/rate-limited compaction copies.

Overview
Segment.WriteEntry now splits payload copy behavior by reader type: sources that implement io.WriterTo (e.g. *os.File) still copy directly to the segment file with the existing pooled 64 KiB buffer; readers without it (notably io.SectionReader during recompaction) copy through a plain io.Writer wrapper so io.CopyBuffer does not take the destination file’s ReaderFrom path and allocate an extra ~32 KiB buffer per call.

Benchmark and test coverage adds Linux timer-driven recompaction loop benchmarks (1 ms and production intervals, with optional CPU/GC/allocation metrics), factors startRecompactionBenchmark to accept a configurable interval, and adds TestManager_WriteEntrySectionReader plus a micro-benchmark for 64 KiB SectionReader writes.

Reviewed by Cursor Bugbot for commit 10706e8. Bugbot is set up for automated code reviews on this repo. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 28, 2026 •

Copy link
Copy Markdown

Greptile Summary

The PR reduces recompaction-copy allocations by forcing non-WriterTo readers through io.CopyBuffer with the existing pooled buffer while preserving source-owned fast paths.

  • Adds SectionReader correctness and allocation benchmark coverage.
  • Adds timer-driven recompaction benchmarks at configurable intervals.
  • Refactors benchmark setup to accept a recompaction interval.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
storage/segment/segment.go Selects the source WriterTo fast path when available and otherwise hides the file’s ReaderFrom method so io.CopyBuffer uses the pooled buffer; no eligible follow-up defect was established.
storage/segment/manager_test.go Adds correctness and allocation coverage for writing SectionReader payloads.
storage/compaction_loop_bench_linux_test.go Adds one-pass timer-driven recompaction benchmarks with workload and allocation metrics.
storage/compaction_serving_bench_linux_test.go Generalizes recompaction benchmark startup to accept a caller-selected interval.

Reviews (2): Last reviewed commit: "refactor(segment): hide the file's Reade..." | Re-trigger Greptile

efirs
efirs previously approved these changes Aug 28, 2026
@perfloop
perfloop Bot force-pushed the perfloop-pr-open-p714bgyjdv branch from 2a5fbcc to 2b036b0 Compare August 28, 2026 17:31
Comment thread tests/e2e/eviction_test.sh Outdated
@perfloop

perfloop Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in 1460517a1937. Response to this feedback.

The repair keeps the recompaction copy-path change intact and adds the eviction state-transition guard, so the E2E protection assertion cannot accept an unsettled cleaner state.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 1460517. Configure here.

Comment thread tests/e2e/eviction_test.sh Outdated
@perfloop
perfloop Bot force-pushed the perfloop-pr-open-p714bgyjdv branch from 1460517 to 5719bad Compare August 28, 2026 21:54
@perfloop

perfloop Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in 5719bada71f2. Response to this feedback.

The repaired test guard now retries the second order-key batch and avoids requiring a transient full-key listing, while preserving the existing LRU/FIFO survivor assertions.

perfloop-agent and others added 3 commits September 9, 2026 15:46
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
… the copy loop

io.CopyBuffer only ignores buf because *os.File implements io.ReaderFrom;
wrapping the destination in a plain io.Writer makes CopyBuffer use the
pooled buffer, so the explicit copy loop is not needed. Also drops the
unrelated eviction e2e script edits, which main fixed separately.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCoN4Him5ZY4hZovVp37ra
Signed-off-by: Ovais Tariq <ot@tigrisdata.com>
@ovaistariq
ovaistariq force-pushed the perfloop-pr-open-p714bgyjdv branch from 5719bad to 10706e8 Compare September 9, 2026 22:47
@ovaistariq

Copy link
Copy Markdown
Contributor

Rebased onto main and simplified (10706e8). The hand-rolled copy loop is replaced by wrapping the destination in a plain io.Writer, which hides *os.File's ReaderFrom and makes io.CopyBuffer use the pooled buffer — same effect, one line. The unrelated edits to the eviction e2e script are dropped; main fixed that test differently (#230, #261). Segment suites pass. Ready to merge.

@ovaistariq
ovaistariq merged commit d76283b into main Sep 9, 2026
9 checks passed
@ovaistariq
ovaistariq deleted the perfloop-pr-open-p714bgyjdv branch September 9, 2026 23:29
@tigrisdata-argocd-bot

Copy link
Copy Markdown

🎉 This PR is included in version 1.14.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants