Skip to content

db/seg: collapse compression workers=1 case into batched workers flow - #23058

Open
sudeepdino008 wants to merge 2 commits into
mainfrom
sudeep/seg-cover-single-path
Open

db/seg: collapse compression workers=1 case into batched workers flow#23058
sudeepdino008 wants to merge 2 commits into
mainfrom
sudeep/seg-cover-single-path

Conversation

@sudeepdino008

@sudeepdino008 sudeepdino008 commented Aug 6, 2026

Copy link
Copy Markdown
Member

Summary

  • collapse case of workers>1 and workers=1 in single flow
  • this is a simplification; W=1 case was specially handled, removing that and merging with other flow leads to 1% slower merge file creation, which I consider acceptable.

Benchmark

Re-compressing the word stream of a real mainnet storage .kv (527 MB, 17.6M words / 8.8M keys, key compressed + value not, as CompressKeys does), 2 iterations x 2 rounds:

base (#21997) this PR
cover phase, W=1 10.02 s 10.29 s
cover phase, W=4 (control) 3.11 s 3.07 s
end-to-end, W=1 34.30 s 34.53 s

Workers > 1 is unchanged code, so those rows measure the noise floor (±1.5%).

A synthetic corpus (1M sorted 52-byte storage-like keys) resolves the Workers=1 cost more tightly — 10 iterations x 3 alternating rounds, medians varying under 1 ms within each version:

base (#21997) this PR
cover phase, W=1 322.0 / 321.8 / 322.5 ms 329.2 / 331.7 / 330.1 ms
end-to-end, W=1 1432 / 1431 / 1439 ms 1435 / 1442 / 1448 ms

So ~2.6% on the cover phase at Workers=1, under 1% end-to-end — the two extra CompressionWord copies plus the queue push/pop. Dictionary learning dominates a real merge, which is why the end-to-end cost stays inside the noise.

Notes

  • Pure refactor, no behavior change, so no new red-green cycle — the existing tests plus the golden checksums are the safety net.

@sudeepdino008 sudeepdino008 changed the title db/seg: run the single-worker cover phase through the batched pipeline db/seg: collapse workers=1 case of compression into batched workers flow Aug 7, 2026
@sudeepdino008
sudeepdino008 marked this pull request as ready for review August 7, 2026 04:52
@sudeepdino008
sudeepdino008 requested a review from Copilot August 7, 2026 04:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors db/seg compression so the Workers=1 cover/compression path uses the same batched-worker pipeline as the Workers>1 path, removing the prior special-cased single-worker implementation. This simplifies the code while accepting a small Workers=1 performance regression, and updates tests to pin byte-identical output to pre-batching checksums across worker counts.

Changes:

  • Remove the special-case single-worker cover/compression path and always run through the batched worker pipeline.
  • Adjust cover batching sizing logic to apply uniformly, and unify queue/drain logic for all worker counts.
  • Update parallel compression tests to validate round-trip correctness and stable on-disk encoding via fixed CRC32 checksums across worker counts.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
db/seg/parallel_compress.go Collapses Workers=1 and Workers>1 compression flows into a single batched worker pipeline.
db/seg/compress_parallel_test.go Updates helpers/tests to assert stable output encoding via fixed checksums across worker counts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread db/seg/compress_parallel_test.go Outdated
@sudeepdino008 sudeepdino008 changed the title db/seg: collapse workers=1 case of compression into batched workers flow db/seg: collapse compression workers=1 case into batched workers flow Aug 7, 2026
Base automatically changed from sudeep/seg-cover-batching to main August 7, 2026 07:02
Collapse the Workers==1 special case into the batched worker pipeline so there
is a single code path. Pure refactor, no behavior change: the golden checksums
pin every worker count (including 1) to the original on-disk encoding.

W=1 costs ~2.6% on the cover phase (two extra CompressionWord copies plus the
queue push/pop) and under 1% end-to-end, since dictionary learning dominates a
real merge.
@sudeepdino008
sudeepdino008 force-pushed the sudeep/seg-cover-single-path branch from 40c9fb1 to baae28a Compare August 7, 2026 09:24
@sudeepdino008
sudeepdino008 enabled auto-merge August 8, 2026 13:59
@sudeepdino008
sudeepdino008 added this pull request to the merge queue Aug 9, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 9, 2026
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.

3 participants