-
Notifications
You must be signed in to change notification settings - Fork 409
Simplify and fix AtomicCounter #3302
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.
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
Changes from 1 commit
758747a
6e340c4
2ab133d
1c2bd09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,44 @@ | ||
//! A simple atomic counter that uses AtomicUsize to give a u64 counter. | ||
//! A simple atomic counter that uses mutexes if the platform doesn't support atomic u64s. | ||
|
||
#[cfg(not(any(target_pointer_width = "32", target_pointer_width = "64")))] | ||
compile_error!("We need at least 32-bit pointers for atomic counter (and to have enough memory to run LDK)"); | ||
#[cfg(target_has_atomic = "64")] | ||
use core::sync::atomic::{AtomicU64, Ordering}; | ||
#[cfg(not(target_has_atomic = "64"))] | ||
use crate::sync::Mutex; | ||
|
||
use core::sync::atomic::{AtomicUsize, Ordering}; | ||
|
||
#[derive(Debug)] | ||
pub(crate) struct AtomicCounter { | ||
// Usize needs to be at least 32 bits to avoid overflowing both low and high. If usize is 64 | ||
// bits we will never realistically count into high: | ||
counter_low: AtomicUsize, | ||
counter_high: AtomicUsize, | ||
#[cfg(target_has_atomic = "64")] | ||
counter: AtomicU64, | ||
#[cfg(not(target_has_atomic = "64"))] | ||
counter: Mutex<u64>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the intention is to produce some unique values that maybe don't need to be sequential you can still have a sensible lock-free implementation. Roughly like this: let mut low = self.low.load(Relaxed);
let mut high = self.high.load(Relaxed);
loop {
let new_low = if low == u32::MAX {
// don't use fetch_add to avoid incrementing high by more than 1
if let Err(new) = self.high.compare_exchange(high, high + 1, Relaxed, Relaxed) {
high = new;
}
0
} else {
low + 1
}
// FTR this cannot be weak
match self.low.compare_exchange(low, new_low, Relaxed, Relaxed) {
Ok(_) => break,
Err(new) => low = new,
}
}
u64::from(high) << 32 | u64::from(low) This assumes that a thread doesn't get scheduled-out after incrementing high for so long that other thread(s) manage to increment the counter by 2^32, which I think is a reasonable assumption. There's still a chance that |
||
} | ||
|
||
impl AtomicCounter { | ||
pub(crate) fn new() -> Self { | ||
Self { | ||
counter_low: AtomicUsize::new(0), | ||
counter_high: AtomicUsize::new(0), | ||
#[cfg(target_has_atomic = "64")] | ||
counter: AtomicU64::new(0), | ||
#[cfg(not(target_has_atomic = "64"))] | ||
counter: Mutex::new(0), | ||
} | ||
} | ||
pub(crate) fn get_increment(&self) -> u64 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Preexisting, this naming seems a bit confusing as we're actually not returning the incremented counter. Maybe the field should be called |
||
let low = self.counter_low.fetch_add(1, Ordering::AcqRel) as u64; | ||
let high = if low == 0 { | ||
self.counter_high.fetch_add(1, Ordering::AcqRel) as u64 | ||
} else { | ||
self.counter_high.load(Ordering::Acquire) as u64 | ||
}; | ||
(high << 32) | low | ||
#[cfg(target_has_atomic = "64")] { | ||
self.counter.fetch_add(1, Ordering::AcqRel) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have |
||
} | ||
#[cfg(not(target_has_atomic = "64"))] { | ||
let mut mtx = self.counter.lock().unwrap(); | ||
*mtx += 1; | ||
*mtx - 1 | ||
} | ||
} | ||
#[cfg(test)] | ||
pub(crate) fn set_counter(&self, count: u64) { | ||
#[cfg(target_has_atomic = "64")] { | ||
self.counter.store(count, Ordering::Release); | ||
} | ||
#[cfg(not(target_has_atomic = "64"))] { | ||
let mut mtx = self.counter.lock().unwrap(); | ||
*mtx = count; | ||
} | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.