rust-lang / log

Logging implementation for Rust
https://docs.rs/log
Apache License 2.0
2.12k stars 248 forks source link

Use `Acquire` ordering for initialization check #610

Closed AngelicosPhosphoros closed 6 months ago

AngelicosPhosphoros commented 6 months ago

Reasoning is provided in a comment.

AngelicosPhosphoros commented 6 months ago

This generates different code than current for aarch64-apple-darwin, allowing it to do more instruction reordering for unrelated memory accesses.

Thomasdezeeuw commented 6 months ago

It seems the ordering was changed from Acquire to SeqCst in commit 28ce2f8b963e602da385c4c9698306ad1b1c8ae9, specifically: https://github.com/rust-lang/log/commit/28ce2f8b963e602da385c4c9698306ad1b1c8ae9#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R552, from pr #12.

@sfackler you wrote the code, but the commit message doesn't mention why. Do you know of a good reason not to merge this?

AngelicosPhosphoros commented 6 months ago

@Thomasdezeeuw It seems that @sfackler wouldn't respond.

Anyway, this old code was very different (e.g. it uses refcounting, and with too strong orderings too, Arc, for example, uses mostly acquire and relaxed orderings).

I wrote the comment in code that explains why Acquire ordering is correct and enough. With atomics, in general, it is preferable to use weakest as possible but still correct ordering because stronger ordering does make whole CPU slower, especially, if called frequently.

sfackler commented 6 months ago

If I remember correctly, it was Alex’s general preference to use stronger orderings to avoid potential correctness issues. I wouldn’t read to much into it if people are confident that a weaker ordering is sufficient.

Thomasdezeeuw commented 6 months ago

Thanks @sfackler

Then I think we can merge this.

Thomasdezeeuw commented 6 months ago

Thanks @AngelicosPhosphoros