std: use `sync::Mutex` for internal statics #100579

joboet · 2022-08-15T12:11:01Z

Since sync::Mutex is now const-constructible, it can be used for internal statics, removing the need for sys_common::StaticMutex. This adds some extra allocations on platforms which need to box their mutexes (currently SGX and some UNIX), but these will become unnecessary with the lock improvements tracked in #93740.

I changed the program argument implementation on Hermit, it does not need Mutex but can use atomics like some UNIX systems (ping @mkroening @stlankes).

rustbot · 2022-08-15T12:11:04Z

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 · 2022-08-15T12:11:05Z

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

library/std/src/backtrace.rs

joshtriplett · 2022-08-16T17:21:59Z

LGTM

mkroening · 2022-08-23T09:37:54Z

The hermit changes look good to me. 👍

stlankes · 2022-08-23T12:06:04Z

Looks also good for me!

library/std/src/sys_common/thread_local_key.rs

thomcc · 2022-09-18T15:53:01Z

As mentioned in #100581 I'm not a fan of this. I care more about this on Darwin than on other platforms, and a bit more about this for StaticMutex than StaticRwlock.

I also would eventually like to move StaticMutex on Darwin to os_unfair_lock which is much more efficient (this is blocked by rust-lang/compiler-team#556). This change would make that harder.

m-ou-se · 2022-09-18T16:04:48Z

I also would eventually like to move StaticMutex on Darwin to os_unfair_lock which is much more efficient (this is blocked by rust-lang/compiler-team#556).

I agree that that'd be good.

If we decouple the mutex and condition variable on macOS, we could possibly also make the standard Mutex a os_unfair_lock, which would be compatible with this PR. (Not sure yet if that's a good idea though, but it might be.)

thomcc · 2022-09-18T16:08:21Z

I guess it wouldn't be the end of the world. It looks like on Darwin the only time we actually would be taking a StaticMutex is when producing a backtrace and spawning processes (it has 64 bit atomics, and doesnt require synchronized create for TLS). These aren't so performance sensitive that an additional allocation will matter.

thomcc · 2022-09-18T16:09:03Z

r? @thomcc

thomcc · 2022-09-18T16:30:02Z

This looks fine. This takes locks in the same cases as before (well, strictly fewer, actually -- hermit will use atomics with this PR), so there should be no concerns about additional deadlocks here (mentioned by @m-ou-se on Zulip). No rollup for similar reasons to the rwlock changes.

@bors r+ rollup=never

bors · 2022-09-18T16:30:04Z

📌 Commit dce2d6a65d5bf154003eb95f1cc152b46097921c has been approved by thomcc

It is now in the queue for this repository.

joboet · 2022-09-18T16:32:17Z

I also would eventually like to move StaticMutex on Darwin to os_unfair_lock which is much more efficient (this is blocked by rust-lang/compiler-team#556).

I agree that that'd be good.

If we decouple the mutex and condition variable on macOS, we could possibly also make the standard Mutex a os_unfair_lock, which would be compatible with this PR. (Not sure yet if that's a good idea though, but it might be.)

That would be great, but it probably requires Apple to promise that the os_unfair_lock is movable when unlocked.

m-ou-se · 2022-09-18T16:51:18Z

That would be great, but it probably requires Apple to promise that the os_unfair_lock is movable when unlocked.

os_unfair_lock doesn't have constructor nor a destructor function, which is nearly enough to guarantee it's movable: moving a freshly initialized os_unfair_lock is equivalent to just destructing it in place and creating a new one in a new location. I guess the only thing that's technically missing is the guarantee that this still works after locking and unlocking. But without a destructor, it's safe to assume there's no external pointers pointing into it. And with its tiny size (1 pointer), internal pointers or alignment-dependent operation also seem very unlikely. (But yes, official confirmation from Apple would be nice. ^^)

bors · 2022-09-18T16:52:24Z

⌛ Testing commit dce2d6a65d5bf154003eb95f1cc152b46097921c with merge fc4d094db22a53d020f3486c334dd9bee77b503a...

thomcc · 2022-09-18T16:59:49Z

It's hard to get Apple to update their documentation, so I wouldn't hold my breath there honestly. If we want to block using it on that, I could file an issue with them to ask though (I have a dev account and don't mind). That said, I agree it's unclear on whether or not it's worth using for Mutex, given the trickiness it adds to condvars (it's hard for me to imagine we'd have an impl that doesn't penalize condvar-heavy code, for example).

I agree that it's hard to imagine this being immovable in any way, especially given that it's intended as a replacement for OSSpinLock.

joboet · 2022-09-18T17:16:34Z

I agree it's unclear on whether or not it's worth using for Mutex, given the trickiness it adds to condvars (it's hard for me to imagine we'd have an impl that doesn't penalize condvar-heavy code, for example).

If the condvar implementation is changed to a parking-lot or atomic queue (see usync), it does not matter what the underlying mutex is, so this is definitely worth doing once condvars have been optimized.

bors · 2022-10-13T18:08:15Z

⌛ Testing commit 2d2c9e4 with merge 3993d24c2658c1b2e4a9e0600bf6cbec13a246f1...

bors · 2022-10-13T22:23:15Z

💥 Test timed out

rust-log-analyzer · 2022-10-13T22:46:38Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

…omcc std: use `sync::Mutex` for internal statics Since `sync::Mutex` is now `const`-constructible, it can be used for internal statics, removing the need for `sys_common::StaticMutex`. This adds some extra allocations on platforms which need to box their mutexes (currently SGX and some UNIX), but these will become unnecessary with the lock improvements tracked in rust-lang#93740. I changed the program argument implementation on Hermit, it does not need `Mutex` but can use atomics like some UNIX systems (ping `@mkroening` `@stlankes).`

joboet · 2022-10-15T12:03:20Z

This seems like a spurious failure, as the tests passed before and nothing was changed for macOS since.

thomcc · 2022-10-15T17:03:37Z

@bors delegate+

bors · 2022-10-15T17:03:39Z

✌️ @joboet can now approve this pull request

joboet · 2022-10-15T21:27:39Z

@bors r+ rollup=iffy

bors · 2022-10-15T21:27:41Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

bors · 2022-10-15T21:27:42Z

📌 Commit 2d2c9e4 has been approved by joboet

It is now in the queue for this repository.

joboet · 2022-10-15T21:29:13Z

@bors r- r=thomcc

bors · 2022-10-15T21:29:15Z

📌 Commit 2d2c9e4 has been approved by thomcc

It is now in the queue for this repository.

bors · 2022-10-15T22:49:33Z

⌛ Testing commit 2d2c9e4 with merge ddc7fd9...

bors · 2022-10-16T01:42:37Z

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing ddc7fd9 to master...

rust-timer · 2022-10-16T03:02:54Z

Finished benchmarking commit (ddc7fd9): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.6%	[-5.6%, -5.6%]	1
Improvements ✅ (secondary)	-2.3%	[-2.8%, -1.7%]	4
All ❌✅ (primary)	-5.6%	[-5.6%, -5.6%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	2.9%	[2.4%, 3.5%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	[-1.3%, -1.3%]	1
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	1.5%	[-1.3%, 3.5%]	3

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

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

rust-highfive assigned m-ou-se Aug 15, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2022

joboet mentioned this pull request Aug 15, 2022

Do not wrap platform locks in sys_common #100505

Closed

deltragon reviewed Aug 15, 2022

View reviewed changes

library/std/src/backtrace.rs Outdated Show resolved Hide resolved

library/std/src/backtrace.rs Outdated Show resolved Hide resolved

joboet force-pushed the sync_mutex_everywhere branch from c624583 to 6ffd8e4 Compare August 15, 2022 15:18

a1phyr reviewed Aug 24, 2022

View reviewed changes

library/std/src/sys_common/thread_local_key.rs Outdated Show resolved Hide resolved

joboet force-pushed the sync_mutex_everywhere branch from 6ffd8e4 to dce2d6a Compare September 3, 2022 12:22

thomcc mentioned this pull request Sep 18, 2022

std: use sync::RwLock for internal statics #100581

Merged

rust-highfive assigned thomcc and unassigned m-ou-se Sep 18, 2022

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 Sep 18, 2022

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 13, 2022

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 13, 2022

albertlarsan68 mentioned this pull request Oct 14, 2022

Rollup of 13 pull requests #103049

Closed

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 15, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 16, 2022

bors merged commit ddc7fd9 into rust-lang:master Oct 16, 2022

rustbot added this to the 1.66.0 milestone Oct 16, 2022

joboet deleted the sync_mutex_everywhere branch October 16, 2022 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std: use `sync::Mutex` for internal statics #100579

std: use `sync::Mutex` for internal statics #100579

joboet commented Aug 15, 2022 •

edited

Loading

rustbot commented Aug 15, 2022

rust-highfive commented Aug 15, 2022

joshtriplett commented Aug 16, 2022

mkroening commented Aug 23, 2022

stlankes commented Aug 23, 2022

thomcc commented Sep 18, 2022 •

edited

Loading

m-ou-se commented Sep 18, 2022

thomcc commented Sep 18, 2022 •

edited

Loading

thomcc commented Sep 18, 2022

thomcc commented Sep 18, 2022

bors commented Sep 18, 2022

joboet commented Sep 18, 2022

m-ou-se commented Sep 18, 2022

bors commented Sep 18, 2022

thomcc commented Sep 18, 2022 •

edited

Loading

joboet commented Sep 18, 2022 •

edited

Loading

bors commented Oct 13, 2022

bors commented Oct 13, 2022

rust-log-analyzer commented Oct 13, 2022

joboet commented Oct 15, 2022

thomcc commented Oct 15, 2022

bors commented Oct 15, 2022

joboet commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 15, 2022

joboet commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 16, 2022

rust-timer commented Oct 16, 2022

std: use sync::Mutex for internal statics #100579

std: use sync::Mutex for internal statics #100579

Conversation

joboet commented Aug 15, 2022 • edited Loading

rustbot commented Aug 15, 2022

rust-highfive commented Aug 15, 2022

joshtriplett commented Aug 16, 2022

mkroening commented Aug 23, 2022

stlankes commented Aug 23, 2022

thomcc commented Sep 18, 2022 • edited Loading

m-ou-se commented Sep 18, 2022

thomcc commented Sep 18, 2022 • edited Loading

thomcc commented Sep 18, 2022

thomcc commented Sep 18, 2022

bors commented Sep 18, 2022

joboet commented Sep 18, 2022

m-ou-se commented Sep 18, 2022

bors commented Sep 18, 2022

thomcc commented Sep 18, 2022 • edited Loading

joboet commented Sep 18, 2022 • edited Loading

bors commented Oct 13, 2022

bors commented Oct 13, 2022

rust-log-analyzer commented Oct 13, 2022

joboet commented Oct 15, 2022

thomcc commented Oct 15, 2022

bors commented Oct 15, 2022

joboet commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 15, 2022

joboet commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 16, 2022

rust-timer commented Oct 16, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

std: use `sync::Mutex` for internal statics #100579

std: use `sync::Mutex` for internal statics #100579

joboet commented Aug 15, 2022 •

edited

Loading

thomcc commented Sep 18, 2022 •

edited

Loading

thomcc commented Sep 18, 2022 •

edited

Loading

thomcc commented Sep 18, 2022 •

edited

Loading

joboet commented Sep 18, 2022 •

edited

Loading