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! |
| } | ||
|
|
||
| /// 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.
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.
This one I am rather sure should be Acquire, as it guards get_unchecked accesses to the value.
Lines 864 to 867 in b57c774
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.
| 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.
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+ |
|
Canceled. |
|
bors r+ |
|
Build succeeded: |
This is primaraliy to enable #177 for pl as well