Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is it necessary to use Ordering::SeqCst in the log! ?? #509

Closed
relufi opened this issue Apr 29, 2022 · 5 comments
Closed

Is it necessary to use Ordering::SeqCst in the log! ?? #509

relufi opened this issue Apr 29, 2022 · 5 comments

Comments

@relufi
Copy link

relufi commented Apr 29, 2022

pub fn logger() -> &'static dyn Log {
   // Is it necessary to use Ordering::SeqCst in log!
    if STATE.load(Ordering::SeqCst) != INITIALIZED {
        static NOP: NopLogger = NopLogger;
        &NOP
    } else {
        unsafe { LOGGER }
    }
}

See the implementation of std::lazy::SyncOnceCell or parking_lot::Once

@relufi relufi changed the title Is it necessary to use Ordering::SeqCst in log! Is it necessary to use Ordering::SeqCst in the log! ?? Apr 29, 2022
@relufi
Copy link
Author

relufi commented Apr 29, 2022

I tried to use a more relaxed order and wrote a test
https://github.com/relufi/log/blob/use-relaxed-memory-barriers/src/lib.rs

However, when I tried to change all the memory order to Relaxed and tested on the arm platform, there was no visibility problem either.

@KodrAus
Copy link
Contributor

KodrAus commented May 4, 2022

I think the ordering there could probably be downgraded so long as we always see the store of STATE after the store of LOGGER. The orderings on initialization are also pretty conservative, but it's always hard to tell whether that's because they're necessary or because they haven't been optimized.

@relufi
Copy link
Author

relufi commented May 4, 2022

Reference std::sync::Once SeqCst is not necessary, I am concerned that using SeqCst in every log! will affect performance (log! is too common and SeqCst is too expensive on some platforms).
If you're worried about problems you can just introduce std::sync::Once or parking_lot::Once to avoid them.

@relufi
Copy link
Author

relufi commented May 4, 2022

I am using a translation tool, so please bear with me if I make any mistakes.

EFanZh pushed a commit to EFanZh/log that referenced this issue Jul 23, 2023
chore: Release

Co-authored-by: github-actions <github-actions@github.com>
@Thomasdezeeuw
Copy link
Collaborator

Since #610 we're using Acquire in logger, so closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants