fix: [Geneva Exporter] Add LZ4 compression benchmark #284
Conversation
lalitb
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #284 +/- ##
=======================================
- Coverage 48.0% 47.9% -0.1%
=======================================
Files 60 61 +1
Lines 8576 8602 +26
=======================================
+ Hits 4121 4126 +5
- Misses 4455 4476 +21 ☔ 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 |
| - No significant regressions or improvements detected. | ||
| - Machine: WSL2, Linux 6.6.36, AMD EPYC 7763 64-core (8 cores visible), 62GiB RAM, 16GiB swap. | ||
| */ | ||
| #[test] |
There was a problem hiding this comment.
This setup looks a bit unusual. Does this mean that we would be running this benchmark in the CI test? I think we only want to run this with cargo bench. Could we follow something similar to how we have added benchmarks in other methods?
There was a problem hiding this comment.
This is testing the crate private code, which won't be possible under benches/*. Let me see if this is something which can be re-exported as public specifically for benchmark, and then add it there. Else, will make it to be ignored with cargo test.
There was a problem hiding this comment.
Got it. I would suggest using [[bench]] target in that case and keep the benchmarks under src/. That way you can access private methods of the crate.
There was a problem hiding this comment.
As discussed offline, have made to be ignored by cargo test.
| #[test] | ||
| fn lz4_benchmark() { | ||
| let mut criterion = Criterion::default() | ||
| .sample_size(100) |
There was a problem hiding this comment.
Any specific reason to be changing the default sample size and measurement time?
There was a problem hiding this comment.
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.
I don't follow. We had removed all temporary buffer allocation. Why are we introducing it again?
There was a problem hiding this comment.
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.
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.
lalitb
changed the title
Changes
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes