Conversation
|
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 (
|
rustbot
added
S-waiting-on-review
|
@kawadakk I am very unfamiliar with the SOLID platform, do you know if the external rwlock implementation for SOLID supports downgrade? |
|
r? @joboet |
|
☔ The latest upstream changes (presumably #128313) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@connortsui20 The SOLID platform doesn't support the downgrade operation for rwlocks. Migrating to the |
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). |
This comment has been minimized.
This comment has been minimized.
|
I'm reassigning this, I shouldn't review my own code... r? libs |
|
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: The following commits are merge commits: |
rustbot
added
has-merge-commits
connortsui20
force-pushed
the
rwlock-downgrade
branch
from
5b1daa6 to
e7d0803
Compare
rustbot
removed
has-merge-commits
This commit only has documentation changes and a few things moved around the file. The very few code changes are cosmetic: changes like turning a `match` statement into an `if let` statement or reducing indentation for long if statements. This commit also adds several safety comments on top of `unsafe` blocks that might not be immediately obvious to a first-time reader. Code "changes" are in: - `add_backlinks_and_find_tail` - `lock_contended` A majority of the changes are just expanding the comments from 80 columns to 100 columns.
This commit adds the `downgrade` method onto the inner `RwLock` queue implementation. There are also a few other style patches included in this commit. Co-authored-by: Jonas Böttiger <jonasboettiger@icloud.com>
This commit fixes a memory ordering bug in the futex implementation (`Relaxed` -> `Release` on `downgrade`). This commit also removes a badly written test that deadlocked and replaces it with a more reasonable test based on an already-tested `downgrade` test from the parking-lot crate.
Co-authored-by: Jonas Böttiger <jonasboettiger@icloud.com>
This comment was marked as outdated.
This comment was marked as outdated.
|
Hi bors @bors r+ |
|
@bors r+ |
|
@bors ping |
|
@bors r+ |
|
💔 Test failed - checks-actions |
|
@bors retry |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
|
That doesn't look like a failure from this change @bors retry |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (e83c45a): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (secondary 1.1%)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.
CyclesResults (secondary -1.5%)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.
Binary sizeResults (secondary 0.0%)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.
Bootstrap: 791.022s -> 790.339s (-0.09%) |
15 participants
Tracking Issue: #128203
This PR adds a
downgrademethod forRwLock/RwLockWriteGuardon all currently supported platforms.Outstanding questions:
Does thefutex.rschange 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 thequeue.rsimplementation to allow it to support downgrades?