-
Notifications
You must be signed in to change notification settings - Fork 13
feat(profiling): zstd as a compression option #1065
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: PROF-11830-Add-upload-compression-configuration-to-libdatadog
Are you sure you want to change the base?
Conversation
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
BenchmarksComparisonBenchmark execution time: 2025-05-29 17:59:41 Comparing candidate commit b751d09 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 2 unstable metrics. CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
BaselineOmitted due to size. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## PROF-11830-Add-upload-compression-configuration-to-libdatadog #1065 +/- ##
=================================================================================================
- Coverage 70.92% 70.91% -0.02%
=================================================================================================
Files 329 329
Lines 49916 49919 +3
=================================================================================================
- Hits 35403 35399 -4
- Misses 14513 14520 +7
🚀 New features to boost your workflow:
|
80c8f07
to
0d6cabc
Compare
UploadCompression::Zstd => Self { | ||
buffer: Vec::with_capacity(TEMPORARY_BUFFER_SIZE), | ||
compressor: Compressor::Zstd { | ||
// A level of 0 uses zstd's default (currently 3). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that the level 3 from the upstream zstd C library? If yes, level 3 is good. I'm asking because there are different zstd implementations out there which assign different meanings to different levels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, at least as far as I understood it.
datadog-profiling/src/serializer/compressed_streaming_encoder.rs
Outdated
Show resolved
Hide resolved
d301705
to
8fa167e
Compare
8788621
to
714ecc0
Compare
5d54a41
to
9cafb27
Compare
714ecc0
to
5ed02e4
Compare
9cafb27
to
c4c8301
Compare
064a512
to
6f72248
Compare
49d50fa
to
9fa834b
Compare
6f72248
to
a13e0dd
Compare
a13e0dd
to
b751d09
Compare
9fa834b
to
35933df
Compare
What does this PR do?
Adds zstd compression as on option for upload config.
Motivation
zstd may have a better tradeoff for performance to compressed size.
Here are some benchmarks on compressors for a CPU profile which I cannot share publicly yet. We're working on anonymizing it so we can commit these to the repository. Throughput is in MiB/s. Utility is the compression ratio multiplied by throughput. This is on an Apple M1 Max.
I tested zstd-5 and zstd-6 informally, and compression rations were incrementally better, but you lose throughput steadily. Sticking to something in the level 1-4 range seems better.
And here's from another, also not public set of data:
My personal opinion is that zstd-1 is a safe upgrade. The performance is roughly similar to lz4, but the compression is much better.. Moving to a higher level is something that could be discussed.
Additional Notes
Currently lacks tests, pushed to see if this will cause build issues or significant artifact size bloat.
How to test the change?
Use
Zstd
instead ofOn
orLz4
, and most things should work the same, except it's compressed differently.