experimental/air: parallel gzip for the plain_tar snapshot packer - #6571
Merged
Merged
Conversation
This was referenced Sep 8, 2026
ben-hansen-db
marked this pull request as ready for review
September 8, 2026 17:42
vinchenzo-db
approved these changes
Sep 8, 2026
| @@ -9,6 +9,8 @@ import ( | |||
| "os/exec" | |||
| "path/filepath" | |||
| "strings" | |||
|
|
|||
Contributor
There was a problem hiding this comment.
is Go semantics/lint to have this empty space? if not then rm
Contributor
Author
There was a problem hiding this comment.
need to keep it according to claude:
that blank line is the standard import grouping (stdlib vs third-party) that goimports/gofumpt enforce, and this repo runs both as formatters. Without it, gofmt sorts github.com/klauspost/pgzip alphabetically into the stdlib block (between fmt and os) and goimports re-adds the separator, so ./task fmt would put it right back.
Comment on lines
+69
to
+71
| // level 9 buys almost nothing beyond that for ~2x the time. Compressing outside tar | ||
| // also passes no archive path to tar, sidestepping the Windows colon-in-path issue a | ||
| // `-f <path>` argument otherwise hits (tar reads the `C:` in `C:\out\x` as a host). |
Contributor
There was a problem hiding this comment.
Either dumb down this part or rm imo
createPlainTarball shelled out to `tar -czf`, whose gzip is single-threaded and dominates packaging time for a large code_source tree. Pipe `tar -cf -` through klauspost/pgzip instead, spreading compression across cores. Measured ~4x on universe/research (2456 ms -> 609 ms) and ~18x on the 470 MiB gzip step alone; the output is an ordinary gzip stream. Level is BestSpeed since the uploaded size does not matter for this workflow, only latency. Compressing outside tar also passes no archive path to tar, which removes the Windows colon-in-path workaround (bare `-f` basename + -C) the -czf form needed. Co-authored-by: Isaac <no-reply@databricks.com>
Now that gzip is parallel the compression level is nearly free, so trade a little CPU for a smaller upload -- the plain_tar archive is re-uploaded on every run. DefaultCompression matches the old `tar -czf` size at ~18x the speed (research: 716 ms / 24 MB, vs BestSpeed 609 ms / 27 MB). Level 9 buys ~1% fewer bytes for ~2x the time, so 6 is the knee. Co-authored-by: Isaac <no-reply@databricks.com>
Vincent found the pack-step comment too dense. Cut it from the compression-level essay (that rationale lives in the PR description and commit message) down to the two non-obvious whys: parallel gzip vs tar's single-threaded -z, and compressing outside tar to avoid the Windows colon-in-path issue. Co-authored-by: Isaac <no-reply@databricks.com>
ben-hansen-db
force-pushed
the
air-plain-tar-pgzip
branch
from
September 8, 2026 21:52
b732496 to
8155ccf
Compare
Collaborator
Integration test reportCommit: 660b22e
Top 6 slowest tests (at least 2 minutes):
|
pietern
approved these changes
Sep 10, 2026
… out to tar Per review, replace the `tar -cf -` subprocess in createPlainTarball with a Go-native tar writer. Add libs/tarpack, a small helper that writes a tar stream to a caller-supplied io.Writer, so compression stays the caller's choice: air run wraps it in klauspost/pgzip, keeping the parallel-gzip win while dropping the external tar dependency and its Windows colon-in-path workaround. Symlinks, file modes, and mtimes are preserved, matching what tar stored. Enumeration (git ls-files) and the git-ref path (git archive) are unchanged. The helper lives under libs/ so other packers can adopt it, but only air run is wired to it here. Co-authored-by: Isaac <no-reply@databricks.com>
Collaborator
Integration test reportCommit: 6ec1ee1
492 interesting tests: 486 FAIL, 5 KNOWN, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
sunishsheth2009
pushed a commit
to sunishsheth2009/cli
that referenced
this pull request
Sep 15, 2026
## Summary Makes the `plain_tar` upload content-addressed so an unchanged working tree skips packaging **and** upload. Today `git_archive` names its tarball by `(commit, include_paths)` and, if that object is already uploaded, reuses it and moves no bytes. `plain_tar` (dirty working tree / no ref — the normal iterate-and-resubmit loop) used a **timestamped** name, so it re-packaged and re-uploaded the full tarball on *every* submission, even when nothing changed. This change: - **`snapshotFiles`** records each path's size, mtime, mode/type, and symlink target. Size, mtime, and mode/type come from the existing `Lstat`; only symlinks require an additional `Readlink`. - **`computePlainTarKey`** hashes the sorted `path+size+mtime+mode+symlink_target` set into a versioned working-tree fingerprint. - **`snapshotTarName`** names `plain_tar` `<dir>_<fingerprint>.tar.gz` (was `<dir>_<timestamp>.tar.gz`) and returns the file listing so packaging reuses it — the tree is walked once, not twice. - **`uploadSnapshotViaDABs`** runs the existing `snapshotExists` skip for **both** modes: if the object already exists, it reuses the remote path and uploads nothing. ## Validation - `go test ./experimental/air/cmd` passes. Tests cover remote deduplication and verify that path, size, mtime, mode/type, and symlink-target changes independently alter the cache key. - `./task lint-q` passes with zero issues. - **End-to-end on df1.** Submitting an unchanged universe tree twice: the second run logged `snapshot upload skipped; reusing …<fingerprint>.tar.gz`, returned the identical remote `code_source_path`, and skipped packaging + upload. Both runs launched (1×A10): | code source | first submit (package + upload) | second submit (unchanged → skipped) | |---|---|---| | `research` (~27 MB) | 5.7 s | 4.9 s | | `research` + `js` + `spark` (~97 MB) | 37.6 s | 4.8 s | These measurements preceded the parallel gzip improvement in databricks#6571, so the first-submit numbers are historical; the unchanged-tree skip remains the behavior demonstrated here. ## Notes / trade-offs - Regular-file content detection is size+mtime, not content — the same trade-off DABs file sync makes — so an edit preserving both is not detected. Mode/type and symlink-target changes are detected. - Remote objects accumulate (one per distinct tree state), same as `git_archive`; no eviction is added here. --- This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com>
janniklasrose
pushed a commit
that referenced
this pull request
Sep 15, 2026
) ## Summary The `air run` plain_tar snapshot path (dirty working tree / no git ref) packages the code_source by shelling out to `tar -czf`. tar's built-in gzip is single-threaded, so for a large tree it dominates packaging latency. This pipes `tar -cf -` (uncompressed) through **klauspost/pgzip** (MIT), a parallel drop-in for gzip that spreads compression across cores and still emits an ordinary gzip stream. - Level is **DefaultCompression** — the *same level as the old `tar -czf`*, not BestSpeed. The archive is re-uploaded on every run, so its size matters. So this is a pure latency win with no upload-size regression. - Compressing outside tar means **no archive path is passed to tar**, which lets us drop the Windows colon-in-path workaround the `-f <path>` form required. ## Results (local packaging: git walk + tar + gzip; warm page cache) | Target | before (`tar -czf`, serial gz6) | after (`tar -cf -` \| pgzip, gz6) | |---|---|---| | large folder (7.4k files) | 2,456 ms | **716 ms** (~3.4×) | | gzip step alone, 470 MiB tar | 7,940 ms | **436 ms** (~18×) | Same compression level, so the tarball is the same size as before (~24 MB for large folder). Level chosen from the curve on a 476 MiB tar: L1→L6 costs +150 ms for ~17% fewer bytes (97→80 MB); L6→L9 doubles time for ~1%, so 6 is the knee. ## Validation - Output verified: `gzip -t` clean, entry count matches the file list, extracts correctly. - Existing `snapshot_package_test.go` unit tests pass; the `internal/build` license test passes for the new `pgzip` dep (`// MIT` in go.mod + NOTICE entry). --------- Co-authored-by: Isaac <no-reply@databricks.com>
janniklasrose
pushed a commit
that referenced
this pull request
Sep 15, 2026
## Summary Makes the `plain_tar` upload content-addressed so an unchanged working tree skips packaging **and** upload. Today `git_archive` names its tarball by `(commit, include_paths)` and, if that object is already uploaded, reuses it and moves no bytes. `plain_tar` (dirty working tree / no ref — the normal iterate-and-resubmit loop) used a **timestamped** name, so it re-packaged and re-uploaded the full tarball on *every* submission, even when nothing changed. This change: - **`snapshotFiles`** records each path's size, mtime, mode/type, and symlink target. Size, mtime, and mode/type come from the existing `Lstat`; only symlinks require an additional `Readlink`. - **`computePlainTarKey`** hashes the sorted `path+size+mtime+mode+symlink_target` set into a versioned working-tree fingerprint. - **`snapshotTarName`** names `plain_tar` `<dir>_<fingerprint>.tar.gz` (was `<dir>_<timestamp>.tar.gz`) and returns the file listing so packaging reuses it — the tree is walked once, not twice. - **`uploadSnapshotViaDABs`** runs the existing `snapshotExists` skip for **both** modes: if the object already exists, it reuses the remote path and uploads nothing. ## Validation - `go test ./experimental/air/cmd` passes. Tests cover remote deduplication and verify that path, size, mtime, mode/type, and symlink-target changes independently alter the cache key. - `./task lint-q` passes with zero issues. - **End-to-end on df1.** Submitting an unchanged universe tree twice: the second run logged `snapshot upload skipped; reusing …<fingerprint>.tar.gz`, returned the identical remote `code_source_path`, and skipped packaging + upload. Both runs launched (1×A10): | code source | first submit (package + upload) | second submit (unchanged → skipped) | |---|---|---| | `research` (~27 MB) | 5.7 s | 4.9 s | | `research` + `js` + `spark` (~97 MB) | 37.6 s | 4.8 s | These measurements preceded the parallel gzip improvement in #6571, so the first-submit numbers are historical; the unchanged-tree skip remains the behavior demonstrated here. ## Notes / trade-offs - Regular-file content detection is size+mtime, not content — the same trade-off DABs file sync makes — so an edit preserving both is not detected. Mode/type and symlink-target changes are detected. - Remote objects accumulate (one per distinct tree state), same as `git_archive`; no eviction is added here. --- This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com>
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Sep 18, 2026
…6577) Both DABs-built tgz paths gzip single-threaded today: tarballFromInclude via compress/gzip and tarballFromGit via git's --format=tar.gz. gzip dominates packaging time on a large tree. Pipe both through klauspost/pgzip (MIT), which spreads compression across cores and still emits an ordinary gzip stream, at the same default level so tarball sizes are unchanged. The tgz artifact is content-addressed on upload, so the compressed bytes must be reproducible. pgzip only parallelizes across fixed-size blocks, so pinning the block size keeps output identical regardless of the build host's core count; TestTarballFromGitIsReproducible guards this. Mirrors the pgzip approach in the air plain_tar packer (#6571), requested by pietern. Co-authored-by: Isaac <no-reply@databricks.com> Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>
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.
Summary
The
air runplain_tar snapshot path (dirty working tree / no git ref) packages the code_source by shelling out totar -czf. tar's built-in gzip is single-threaded, so for a large tree it dominates packaging latency.This pipes
tar -cf -(uncompressed) through klauspost/pgzip (MIT), a parallel drop-in for gzip that spreads compression across cores and still emits an ordinary gzip stream.tar -czf, not BestSpeed. The archive is re-uploaded on every run, so its size matters. So this is a pure latency win with no upload-size regression.-f <path>form required.Results (local packaging: git walk + tar + gzip; warm page cache)
tar -czf, serial gz6)tar -cf -| pgzip, gz6)Same compression level, so the tarball is the same size as before (~24 MB for large folder). Level chosen from the curve on a 476 MiB tar: L1→L6 costs +150 ms for ~17% fewer bytes (97→80 MB); L6→L9 doubles time for ~1%, so 6 is the knee.
Validation
gzip -tclean, entry count matches the file list, extracts correctly.snapshot_package_test.gounit tests pass; theinternal/buildlicense test passes for the newpgzipdep (// MITin go.mod + NOTICE entry).