Skip to content

Small snapshot uploads no longer reserve 1 MiB compression chunks per worker - #3375

Open
Abhishek Sharma (abhisheksharma2411) wants to merge 1 commit into
agent-substrate:mainfrom
abhisheksharma2411:fix/2553-lower-parzstd-min-chunk
Open

Abhishek Sharma (abhisheksharma2411) wants to merge 1 commit into
agent-substrate:mainfrom
abhisheksharma2411:fix/2553-lower-parzstd-min-chunk

Conversation

@abhisheksharma2411

Copy link
Copy Markdown

Why is this needed? (Or link the issue.)

Follow-up to #2945, under #2553. #2945 sizes the compression chunk to the data,
but floors it at 1 MiB, and the pool is still filled eagerly with
workers * parZstdQueue buffers. So a tiny file still costs workers * 2 * 1 MiB,
and planStream takes workers from GOMAXPROCS, so the cost grows with the
node rather than with the file.

Measured on a 36-byte file, allocation during writeSparseZstd:

workers before after
1 1.6 MiB 0.6 MiB
8 16.6 MiB 1.6 MiB
16 24.6 MiB 2.1 MiB

The floor can come down because a chunk is a whole single-segment frame. An
input at or below the floor is one frame either way and nothing can match past
its end, so a smaller floor costs no ratio -- it only narrows the encoder's
window to something the frame could never have used.

This also shows up in the planner: for an all-holes file the reservation drops
from 8.5 MiB to 1 MiB on GCS, and 168 MiB to 160.5 MiB on S3.

Two existing uploader expectations moved. Both were written in terms of
parZstdMinChunk where they meant the chunk actually chosen for that input --
the two were equal only while the floor was 1 MiB. They now say
parZstdChunkSize(...) and pass at either floor, which I checked by reverting
the constant.

parZstdChunkSize had no test, so this adds one, plus a round-trip at sizes
either side of the new floor (the one place a too-small window would corrupt
rather than just enlarge output).

Not included: making the pool allocate lazily instead of eagerly, which would
flatten the remaining worker-count scaling. That is a real change to the pool
rather than a constant, so it belongs in its own PR.

Adjacent, not conflicting as far as I can tell: #1766 also touches parzstd.go
but predates #2945.

AI use: AI-assisted. I reviewed and tested it.

  • Tests pass
  • Docs updated (no user-facing docs change)

… worker

A chunk is a whole single-segment frame, so the floor only has to cover the
encoder's own state. Lower it to 64 KiB.

Measured on a 36-byte file: 16.6 MiB -> 1.6 MiB at 8 workers.

The two uploader expectations that moved were written against parZstdMinChunk
where they meant the chunk actually chosen; they now pass at either floor.

Refs agent-substrate#2553

Signed-off-by: Abhishek Sharma <[email protected]>

This branch has not been deployed

No deployments
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.

1 participant