rust-lang / log

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

Rewrite `set_logger` function to use `std::sync::Once` on std #605

Closed AngelicosPhosphoros closed 6 months ago

AngelicosPhosphoros commented 7 months ago

I split implementations for 3 cases:

I think, this separation makes easier to understand what is happening in each case.

Also:

AngelicosPhosphoros commented 7 months ago

I think, CI failed because of errors in rustc. Newer rustc doesn't complain about ordering.

Edit: found a reason - https://github.com/rust-lang/rust/pull/98383/ Edit2: On the other hand, we don't ever need Acquire ordering during initialization of logger at all.

Thomasdezeeuw commented 7 months ago

I think we should replace the atomic with OnceLock, or our own implementation of it (since it's rustc v1.70+).

AngelicosPhosphoros commented 7 months ago

What to do about targets without atomic sized pointers? I think once_cell doesn't support that.

Thomasdezeeuw commented 7 months ago

What to do about targets without atomic sized pointers? I think once_cell doesn't support that.

I'm not suggesting once_cell. I'm suggesting std::sync::OnceCell and std::sync::Once.

AngelicosPhosphoros commented 7 months ago

So what should I do? I suppose I should not up the minimal rustc?

I think, you should merge this PR and switch to OnceCell later when you move to rustc 1.70.

AngelicosPhosphoros commented 7 months ago

I can also try to detect rustc during using build script and cfg in OnceCell but I don't think that it is wise.

Thomasdezeeuw commented 7 months ago

So what should I do? I suppose I should not up the minimal rustc?

I think, you should merge this PR and switch to OnceCell later when you move to rustc 1.70.

If you want, you can try to replace STATE with std::sync::Once.

Then once we update our MSRV to 1.70+ we can use std::sync::OnceLock.

However, I don't know the reason why the current approach was taken over using Once. It could be that the lack of support for targets without an OS was a deal breaker, I'm not sure.

I can also try to detect rustc during using build script and cfg in OnceCell but I don't think that it is wise.

We used to have that, let's not go back to that.

AngelicosPhosphoros commented 7 months ago

I think, the main problem with std::sync::Once is that it is supported only in std. I can rewrite code to use std in std configuration and atomics otherwise.

AngelicosPhosphoros commented 7 months ago

Put it into draft until https://github.com/rust-lang/log/pull/608 is merged.

thomcc commented 7 months ago

If there is no atomic with pointer size, assume that there is no threads and use Cell.

I don't have a horse in this race, but FWIW there are a number of targets where this is not true.

Thomasdezeeuw commented 7 months ago

I'm not sure about these changes. As @thomcc mentions they could be incorrect for a number of targets we support. Furthermore the code doesn't get any simpler from it.

AngelicosPhosphoros commented 7 months ago

@thomcc comment is about your current code, btw, because it is written cleverly to hide the fact it uses Cell by renaming Cell to AtomicUsize on targets without atomic pointers.

The fact, that he noticed that now, proves that I made code simpler to read.

If you're not going to accept this version, can we return PR to using just correct Orderings at least?

AngelicosPhosphoros commented 7 months ago

Btw, @thomcc can you tell us which platforms you mean? And what people use in them instead of atomics?

Thomasdezeeuw commented 7 months ago

@AngelicosPhosphoros the current code is carefully written to work on as many platforms as possible. We can't easily change it without possibly breaking support for some platforms. The work around in place are for platforms that don't support atomic operations or threads (think micro controllers). Rewriting the core of this library is not done on a whim, we need to be consider why the code was written as-is.