Skip to content

refactor(allocator): replace AtomicUsize with Cell<usize> in AllocationStats #13043

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 19, 2025
Merged

refactor(allocator): replace AtomicUsize with Cell<usize> in AllocationStats #13043

merged 1 commit into from
Aug 19, 2025

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Aug 13, 2025 •
edited
Loading

AllocationStats (introduced in #12555 and #12937) previously had to contain AtomicUsizes because Allocator was Sync. #13033 removed the Sync impl for Allocator, so now there's no need for synchronization in AllocationStats, and these fields can be Cell<usize> instead.

@github-actions github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Aug 13, 2025
This was referenced Aug 13, 2025
Merged
Merged
Merged
Merged
Merged
@overlookmotel Graphite App
Copy link
Member Author

overlookmotel commented Aug 13, 2025 •
edited
Loading

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@overlookmotel overlookmotel mentioned this pull request Aug 13, 2025
Merged
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 13, 2025 •
edited
Loading

CodSpeed Instrumentation Performance Report

Merging #13043 will not alter performance

Comparing 08-13-refactor_allocator_replace_atomicusize_with_cell_usize_in_allocationstats_ (93eaf5f) with 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ (cc067c2)

Summary

✅ 34 untouched benchmarks

@overlookmotel overlookmotel marked this pull request as ready for review August 13, 2025 00:45
@overlookmotel overlookmotel force-pushed the 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ branch from 76d7ef7 to 94907c8 Compare August 13, 2025 01:37
@overlookmotel overlookmotel force-pushed the 08-13-refactor_allocator_replace_atomicusize_with_cell_usize_in_allocationstats_ branch from 9f84d46 to 1367a1b Compare August 13, 2025 01:37
@graphite-app graphite-app bot force-pushed the 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ branch from 94907c8 to b85beb1 Compare August 13, 2025 11:54
@graphite-app graphite-app bot force-pushed the 08-13-refactor_allocator_replace_atomicusize_with_cell_usize_in_allocationstats_ branch from 1367a1b to 5e6a9e7 Compare August 13, 2025 11:54
aapoalas
aapoalas reviewed Aug 14, 2025
View reviewed changes
@overlookmotel overlookmotel mentioned this pull request Aug 14, 2025
Merged
graphite-app bot pushed a commit that referenced this pull request Aug 14, 2025
@overlookmotel
fix(tasks): allocation tracker prevent counter overflow (#13085)
8c141e5 
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.
@overlookmotel overlookmotel force-pushed the 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ branch from b85beb1 to fd22653 Compare August 18, 2025 23:55
@overlookmotel overlookmotel force-pushed the 08-13-refactor_allocator_replace_atomicusize_with_cell_usize_in_allocationstats_ branch from 8843a5a to 45bfbd0 Compare August 18, 2025 23:55
@overlookmotel overlookmotel mentioned this pull request Aug 18, 2025
Merged
@overlookmotel overlookmotel force-pushed the 08-13-refactor_allocator_replace_atomicusize_with_cell_usize_in_allocationstats_ branch from 45bfbd0 to 1e22ad0 Compare August 19, 2025 00:12
@overlookmotel overlookmotel force-pushed the 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ branch from fd22653 to 32c7632 Compare August 19, 2025 00:12
@overlookmotel
Copy link
Member Author

overlookmotel commented Aug 19, 2025

Thanks for reviewing Cam. Merging!

@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Aug 19, 2025
@overlookmotel Graphite App
Copy link
Member Author

overlookmotel commented Aug 19, 2025 •
edited by graphite-app bot
Loading

Merge activity

@overlookmotel
refactor(allocator): replace AtomicUsize with Cell<usize> in `All…
93eaf5f 
…ocationStats` (#13043)

`AllocationStats` (introduced in #12555 and #12937) previously had to contain `AtomicUsize`s because `Allocator` was `Sync`. #13033 removed the `Sync` impl for `Allocator`, so now there's no need for synchronization in `AllocationStats`, and these fields can be `Cell<usize>` instead.
@graphite-app graphite-app bot force-pushed the 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ branch from 32c7632 to cc067c2 Compare August 19, 2025 21:38
@graphite-app graphite-app bot force-pushed the 08-13-refactor_allocator_replace_atomicusize_with_cell_usize_in_allocationstats_ branch from 1e22ad0 to 93eaf5f Compare August 19, 2025 21:39
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Aug 19, 2025
Base automatically changed from 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ to main August 19, 2025 21:47
@graphite-app graphite-app bot merged commit 93eaf5f into main Aug 19, 2025
27 checks passed
@graphite-app graphite-app bot deleted the 08-13-refactor_allocator_replace_atomicusize_with_cell_usize_in_allocationstats_ branch August 19, 2025 21:48
@oxc-bot oxc-bot mentioned this pull request Aug 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@camc314 camc314 camc314 approved these changes

@Dunqing Dunqing Awaiting requested review from Dunqing

@Sysix Sysix Awaiting requested review from Sysix

+1 more reviewer

@aapoalas aapoalas aapoalas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@overlookmotel @camc314 @aapoalas