fix(tasks): allocation tracker prevent counter overflow #13085
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@camchenry If you think this is ridiculous overkill, feel free to close this. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a potential counter overflow issue in the allocation tracker by introducing an AtomicCounter type that uses saturating addition to prevent wrapping when counters reach usize::MAX. While this scenario is highly unlikely on 64-bit systems, it could potentially occur on 32-bit systems.
- Introduces a new
AtomicCounterwrapper type with saturating arithmetic - Replaces direct
AtomicUsizeusage with the new counter type - Updates all allocation tracking calls to use the safer increment method
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@overlookmotel I think it's a fair fix. Performance for this task isn't a high priority and it's fast enough for now. |
Merge activity
|
Allocation tracker did not deal with possibility that counters could reach `usize::MAX`. In this case, addition would wrap around back to 0. This problem was pointed out by @aapoalas in #13043 (review). Introduce an `AtomicCounter` type which uses saturating addition, to prevent this happening. Reaching such a high allocation count is highly unlikely on 64-bit systems, but might just about be possible on 32-bit. Maybe this is overkill, but since `cargo allocs` is just an internal tool, performance is not the highest priority. So I figure on balance probably better to handle this edge case. It doesn't seem to hurt perf much. ``` # Before % hyperfine -i --warmup 3 'cargo allocs' Benchmark 1: cargo allocs Time (mean ± σ): 299.9 ms ± 2.4 ms [User: 132.8 ms, System: 31.0 ms] Range (min … max): 297.6 ms … 303.9 ms 10 runs # After % hyperfine -i --warmup 3 'cargo allocs' Benchmark 1: cargo allocs Time (mean ± σ): 301.3 ms ± 3.4 ms [User: 133.3 ms, System: 31.5 ms] Range (min … max): 297.8 ms … 310.0 ms 10 runs ``` Note: The same problem in arena allocation tracker is addressed in #13043.
graphite-app
bot
force-pushed
the
08-14-fix_tasks_allocation_tracker_prevent_counter_overflow
branch
from
254a7cf to
8c141e5
Compare
graphite-app
bot
deleted the
08-14-fix_tasks_allocation_tracker_prevent_counter_overflow
branch
Allocation tracker did not deal with possibility that counters could reach
usize::MAX. In this case, addition would wrap around back to 0.This problem was pointed out by @aapoalas in #13043 (review).
Introduce an
AtomicCountertype which uses saturating addition, to prevent this happening.Reaching such a high allocation count is highly unlikely on 64-bit systems, but might just about be possible on 32-bit.
Maybe this is overkill, but since
cargo allocsis just an internal tool, performance is not the highest priority. So I figure on balance probably better to handle this edge case. It doesn't seem to hurt perf much.Note: The same problem in arena allocation tracker is addressed in #13043.