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

Rwlock downgrade #128219

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Conversation

connortsui20
Copy link

@connortsui20 connortsui20 commented Jul 26, 2024

Tracking Issue: #128203

This PR adds a downgrade method for RwLock / RwLockWriteGuard on all currently supported platforms.

Outstanding questions:

  • Does the futex.rs change affect performance at all? It doesn't seem like it will but we can't be certain until we bench it...
  • Should the SOLID platform implementation be ported over to the queue.rs implementation to allow it to support downgrades?

@rustbot
Copy link
Collaborator

rustbot commented Jul 26, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 26, 2024
@connortsui20
Copy link
Author

@kawadakk I am very unfamiliar with the SOLID platform, do you know if the external rwlock implementation for SOLID supports downgrade?

@Amanieu
Copy link
Member

Amanieu commented Jul 27, 2024

r? @joboet

@rustbot rustbot assigned joboet and unassigned Amanieu Jul 27, 2024
@bors
Copy link
Contributor

bors commented Jul 28, 2024

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

@bors
Copy link
Contributor

bors commented Jul 29, 2024

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

@kawadakk
Copy link
Contributor

kawadakk commented Jul 29, 2024

@connortsui20 The SOLID platform doesn't support the downgrade operation for rwlocks. Migrating to the queue implementation is a possible (and likely more performant) option, though this would mean losing priority-ordered queueing.

@connortsui20
Copy link
Author

Migrating to the queue implementation is a possible (and likely more performant) option, though this would mean losing priority-ordered queueing.

This is not strictly necessary, as we could make the downgrade operation a noop where we just don't allow other readers to read. However there are some things that I think need to be discussed first (see #128203).

@rust-log-analyzer

This comment has been minimized.

@joboet
Copy link
Contributor

joboet commented Aug 16, 2024

I'm reassigning this, I shouldn't review my own code...

r? libs

@rustbot rustbot assigned Mark-Simulacrum and unassigned joboet Aug 16, 2024
@connortsui20 connortsui20 marked this pull request as ready for review August 16, 2024 20:08
@rustbot
Copy link
Collaborator

rustbot commented Aug 16, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 16, 2024
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 16, 2024
@rustbot rustbot added the has-merge-commits PR has merge commits, merge with caution. label Aug 17, 2024
@rustbot rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Aug 24, 2024
@tgross35
Copy link
Contributor

Sorry about this, I didn't get the chance to review the rest of this. I can do that when I'm back later this month, but in case somebody wants to pick it up before then.

r? @libs

@rustbot rustbot assigned Mark-Simulacrum and unassigned tgross35 Sep 10, 2024
@Mark-Simulacrum
Copy link
Member

r? tgross35

I'll set the reviewer back for now, since I don't currently have the bandwidth to dig in here.

@rustbot
Copy link
Collaborator

rustbot commented Sep 14, 2024

Could not assign reviewer from: tgross35.
User(s) tgross35 are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@connortsui20
Copy link
Author

r? @libs

@rustbot rustbot assigned Noratrieb and unassigned Mark-Simulacrum Sep 14, 2024
@connortsui20
Copy link
Author

Should I split out the queue implementation changes (of which a significant portion are just documentation changes) into a separate PR to keep this one more simple? I understand that the queue implementation is somewhat tricky so it might deserve its own PR

@Noratrieb
Copy link
Member

@m-ou-se @kprotty in case you have input on the queue implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants