Unbox mutexes and condvars on some platforms #77380

m-ou-se · 2020-09-30T23:44:48Z

Both mutexes and condition variables contained a Box containing the actual os-specific object. This was done because moving these objects may cause undefined behaviour on some platforms.

However, this is not needed on Windows[1], Wasm[2], cloudabi[2], and 'unsupported'[3], were the box was only needlessly making them less efficient.

This change gets rid of the box on those platforms.

On those platforms, Condvar can no longer verify it is only used with one Mutex, as mutexes no longer have a stable address. This was addressed and considered acceptable in #76932.

[1]: https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-initializesrwlock
[2]: These are just a single atomic integer together with futex wait/wake calls/instructions.
[3]: The unsupported platform doesn't support multiple threads at all.

rust-highfive · 2020-09-30T23:44:52Z

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

…ex, r=dtolnay Split sys_common::Mutex in StaticMutex and MovableMutex. The (unsafe) `Mutex` from `sys_common` had a rather complicated interface. You were supposed to call `init()` manually, unless you could guarantee it was neither moved nor used reentrantly. Calling `destroy()` was also optional, although it was unclear if 1) resources might be leaked or not, and 2) if `destroy()` should only be called when `init()` was called. This allowed for a number of interesting (confusing?) different ways to use this `Mutex`, all captured in a single type. In practice, this type was only ever used in two ways: 1. As a static variable. In this case, neither `init()` nor `destroy()` are called. The variable is never moved, and it is never used reentrantly. It is only ever locked using the `LockGuard`, never with `raw_lock`. 2. As a `Box`ed variable. In this case, both `init()` and `destroy()` are called, it will be moved and possibly used reentrantly. No other combinations are used anywhere in `std`. This change simplifies things by splitting this `Mutex` type into two types matching the two use cases: `StaticMutex` and `MovableMutex`. The interface of both new types is now both safer and simpler. The first one does not call nor expose `init`/`destroy`, and the second one calls those automatically in its `new()` and `Drop` functions. Also, the locking functions of `MovableMutex` are no longer unsafe. --- This will also make it easier to conditionally box mutexes later, by moving that decision into sys/sys_common. Some of the mutex implementations (at least those of Wasm and 'sys/unsupported') are safe to move, so wouldn't need a box. ~~(But that's blocked on rust-lang#76932 for now.)~~ (See rust-lang#77380.)

bors · 2020-10-02T03:38:00Z

☔ The latest upstream changes (presumably #77436) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

dtolnay

r=me on the diff from #77147 to this (74237e9...dc67bde).

This commit keeps all mutexes boxed on all platforms, but makes it trivial to remove the box on some platforms later.

Cloudabi mutexes may be moved safely.

These mutexes are just a bool (in a cell), so can be moved without problems.

These mutexes are just an AtomicUsize, so can be moved without problems.

Windows SRW locks are movable (while not borrowed) according to their documentation.

This commit keeps all condvars boxed on all platforms, but makes it trivial to remove the box on some platforms later.

Cloudabi condvars may be moved safely.

These condvars are unsupported and implemented as a ZST, so can be moved without problems.

These condvars are just an AtomicUsize, so can be moved without problems.

Windows condition variables are movable (while not borrowed) according to their documentation.

Condvars are no longer guaranteed to panic in this case on all platforms. At least the unix implementation still does.

m-ou-se · 2020-10-02T08:24:30Z

Thanks!

Rebased to include #77147.

dtolnay · 2020-10-02T12:41:33Z

@bors r+

bors · 2020-10-02T12:41:34Z

📌 Commit b1ce7a3 has been approved by dtolnay

m-ou-se · 2020-10-02T13:19:23Z

Thanks!

Might be good to mark this as rollup=never, considering this changes platform-specific things. ^^

dtolnay · 2020-10-02T16:00:25Z

@bors rollup=iffy

bors · 2020-10-03T09:34:21Z

⌛ Testing commit b1ce7a3 with merge 84e18dd511d49cc9dcb9a3e2b6fefb7eed27be77...

bors · 2020-10-03T13:49:21Z

💥 Test timed out

m-ou-se · 2020-10-03T13:57:26Z

💥 Test timed out

This seems unrelated. The tests passed on all platforms except the dist-x86_64-apple-alt test, which timed out. This PR shouldn't affect anything on mac. That test was still running (it didn't deadlock), but it was running at 20%-50% of the usual speed: it was outputting [RUSTC-TIMING] messages with values 2 to 4 times higher than the last few runs.

dtolnay · 2020-10-03T15:25:33Z

@bors retry

bors · 2020-10-04T06:48:22Z

⌛ Testing commit b1ce7a3 with merge 32cbc65...

bors · 2020-10-04T08:55:59Z

☀️ Test successful - checks-actions, checks-azure
Approved by: dtolnay
Pushing 32cbc65 to master...

jethrogb · 2021-04-22T12:56:49Z

@m-ou-se do you have any guidance for target maintainers that may want to get rid of the box?

m-ou-se · 2021-04-22T13:01:04Z

@jethrogb The reason for the Box is that many mutex and condvar implementations are not trivially movable. If you can just 'memcpy' a mutex and forget about the old one, only then it can be used without a Box in Rust. This is often not possible because of things like internal pointers. Feel free to ping me in the #t-libs channel on Zulip if I can help with anything.

Multiple improvements to RwLocks This PR replicates rust-lang#77147, rust-lang#77380 and rust-lang#84650 on RWLocks : - Split `sys_common::RWLock` in `StaticRWLock` and `MovableRWLock` - Unbox rwlocks on some platforms (Windows, Wasm and unsupported) - Simplify `RwLock::into_inner` Notes to reviewers : - For each target, I copied `MovableMutex` to guess if `MovableRWLock` should be boxed. - ~A comment says that `StaticMutex` is not re-entrant, I don't understand why and I don't know whether it applies to `StaticRWLock`.~ r? `@m-ou-se`

Make {Mutex, Condvar, RwLock}::new() const. This makes it possible to have `static M: Mutex<_> = Mutex::new(..);` 🎉 Our implementations [on Linux](rust-lang#95035), [on Windows](rust-lang#77380), and various BSDs and some tier 3 platforms have already been using a non-allocating const-constructible implementation. As of rust-lang#97647, the remaining platforms (most notably macOS) now have a const-constructible implementation as well. This means we can finally make these functions publicly const. Tracking issue: rust-lang#93740

Make {Mutex, Condvar, RwLock}::new() const. This makes it possible to have `static M: Mutex<_> = Mutex::new(..);` 🎉 Our implementations [on Linux](rust-lang/rust#95035), [on Windows](rust-lang/rust#77380), and various BSDs and some tier 3 platforms have already been using a non-allocating const-constructible implementation. As of rust-lang/rust#97647, the remaining platforms (most notably macOS) now have a const-constructible implementation as well. This means we can finally make these functions publicly const. Tracking issue: rust-lang/rust#93740

rust-highfive assigned joshtriplett Sep 30, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2020

m-ou-se mentioned this pull request Sep 30, 2020

Split sys_common::Mutex in StaticMutex and MovableMutex. #77147

Merged

dtolnay approved these changes Oct 2, 2020

View reviewed changes

m-ou-se added 12 commits October 2, 2020 09:47

Move boxing and mutex checking logic of condvar into sys_common.

a8c2d4f

Make it possible to have unboxed mutexes on specific platforms.

58deb70

This commit keeps all mutexes boxed on all platforms, but makes it trivial to remove the box on some platforms later.

No longer put cloudabi mutexes in a box.

def5188

Cloudabi mutexes may be moved safely.

No longer put mutexes on the 'unsupported' platform in a box.

2f03867

These mutexes are just a bool (in a cell), so can be moved without problems.

No longer put wasm mutexes in a box.

4f1353e

These mutexes are just an AtomicUsize, so can be moved without problems.

No longer put windows mutexes in a box.

dc81cbd

Windows SRW locks are movable (while not borrowed) according to their documentation.

Make it possible to have unboxed condvars on specific platforms.

b181f5a

This commit keeps all condvars boxed on all platforms, but makes it trivial to remove the box on some platforms later.

No longer put cloudabi condvars in a box.

5769a46

Cloudabi condvars may be moved safely.

No longer put condvars on the 'unsupported' platform in a box.

7f56a35

These condvars are unsupported and implemented as a ZST, so can be moved without problems.

No longer put wasm condvars in a box.

ec69a85

These condvars are just an AtomicUsize, so can be moved without problems.

No longer put windows condvars in a box.

f3837e7

Windows condition variables are movable (while not borrowed) according to their documentation.

Disable condvar::two_mutexes test on non-unix platforms.

b1ce7a3

Condvars are no longer guaranteed to panic in this case on all platforms. At least the unix implementation still does.

m-ou-se force-pushed the unbox-the-mutex branch from dc67bde to b1ce7a3 Compare October 2, 2020 08:03

m-ou-se marked this pull request as ready for review October 2, 2020 08:24

dtolnay approved these changes Oct 2, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2020

dtolnay assigned dtolnay and unassigned joshtriplett Oct 2, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 3, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 4, 2020

bors merged commit 32cbc65 into rust-lang:master Oct 4, 2020

rustbot added this to the 1.49.0 milestone Oct 4, 2020

m-ou-se deleted the unbox-the-mutex branch November 19, 2020 15:47

a1phyr mentioned this pull request Apr 29, 2021

Multiple improvements to RwLocks #84687

Merged

mkroening mentioned this pull request Jun 13, 2021

Unbox mutexes, condvars and rwlocks on hermit hermit-os/rust#3

Merged

m-ou-se mentioned this pull request Feb 7, 2022

Tracking issue for improving std::sync::{Mutex, RwLock, Condvar} #93740

Closed

63 tasks

m-ou-se mentioned this pull request Jun 6, 2022

Make {Mutex, Condvar, RwLock}::new() const. #97791

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unbox mutexes and condvars on some platforms #77380

Unbox mutexes and condvars on some platforms #77380

m-ou-se commented Sep 30, 2020 •

edited

Loading

rust-highfive commented Sep 30, 2020

bors commented Oct 2, 2020

dtolnay left a comment

m-ou-se commented Oct 2, 2020

dtolnay commented Oct 2, 2020

bors commented Oct 2, 2020

m-ou-se commented Oct 2, 2020

dtolnay commented Oct 2, 2020

bors commented Oct 3, 2020

bors commented Oct 3, 2020

m-ou-se commented Oct 3, 2020 •

edited

Loading

dtolnay commented Oct 3, 2020

bors commented Oct 4, 2020

bors commented Oct 4, 2020

jethrogb commented Apr 22, 2021

m-ou-se commented Apr 22, 2021

Unbox mutexes and condvars on some platforms #77380

Unbox mutexes and condvars on some platforms #77380

Conversation

m-ou-se commented Sep 30, 2020 • edited Loading

rust-highfive commented Sep 30, 2020

bors commented Oct 2, 2020

dtolnay left a comment

Choose a reason for hiding this comment

m-ou-se commented Oct 2, 2020

dtolnay commented Oct 2, 2020

bors commented Oct 2, 2020

m-ou-se commented Oct 2, 2020

dtolnay commented Oct 2, 2020

bors commented Oct 3, 2020

bors commented Oct 3, 2020

m-ou-se commented Oct 3, 2020 • edited Loading

dtolnay commented Oct 3, 2020

bors commented Oct 4, 2020

bors commented Oct 4, 2020

jethrogb commented Apr 22, 2021

m-ou-se commented Apr 22, 2021

m-ou-se commented Sep 30, 2020 •

edited

Loading

m-ou-se commented Oct 3, 2020 •

edited

Loading