Implement wide sequence lock #359
Conversation
|
https://travis-ci.org/crossbeam-rs/crossbeam/jobs/523343511#L464 Tempting to bump the minimum Rust version.. @stjepang @Vtec234 what do you think? |
|
Oh, I like this change! Will take another look and do a proper review today :) Regarding bumping the minimum Rust version... don't worry about it, that's fine. |
jeehoonkang
force-pushed
the
seq-lock-wide
branch
2 times, most recently
from
46eb3f7
to
be50d6b
Compare
There was a problem hiding this comment.
This looks great, thank you!
I wonder - is the Rust version bumped only because of const fn? If so, you could also work around that by replacing SeqLock::new() with an initializer constant SEQ_LOCK_INIT defined like so:
pub const SEQ_LOCK_INIT: SeqLock = SeqLock { ... };
|
iOS has a very weird scheduler where this issue is more likely to crop up under certain scenarios (see Update: A Warning for iOS). Thanks for the fix! |
|
Just so we don't forget, in all the other places where we have atomic timestamps, we should use
But that can come later in another PR. It will also require Rust 1.34 minimum. |
crossbeam-utils/src/atomic/mod.rs
Outdated
| @@ -1,5 +1,16 @@ | |||
| //! Atomic types. | |||
|
|
|||
| cfg_if! { | |||
| // TODO(jeehoonkang): want to express `target_pointer_width >= "64"`, but it's not expressible | |||
There was a problem hiding this comment.
In principle Rust also supports a 16-bit target for which two AtomicUsizes would make a 32-bit counter. I think this can safely be ignored as it's very primitive hardware, but mentioning it here for completeness. There are no 128-bit targets currently as far as I can tell from a quick search.
There was a problem hiding this comment.
I added some documentation: https://github.com/crossbeam-rs/crossbeam/pull/359/files#diff-7e838416d7e6b94eeac80063d0b6a446R10
| /// This method should be called before optimistic reads. | ||
| #[inline] | ||
| pub fn optimistic_read(&self) -> Option<(usize, usize)> { | ||
| let state_hi = self.state_hi.load(Ordering::Acquire); |
There was a problem hiding this comment.
Any reason why this is Acquire while other loads of state_hi are Relaxed?
There was a problem hiding this comment.
I left some explanation on this and other orderings.
jeehoonkang
force-pushed
the
seq-lock-wide
branch
from
be50d6b
to
f8bee40
Compare
|
@stjepang @Vtec234 Thank you for your comments. I addressed all of them. I think @stjepang's comment on |
jeehoonkang
force-pushed
the
seq-lock-wide
branch
from
2e65130
to
6b6fb0e
Compare
|
@stjepang any opinions on this PR? |
|
ping? @stjepang |
jeehoonkang
force-pushed
the
seq-lock-wide
branch
from
6b6fb0e
to
5ed65f2
Compare
jeehoonkang
force-pushed
the
seq-lock-wide
branch
from
5ed65f2
to
e870d0c
Compare
|
I'm trying to merge it, as it went through the test of time... bors r+ |
359: Implement wide sequence lock r=jeehoonkang a=jeehoonkang In #345, @mtak- discusses the problem of using `AtomicUsize` as a wrapping counter: wrapping takes only ~10 seconds, which will render the Crossbeam implementation of sequence lock wrong. Using `AtomicU64` was proposed as a solution, but it is compiled to an expensive compare-exchange instruction in 32-bit x86 architectures. Here is another solution: using two `AtomicUsize` as a counter (`seq_lock_wide.rs`). Observation is that we don't need the full atomicity of `AtomicU64` for the counter of sequence lock. We just can use two `AtomicUsize` variables and synchronize accesses to them properly. r? @stjepang closes #345 Co-authored-by: Jeehoon Kang <[email protected]>
Build succeeded |
In #345, @mtak- discusses the problem of using
AtomicUsizeas a wrapping counter: wrapping takes only ~10 seconds, which will render the Crossbeam implementation of sequence lock wrong. UsingAtomicU64was proposed as a solution, but it is compiled to an expensive compare-exchange instruction in 32-bit x86 architectures.Here is another solution: using two
AtomicUsizeas a counter (seq_lock_wide.rs). Observation is that we don't need the full atomicity ofAtomicU64for the counter of sequence lock. We just can use twoAtomicUsizevariables and synchronize accesses to them properly.r? @stjepang
closes #345