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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

AngelicosPhosphoros
Copy link
Contributor

@AngelicosPhosphoros AngelicosPhosphoros commented Dec 17, 2023

I split implementations for 3 cases:

  • When std is available, use standard locking primitive.
  • When std is not available but there is atomics with pointer size, use spinlock
  • If there is no atomic with pointer size, assume that there is no threads and use Cell.

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

Also:

  • Replaced SeqCst by weaker orderings because they are enough
  • Removed alias from core to std because it is confusing

@AngelicosPhosphoros
Copy link
Contributor Author

AngelicosPhosphoros commented Dec 17, 2023

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

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

@Thomasdezeeuw
Copy link
Collaborator

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

@AngelicosPhosphoros
Copy link
Contributor Author

AngelicosPhosphoros commented Dec 17, 2023

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

@Thomasdezeeuw
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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 AngelicosPhosphoros changed the title Use more suitable atomic orderings instead of SeqCst Rewrite set_logger function to use std::sync::Once on std Dec 19, 2023
I split implementations for 3 cases:
* When std is available, use standard locking primitive.
* When std is not available but there is atomics with pointer size, use spinlock
* If there is no atomic with pointer size, assume that there is no threads and use `Cell`.

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

Also:
* Replaced `SeqCst` by weaker orderings because they are enough
* Removed alias from `core` to `std` because it is confusing
@AngelicosPhosphoros
Copy link
Contributor Author

Put it into draft until #608 is merged.

@thomcc
Copy link
Member

thomcc commented Dec 19, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

AngelicosPhosphoros commented Dec 19, 2023

@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
Copy link
Contributor Author

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

@Thomasdezeeuw
Copy link
Collaborator

@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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants