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

Do not wrap platform locks in sys_common #100505

Closed
wants to merge 1 commit into from

Conversation

joboet
Copy link
Member

@joboet joboet commented Aug 13, 2022

Now that Mutex and RwLock are const, StaticMutex and StaticRwLock are no longer necessary. Removing them results in a extra few allocations on platforms which require boxed locks (currently some UNIXes and SGX), but it makes the codebase quite a lot cleaner. With the lock improvements tracked in #93740, these allocations will become unnecessary.

The rest of the lock abstractions in sys_common can be removed by modifying the lock implementations in sys so that they are const and movable on all platforms. This entails a slight bit of code duplication (both SGX and UNIX need to wrap their locks in LazyBox, for now), but reduces the usage of unsafe quite considerably.

The Condvar check is merged into the pthread_condvar implementation, making sys_common::condvar redundant. The check is otherwise only performed on SGX and Hermit, which however do not rely on the check for soundness. I therefore removed it on these platforms, as it will be made redundant by the aforementioned lock improvements anyway.

Additionally, this PR makes the condition variable on Hermit movable by lazily initializing it in place instead of lazily allocating and initializing.

@rustbot label +T-libs, +T-libs-api
r? @m-ou-se

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 13, 2022
@rustbot
Copy link
Collaborator

rustbot commented Aug 13, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2022
@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 13, 2022
@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member

m-ou-se commented Aug 14, 2022

Always good to see a PR that removes more code than it adds. ^^

It's quite a large PR though, affecting a lot of different things, so it'd be good if you could split it up a bit. Then if this accidentally causes some issues later, it'll be easier to track down.

  • It seems to me it should be easy to split off the changes that make all MovableThing = Thing on all platforms. (One PR for all platforms, or one PR each; whatever you prefer.) Then we can merge that first, making the remaining change less complicated.

  • The removal of imp::requires_synchronized_create() is mostly unrelated to this change, and should be possible to do separately in an independent PR.

  • The changes to stdout() don't only remove the Pin, but also move the lazy initialization of the buffer from the stdout() call to the first write() call. Is there any benefit in doing it that way? If so, it'd be good to put that in a separate PR.

Comment on lines -59 to +60
target_os = "espidf"
target_os = "espidf",
target_os = "horizon",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change? This seems unrelated to this PR.

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2022
@joboet
Copy link
Member Author

joboet commented Aug 15, 2022

I got a bit too carried away with the changes... 😉

This PR is now split in #100576, #100579, #100581 and #100583, the fifth part will be removing the sys_common wrappers, but I'll wait with that until the others are merged.

@joboet joboet closed this Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants