chore: bump alloc_tracker from 0.6.0 to 0.7.0 - #568
Conversation
Bumps [alloc_tracker](https://github.com/folo-rs/folo) from 0.6.0 to 0.7.0. - [Release notes](https://github.com/folo-rs/folo/releases) - [Changelog](https://github.com/folo-rs/folo/blob/main/docs/release-automation.md) - [Commits](folo-rs/folo@alloc_tracker-v0.6.0...alloc_tracker-v0.7.0) --- updated-dependencies: - dependency-name: alloc_tracker dependency-version: 0.7.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
dependabot
Bot
added
dependencies
dependabot
Bot
added
dependencies
|
@copilot, do a proper upgrade |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (91.8%) is below the target coverage (100.0%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## main #568 +/- ##
=========================================
- Coverage 100.0% 91.8% -8.2%
=========================================
Files 470 430 -40
Lines 45251 36326 -8925
=========================================
- Hits 45251 33365 -11886
- Misses 0 2961 +2961
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| BytesBuf::new, | ||
| |buf| { | ||
| let _span = allocs_op.measure_thread(); | ||
| let _span = allocs_op.measure_thread().iterations(1); |
There was a problem hiding this comment.
The documentation of alloc_tracker specifically mentions that iterations should be batched. Forcing us to call .iterations(N) is the breaking change, to help highlight when we are doing iterations one by one, which includes huge (relatively) measurement overhead.
The proper upgrade would also fix the pattern we use to iter_custom() so we batch all our iterations for one Criterion sample, instead of measuring microbenchmark iterations one by one.
There was a problem hiding this comment.
@copilot there are still a lot of iterations(1) remaining in the diff! Those are all anti-patterns to fix, you only addressed a few of them. Also merge latest origin/main and resolve conflicts.
There was a problem hiding this comment.
[Copilot speaking]
F004 — [P2] Batch the remaining multitude allocation measurements per Criterion sample
Status: Follow-up
Impacted locations:
crates/multitude/benches/multitude_record_batch.rs:45-63crates/multitude/benches/multitude_record_batch.rs:94-106crates/multitude/benches/multitude_record_batch.rs:180-212
Ten allocation-tracked benchmarks still use Bencher::iter and create a measure_thread().iterations(1) span inside every individual benchmark iteration. Span creation, counter capture, aggregation, and drop are therefore paid and timed once per workload operation instead of once per Criterion sample.
alloc_tracker 0.7 documents that measurement overhead is on the order of tens of nanoseconds and states that batching multiple iterations in one sample is crucial to amortize it. The rest of this migration follows that iter_custom pattern, but the decode, reuse, and refresh-workload cases in this file remain on the one-span-per-iteration pattern.
Reproducible reasoning:
Bencher::iterinvokes the closure once per timed iteration.- Each listed closure opens and drops a tracker span around one workload call.
.iterations(1)satisfies the new API requirement but does not amortize measurement overhead.- The upstream canonical pattern opens one span in
iter_custom, sets.iterations(iters), and runs the operationiterstimes.
Consequence: These timings include a fixed tracker cost for every operation and are not methodologically comparable with the correctly batched benchmarks elsewhere in the PR. The overhead can conceal or exaggerate workload deltas, especially for the shorter reuse and refresh operations.
Recommended action: Convert all ten cases to iter_custom, open one tracker span with .iterations(iters) for the sample, and execute the workload iters times inside the timed loop. Preserve the current mutable-state semantics by keeping each state object outside the repeated closure.
References:
- alloc_tracker 0.7 benchmarking and overhead guidance
docs/benchmarks.md, “Benchmark design”
Existing thread: Follow-up to discussion_r3603199008, which requested removal of all remaining .iterations(1) benchmark patterns and was marked addressed.
| /// Times a Criterion sample after preparing per-iteration inputs ahead of time. | ||
| pub fn time_sample_with_inputs<T, R>(mut setup: impl FnMut() -> T, mut bench: impl FnMut(T) -> R) -> impl FnMut(u64) -> Duration { | ||
| move |iters| { | ||
| let inputs = (0..iters).map(|_| setup()).collect::<Vec<_>>(); |
There was a problem hiding this comment.
@copilot this is not right, we are constructing the inputs list ouitside the timed section; but what if we are already inside an alloc_tracker or all_the_time scope? In this case we would be also measuring the input creation, which is incorrect.
A correct approach takes a pre-prepared inputs vec here already, so the inputs can be prepared before entering any measurement scope.
Furthermore, I observe that not every benchmark that could use these helpers does so - some benchmarks still do manual timing. That seems unnecessary - unless there is some special case logic, let's use these helpers everywhere for unform and unified timing logic.
There was a problem hiding this comment.
[Copilot speaking]
F001 — [P1] Preserve prepared input and output lifetimes outside measured samples
Status: Follow-up
Impacted locations:
crates/benchmarking/src/lib.rs:19-25crates/bytesbuf/benches/buf.rs:109-338crates/bytesbuf/benches/bytesbuf_vs_bytes.rs:52-570crates/bytesbuf/benches/global_pool.rs:74-113crates/bytesbuf/benches/view.rs:124-285
time_sample_with_inputs takes the prepared Vec<T> by value, starts the timer, and then consumes the vector with for input in inputs. Each closure argument and each returned value is therefore dropped inside the timed loop, and the vector's owning iterator releases its backing allocation before start.elapsed() is evaluated. All 61 changed call sites create their alloc_tracker span before calling the helper, so the same teardown also occurs while the measurement span is active.
This does not preserve the boundary of the replaced benchmark implementations. The previous buf.rs and view.rs cases used Criterion's iter_batched_ref, and the previous bytesbuf_vs_bytes.rs loops iterated over caller-owned vectors by mutable reference. In both forms, prepared fixtures remained alive until after the routine was timed. Criterion's batched implementation also retains outputs until measurement ends.
Reproducible reasoning:
- Follow any prepared vector, such as the cloned multi-span views in
bytesbuf_vs_bytes.rs, intotime_sample_with_inputs. Instant::now()runs before the consumingforloop.- The closure owns each input. Unless the input is transferred into the result, its remaining state is dropped when the closure invocation ends.
_ = black_box(bench(input));drops nontrivial results, such asBytesVieworMemoryGuard, before the next iteration.- The
Vec::IntoIterand backing allocation are dropped beforestart.elapsed(). - The caller's tracker span is dropped only after the helper returns.
Consequence: Benchmarks for elementary reads, writes, peeks, span consumption, construction, and cold pool operations include reference-count release, span or pool cleanup, output destruction, and setup-container teardown. These costs vary by input type and span count and can dominate the operation named by the benchmark, making results non-comparable with the previous benchmarks.
Recommended action: Combine the lifetime fix with bounded batching. For each bounded chunk, prepare inputs before opening the tracker span, retain inputs and nontrivial outputs while timing the chunk, capture elapsed time, close the tracker span, and only then drop the fixtures. Use borrowed inputs for benchmarks that previously used iter_batched_ref; where consuming ownership is intrinsic to the API, move inputs into the operation but retain outputs until measurement closes. Accumulate the chunk durations to satisfy Criterion's requested iteration count.
References:
docs/benchmarks.md, “Benchmark design”crates/bytesbuf/AGENTS.md, “Spans per buffer/view” and “Inline spans are performance-sensitive”- Criterion 0.8.1
Bencher::iter_batched_ref - alloc_tracker 0.7
ThreadSpan
Existing thread: Follow-up to discussion_r3680525615. Moving input construction before the measurement scopes addressed setup, but input/output teardown remains inside them.
| BytesBuf::new() | ||
| b.iter_custom(|iters| { | ||
| let _span = new_allocs.measure_thread().iterations(iters); | ||
| time_sample(BytesBuf::new)(iters) |
There was a problem hiding this comment.
@copilot what's the point of time_sample returning a closure if we always immediately execute the closure? It seems overcomplicated design, no? Find opportunities to simplify - maybe we should be executing the closure-arguments already inside the time function?
There was a problem hiding this comment.
Simplified in a393ecc - time_sample now takes iters directly instead of returning a closure, and the benchmark call sites were updated to use the simpler API.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
crates/bytesbuf/benches/global_pool.rs:89
- This benchmark buffers
itersfreshly-createdGlobalPoolvalues into aVecbefore timing. Ifitersis large, that can cause very high peak memory and/or OOM before the sample even starts. Chunking the setup keeps memory bounded while still excluding setup time from the measurement.
b.iter_custom(|iters| {
let inputs = (0..iters).map(|_| GlobalPool::new()).collect::<Vec<_>>();
let _span = allocs_op.measure_thread().iterations(iters);
crates/bytesbuf/benches/global_pool.rs:111
- Collecting
iterscoldGlobalPoolinstances into aVeccan blow up memory usage when Criterion selects large iteration counts. A chunked setup keeps peak memory bounded while still keeping the setup out of the timed region.
b.iter_custom(|iters| {
let inputs = (0..iters).map(|_| GlobalPool::new()).collect::<Vec<_>>();
let _span = allocs_op.measure_thread().iterations(iters);
crates/bytesbuf/benches/global_pool.rs:77
inputsis collected into aVecsized toiters. Criterion can choose very largeitersfor fast benchmarks, so this can allocate a huge vector (and potentially OOM) before each sample. Consider chunking the setup so peak memory is bounded while still keeping setup out of the timed region.
This issue also appears in the following locations of the same file:
- line 87
- line 109
b.iter_custom(|iters| {
let inputs = (0..iters).map(|_| GlobalPool::new()).collect::<Vec<_>>();
let _span = allocs_op.measure_thread().iterations(iters);
sandersaares
left a comment
sandersaares
left a comment
There was a problem hiding this comment.
[Copilot speaking]
Submitted 3 findings: 1 P1 and 2 P2. All 3 are follow-ups to existing review feedback; 2 continue existing line threads and 1 is a new line comment following suppressed review feedback.
|
@copilot address all new feedback and ensure that the branch actually builds & tests (there are failed GitHub checks - figure out what happened and fix everything that caused them to fail) |
Addressed in ab4791f. I fixed the remaining alloc-tracker batching issues in |
Bumps alloc_tracker from 0.6.0 to 0.7.0.
Commits
bee10e4Release4394bd9Simplify tracker span API and condense docs (#342)6935825Make cargo-bench-history collect tests hermetic against CARGO_TARGET_DIR (#341)d40a479Pin Cargo tool versions in constants.env; bump gungraun to 0.19.3 (#340)e630a6fAlign install-tools with CI, remove divergent tooling paths (#339)c646b29Verify cargo-semver-checks is operational64859ceAzurite optionalb104e4bDocument measurement overhead in alloc_tracker, align all_the_time (#338)b8cf5c2Attach discriminant-set filter flags to analyze reports (#337)0437b1eRelax workflow timeouts for cold-cache setup-environment (#336)Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)