Potential to weaken ordering for success case of `compare_exhange`s in `race` module

[Imberflur]

For example, the compare exchange for OnceNonZeroUsize uses AcqRel for the success case:

once_cell/src/race.rs

Line 100 in d706539

self.inner.compare_exchange(0, val, Ordering::AcqRel, Ordering::Acquire);

The Acquire portion of the AcqRel here is for the load of 0, however there is no Release store of 0 so this will not synchronize with anything. Note that the construction of OnceNonZeroUsize still has a happens-before relationship to this since we have an &self reference. Thus, I think a Release ordering is sufficient here for the desired synchronization to be achieved (including taking this documentation into account):

 self.inner.compare_exchange(0, val, Ordering::Release, Ordering::Acquire);

I believe this applies to all uses of compare_exchange in this module.

---

(happy to make a PR if this looks reasonable)

[matklad]

This does look reasonable to me, yeah! Although, as usual, I am unnerved by the fact that we don't have any tools to check this (or maybe I am missing them?).

Could you send a PR? You can also adjust the Changelog and bump the patch version in Cargo.toml, so that we release this together with #219

[taiki-e]

Note that CAS failure ordering that is stronger than success ordering requires Rust 1.64+: rust-lang/rust#98383

[Imberflur]

Note that CAS failure ordering that is stronger than success ordering requires Rust 1.64+: rust-lang/rust#98383

Gah! 🙁

Although, as usual, I am unnerved by the fact that we don't have any tools to check this (or maybe I am missing them?).

The only thing that comes to my mind is testing with loom. I don't know if it is exactly what you are looking for though.

[briansmith]

Does anybody know of how the codegen would change such that this would result in better performance for users of the race module?

[briansmith]

I believe it is good to raise MSRV to 1.64 so we can do this. But it would be good to know what the real benefit would be.

[Imberflur]

FWIW I don't have any specific motivating case for this. I asked about this on the community discord and someone mentioned:

so in theory llvm can optimize it in some additional ways, but it doesn't in simple cases

on armv7 llvm avoids the trailing barrier on llsc success https://rust.godbolt.org/z/jEzaMTWar

code from godbolt link

use std::sync::atomic::*;
pub static X: AtomicU32 = AtomicU32::new(0);
#[unsafe(no_mangle)]
fn foo1() -> bool {
    X.compare_exchange(0, 1, Ordering::AcqRel, Ordering::Acquire).is_ok()
}

#[unsafe(no_mangle)]
fn bar1() -> bool {
    X.compare_exchange(0, 1, Ordering::Release, Ordering::Acquire).is_ok()
}

compiled for --target=armv7-unknown-linux-gnueabi

foo1:
        ldr     r1, .LCPI0_0
.LPC0_0:
        ldr     r1, [pc, r1]
        ldrex   r0, [r1]
        cmp     r0, #0
        bne     .LBB0_4
        dmb     ish
.LBB0_2:
        mov     r0, #1
        strex   r2, r0, [r1]
        cmp     r2, #0
        beq     .LBB0_5
        ldrex   r0, [r1]
        cmp     r0, #0
        beq     .LBB0_2
.LBB0_4:
        mov     r0, #0
        clrex
.LBB0_5:
        dmb     ish
        bx      lr
.LCPI0_0:
.Ltmp9:
        .long   example::X(GOT_PREL)-((.LPC0_0+8)-.Ltmp9)

bar1:
        ldr     r1, .LCPI1_0
.LPC1_0:
        ldr     r1, [pc, r1]
        ldrex   r0, [r1]
        cmp     r0, #0
        bne     .LBB1_4
        dmb     ish
.LBB1_2:
        mov     r0, #1
        strex   r2, r0, [r1]
        cmp     r2, #0
        bxeq    lr
        ldrex   r0, [r1]
        cmp     r0, #0
        beq     .LBB1_2
.LBB1_4:
        clrex

in particular, note the bxeq lr vs beq .LBB0_5 (which is dmb ish + bx lr)

[briansmith]

Since the MSRV is 1.65 now, I submitted PR #278 to start this.

[briansmith]

I updated PR #278 and I think it is now ready for review. But note that there is unlikely to be any significant benefit from doing this, since it is all happening in the cold path for users of get_or_init/get_or_try_init.