fix: make ReferenceCount thread-safe with atomic operations#261
fix: make ReferenceCount thread-safe with atomic operations#261lodekeeper-z wants to merge 1 commit intoChainSafe:mainfrom
Conversation
- Switch _ref_count from plain usize to std.atomic.Value(usize) - Use fetchAdd(.monotonic) for acquire, fetchSub(.acq_rel) for release - Fix clone() bug: was missing allocator argument (would not compile if called) - Remove stale TODO comment about switching to std.atomic The ReferenceCount utility is used by EffectiveBalanceIncrements, SyncCommitteeCache, and EpochShuffling caches. Without atomics, concurrent acquire/release from multiple threads (e.g. via NAPI bindings with multi-threaded BLS) would be a data race. 🤖 Generated with AI assistance
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a data race condition in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly implements thread-safety for ReferenceCount by using atomic operations, which is a crucial improvement given its use in shared caches. The memory ordering choices for acquire and release are appropriate and follow standard practices for atomic reference counting. The bug fix in the clone function is also a good catch. I've included a couple of suggestions to further align the code with the repository's style guide, focusing on using explicitly-sized integers and adding assertions for increased robustness.
| _ref_count: std.atomic.Value(usize), | ||
| instance: T, | ||
|
|
||
| pub fn init(allocator: Allocator, instance: T) !*@This() { | ||
| const ptr = try allocator.create(@This()); | ||
| errdefer allocator.destroy(ptr); | ||
| ptr.* = .{ | ||
| .allocator = allocator, | ||
| ._ref_count = 1, | ||
| ._ref_count = std.atomic.Value(usize).init(1), |
There was a problem hiding this comment.
According to the repository style guide (L44), architecture-specific types like usize should be avoided in favor of explicitly-sized types. Please consider using u32 for the reference counter, as it's unlikely to be exceeded and improves portability.
_ref_count: std.atomic.Value(u32),
instance: T,
pub fn init(allocator: Allocator, instance: T) !*@This() {
const ptr = try allocator.create(@This());
errdefer allocator.destroy(ptr);
ptr.* = .{
.allocator = allocator,
._ref_count = std.atomic.Value(u32).init(1),
References
- Use explicitly-sized types like
u32for everything, avoid architecture-specificusize. (link)
| pub fn acquire(self: *@This()) *@This() { | ||
| self._ref_count += 1; | ||
| _ = self._ref_count.fetchAdd(1, .monotonic); | ||
| return self; | ||
| } | ||
|
|
||
| pub fn release(self: *@This()) void { | ||
| self._ref_count -= 1; | ||
| if (self._ref_count == 0) { | ||
| const prev = self._ref_count.fetchSub(1, .acq_rel); | ||
| if (prev == 1) { | ||
| self.deinit(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The repository style guide (L51) recommends asserting pre/postconditions and invariants to improve safety. In acquire, you can assert against overflow, and in release, you can assert against underflow (which could indicate a double-free).
pub fn acquire(self: *@This()) *@This() {
const prev = self._ref_count.fetchAdd(1, .monotonic);
// Assert against overflow.
std.debug.assert(prev < @typeInfo(@TypeOf(prev)).Int.max);
return self;
}
pub fn release(self: *@This()) void {
const prev = self._ref_count.fetchSub(1, .acq_rel);
// Assert against underflow (e.g. double release).
std.debug.assert(prev > 0);
if (prev == 1) {
self.deinit();
}
}
References
- Assert all function arguments and return values, pre/postconditions and invariants. A function must not operate blindly on data it has not checked. The assertion density of the code must average a minimum of two assertions per function. (link)
|
There is already a PR #62 for this. |
|
Closing in favor of #62 which addresses the same issue and includes the rename to |
2 participants
Motivation
ReferenceCountuses plainusizefor its reference counter, which is a data race under concurrent access. This is used by:EffectiveBalanceIncrementsSyncCommitteeCacheEpochShufflingThese caches are accessed via
EpochCachewhich will be shared across threads when NAPI bindings are used with multi-threaded BLS verification.Changes
_ref_countfromusizetostd.atomic.Value(usize)fetchAdd(1, .monotonic)— no ordering needed for incrementfetchSub(1, .acq_rel)— ensures all prior writes are visible before deinitallocatorargument (compile error if ever called)The memory ordering follows the standard reference counting pattern (similar to Rust's
Arc).🤖 Generated with AI assistance