-
Notifications
You must be signed in to change notification settings - Fork 64
fix: [Geneva Exporter] Add LZ4 compression benchmark #284
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #284 +/- ##
=======================================
- Coverage 53.0% 53.0% -0.1%
=======================================
Files 63 64 +1
Lines 8288 8311 +23
=======================================
+ Hits 4400 4405 +5
- Misses 3888 3906 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@lalitb We want the tests related to the changes in the PR to be run in the CI. Could we update the workspace to include these projects first? opentelemetry-rust-contrib/Cargo.toml Lines 14 to 16 in 4bf4667
|
#[test] | ||
fn lz4_benchmark() { | ||
let mut criterion = Criterion::default() | ||
.sample_size(100) |
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.
Any specific reason to be changing the default sample size and measurement time?
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.
The benchmark was not completing in default time, so the sample size was changed. But not getting the issue anymore, so removed this.
Sure. |
let max_chunk_compressed = get_maximum_output_size(CHUNK_SIZE); | ||
let num_chunks = input.len().div_ceil(CHUNK_SIZE); | ||
let mut output = Vec::with_capacity(num_chunks * (4 + max_chunk_compressed)); | ||
let mut temp_buf = vec![0u8; max_chunk_compressed]; |
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.
I don't follow. We had removed all temporary buffer allocation. Why are we introducing it again?
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.
Good point. This is not correct. I see improvements in the small size with this, but not with large size. Have reverted the optimization efforts - the PR is now limited to adding the benchmark! Thanks for catching this.
return Ok(Vec::new()); | ||
} | ||
// Special case: very small data (less than one chunk) | ||
if input.len() <= CHUNK_SIZE { |
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.
From the PR description:
Added a fast path for small inputs (≤ one chunk): compresses the whole input in a single step, avoiding the main loop and extra allocations.
It doesn't look like there is anything special happening for this case. It would have been same (cost-wise) even if you didn't have a special case for this. The while
loop would have only run once.
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.
Agree.
| 1 byte | ~533 ns | ~1.6 MiB/s | | ||
| 1 KiB | ~597 ns | ~1.5 GiB/s | | ||
| 1 MiB | ~42 us | ~22.1 GiB/s | | ||
- No significant regressions or improvements detected. |
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.
nit: I think you could remove this line.
|-------------|--------------------|--------------------| | ||
| 1 byte | ~533 ns | ~1.6 MiB/s | | ||
| 1 KiB | ~597 ns | ~1.5 GiB/s | | ||
| 1 MiB | ~42 us | ~22.1 GiB/s | |
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.
It would be good to identify what size would better represent the actual scenario.
let mut group = criterion.benchmark_group("lz4_compression"); | ||
|
||
for size in [1, 1024, 1024 * 1024] { | ||
let data = setup_test_data(size); |
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.
We should consider adding a benchmark for OTLP data as well.
Changes
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes