-
Notifications
You must be signed in to change notification settings - Fork 110
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
replace parking_lot with parking_lot_core #178
Conversation
@pitdicker if by any chance you have resources and desire to make a review here, that'd be helpful! No worries if you don't! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider setting up loupe
to actually verify the atomics?
} | ||
|
||
/// Safety: synchronizes with store to value via Release/Acquire. | ||
#[inline] | ||
pub(crate) fn is_initialized(&self) -> bool { | ||
self.is_initialized.load(Ordering::Acquire) | ||
self.state.load(Ordering::Acquire) == COMPLETE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably be a plain Relaxed
load.
The use of this API can only ever be used in an informational manner (it is 100% racy no matter how the returned value is used…) so it doesn't matter if it spuriously returns false for a little while when the once cell is already initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I am rather sure should be Acquire
, as it guards get_unchecked
accesses to the value.
Lines 864 to 867 in b57c774
pub fn get(&self) -> Option<&T> { | |
if self.0.is_initialized() { | |
// Safe b/c value is initialized. | |
Some(unsafe { self.get_unchecked() }) |
src/imp_pl.rs
Outdated
let key = state as *const _ as usize; | ||
loop { | ||
let exchange = | ||
state.compare_exchange(INCOMPLETE, RUNNING, Ordering::Acquire, Ordering::Acquire); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state.compare_exchange(INCOMPLETE, RUNNING, Ordering::Acquire, Ordering::Acquire); | |
state.compare_exchange(INCOMPLETE, RUNNING, Ordering::Acquire, Ordering::Relaxed); |
You could also consider compare_exchange_weak
, though I don't know if there's any benefit in doing so since you will need to continue
on Err(INCOMPLETE)
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this I think also needs to be acquire, otherwise Err(COMPLETE) => return,
branch won't synchronize-with the corresponding Release store, and subsequent get_unchecked
would read garbage. Both cases are great to check if loom works though!
Kudos to @Ekleog for finding it!
loupe meaning loom? Yeah, I did some manual hacking with loom, and it indeed didn't complain about the current impl, and did complain about relaxed. Though, pushing that to CI is a bit challenging, as it doesn't support const-fn construction of atomics, something we rely on. Need to think about a nice way to work-around that... |
bors r+ |
178: replace parking_lot with parking_lot_core r=matklad a=matklad This is primaraliy to enable #177 for pl as well Co-authored-by: Aleksey Kladov <[email protected]>
Canceled. |
bors r+ |
Build succeeded: |
This is primaraliy to enable #177 for pl as well