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.

Sign up for GitHub

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

Jump to bottom

Multiple improvements to RwLocks #84687

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
Contributor

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
Contributor

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
Contributor

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
Contributor

bors commented Jun 8, 2021

⌛ Testing commit ac470e9 with merge 7ec1035eca280ff57be0c76930820203a694f316...

@bors
Copy link
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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