Add OnceNonZeroUsize::get_unchecked.

[briansmith]

Each call to OnceNonZeroUsize::get() results in an atomic load as the compiler will not cache and reuse the result of an atomic load.

Provide an analogous get_unchecked() that uses a non-atomic load. The compiler will be able to cache and reuse the result to avoid redundant loads.

[briansmith]

Wow. AtomicUsize::as_ptr() wasn't added until Rust 1.70; see
https://doc.rust-lang.org/std/sync/atomic/struct.AtomicUsize.html#method.as_ptr.

[matklad]

It's fine to bump rust-version to 1.70! We support at least 8 past versions!

[briansmith]

The comment needs to be updated to directly address this, and probably other aspects of the memory model: "The most important aspect of this model is that data races are undefined behavior. A data race is defined as conflicting non-synchronized accesses where at least one of the accesses is non-atomic. Here, accesses are conflicting if they affect overlapping regions of memory and at least one of them is a write. They are non-synchronized if neither of them happens-before the other, according to the happens-before order of the memory model." (from https://doc.rust-lang.org/std/sync/atomic/index.html).

[briansmith]

It's fine to bump rust-version to 1.70! We support at least 8 past versions!

My project doesn't yet, though! :)

[matklad]

My project doesn't yet, though! :)

I am fine with this PR applying some work-around to get it work on older compilers, but, in general no promises that future versions of OnceCell will support 1.70.0! While I won't proactively bump to stable-8 just because, I will bump as soon as compat becomes inconvenient

[briansmith]

Comparing https://doc.rust-lang.org/1.74.0/core/sync/atomic/index.html and https://doc.rust-lang.org/1.75.0/core/sync/atomic/index.html shows that 1.75.0 may be the earliest version where we can fully reason about atomics and UB, as that was the first version that attempted to document the memory model for them.

[briansmith]

Also, https://doc.rust-lang.org/1.83.0/core/sync/atomic/index.html is the first version that talks about conflicting accesses. It says

The most important aspect of this model is that data races are undefined behavior. A data race is defined as conflicting non-synchronized accesses where at least one of the accesses is non-atomic. Here, accesses are conflicting if they affect overlapping regions of memory and at least one of them is a write. They are non-synchronized if neither of them happens-before the other, according to the happens-before order of the memory model.

That leads to the question, is a compare_exchange considered a "write" if it doesn't succeed in changing the value? It isn't clear.

[briansmith]

rust-lang/rust#136669

[briansmith]

According to https://doc.rust-lang.org/1.60.0/core/sync/atomic/struct.AtomicUsize.html, AtomicUsize is guaranteed to have the same layout and bit validity as usize even as far back as 1.60 (maybe further, didn't check), which is OnceCell's MSRV.

In later documentation, it is clarified that the alignment of AtomicUsize may be larger (equal to its size) on targets where usize has lower alignment. Thus I think we can assume alignof::<AtomicUsize>() >= alignof::<usize>() (and we can check this with a const assertion). Thus, I believe we have enough guaranteed to implement a polyfill for as_ptr().

[matklad]

Yes, I agree with this reasoning, it should be valid to cast a pointer to atomic usize to a pointer to a normal usize

[briansmith]

Just FYI, I was able to implement a workaround to deal with the added reloading when switching from spin::Once::get_unchecked to OnceNonZeroUsize::get(). Basically my calls to get() were done in batches, and I introduced an intermediate variable that held the loaded value, and then did the batch of checks on the new intermediate value, instead of calling get() over and over again.

Nonetheless, I am open to continuing to work on this.

[briansmith]

rust-lang/rust#138000 proposes changing the documentation for a future Rust release. However, the question will be whether we have to wait until Rust 1.87 or whatever to rely on that.

[RalfJung]

Note that that is a docs-only change. So it does apply retroactively, in a sense.

However it'd be good to wait until it actually lands, just to be sure. (In theory it could get backed out again but that seems unlikely.)

[briansmith]

I have updated this PR based on the above comments (thanks RalfJung).

[briansmith]

I filed a follow-up at rust-lang/rust#138246.

Also, we still have the issue that as_ptr() is not implemented until Rust 1.70. Are we confident about casting the address of the AtomicUsize to a *const usize for the polyfill?

[briansmith]

Note that the MSRV check is failing because of the use of as_ptr:

error[E0599]: no method named `as_ptr` found for struct `AtomicUsize` in the current scope
    --> src/race.rs:61:28
     |
  61 |         let p = self.inner.as_ptr();
     |                            ^^^^^^ method not found in `AtomicUsize`
  
  For more information about this error, try `rustc --explain E0599`.

Did you intend to merge this?

[matklad]

Life without bors is hard! I did intend to merge this, but I certainly hoped that tests would not fail!

I think it's fine to use 1.70 as MSRV, and I'd be willing to accept the patch lowering it via replacing as_ptr with a cast, which I am sure should be fine!