Skip to content

Multiple improvements to RwLocks #84687

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 10, 2021
Merged

Multiple improvements to RwLocks #84687

merged 1 commit into from
Jun 10, 2021

Conversation

a1phyr
Copy link
Contributor

@a1phyr a1phyr commented Apr 29, 2021 •
edited
Loading

This PR replicates #77147, #77380 and #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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 29, 2021
@CDirkx
Copy link
Contributor

CDirkx commented Apr 29, 2021 •
edited
Loading

A comment says that StaticMutex is not re-entrant, I don't understand why and I don't know whether it applies to StaticRWLock.

StaticMutex deadlocks on re-entrancy, but also causes undefined behaviour when locked twice on unix:

rust/library/std/src/sys/unix/mutex.rs

Lines 32 to 34 in 78c9639

// A pthread mutex initialized with PTHREAD_MUTEX_INITIALIZER will have
// a type of PTHREAD_MUTEX_DEFAULT, which has undefined behavior if you
// try to re-lock it from the same thread when you already hold a lock

StaticRWLock deadlocks on re-entrancy, but doesn't cause undefined behaviour.

@bors
Copy link
Collaborator

bors commented May 7, 2021

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

@a1phyr
Copy link
Contributor Author

a1phyr commented May 15, 2021

Rebased

@bors
Copy link
Collaborator

bors commented May 20, 2021

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

@a1phyr
Multiple improvements to RwLocks
ac470e9 
- Split `sys_common::RWLock` between `StaticRWLock` and `MovableRWLock`
- Unbox `RwLock` on some platforms (Windows, Wasm and unsupported)
- Simplify `RwLock::into_inner`
@m-ou-se m-ou-se added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 1, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 6, 2021

Very nice! Thanks!

(Sorry for the late review.)

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 6, 2021

📌 Commit ac470e9 has been approved by m-ou-se

@bors 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 Jun 6, 2021
@bors
Copy link
Collaborator

bors commented Jun 8, 2021

⌛ Testing commit ac470e9 with merge 7ec1035eca280ff57be0c76930820203a694f316...

@bors
Copy link
Collaborator

bors commented Jun 8, 2021

💔 Test failed - checks-actions

@bors 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 Jun 8, 2021
@rust-log-analyzer
Copy link
Collaborator

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

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

@JohnTitor
Copy link
Member 

2021-06-08T15:52:15.0727607Z Can't find any online and idle self-hosted runner in current repository that matches the required labels: 'ubuntu-latest'
2021-06-08T15:52:15.1076278Z Can't find any online and idle self-hosted runner in current repository's organization account that matches the required labels: 'ubuntu-latest'
2021-06-08T15:52:15.2176794Z Found online and idle hosted runner in current repository's organization account that matches the required labels: 'ubuntu-latest'

@bors retry

@bors 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 Jun 9, 2021
@JohnTitor JohnTitor mentioned this pull request Jun 10, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 10, 2021
@bors
Auto merge of rust-lang#86186 - JohnTitor:rollup-upaw6wx, r=JohnTitor
c5fbcd3 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#82037 (Make symbols stripping work on MacOS X)
 - rust-lang#84687 (Multiple improvements to RwLocks)
 - rust-lang#85997 (rustdoc: Print a warning if the diff when comparing to old nightlies is empty)
 - rust-lang#86051 (Updated code examples and wording in move keyword documentation )
 - rust-lang#86111 (fix off by one in `std::iter::Iterator` documentation)
 - rust-lang#86113 (build doctests with lld if use-lld = true)
 - rust-lang#86175 (update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 578eb6d into rust-lang:master Jun 10, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 10, 2021
@mkroening mkroening mentioned this pull request Jun 18, 2021
Merged
@m-ou-se m-ou-se mentioned this pull request Feb 7, 2022
Closed
63 tasks
@a1phyr a1phyr deleted the improve_rwlock branch August 23, 2022 23:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@CDirkx CDirkx CDirkx left review comments

Assignees

@m-ou-se m-ou-se

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.
8 participants
@a1phyr @CDirkx @bors @m-ou-se @rust-log-analyzer @JohnTitor @rust-highfive @rustbot