Rwlock downgrade #128219

connortsui20 · 2024-07-26T03:49:10Z

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 · 2024-07-26T03:49:17Z

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

connortsui20 · 2024-07-26T03:50:52Z

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

library/std/src/sys/sync/rwlock/solid.rs

library/std/src/sync/rwlock.rs

Amanieu · 2024-07-27T00:51:13Z

r? @joboet

bors · 2024-07-28T22:05:46Z

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

bors · 2024-07-29T06:05:02Z

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

kawadakk · 2024-07-29T09:01:08Z

@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 · 2024-07-29T13:06:00Z

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).

library/std/src/sys/sync/rwlock/queue.rs

joboet · 2024-08-16T18:59:20Z

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

r? libs

rustbot · 2024-08-16T20:08:45Z

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:

library/std/src/sys/sync/rwlock/futex.rs

bors · 2024-11-17T19:28:37Z

⌛ Testing commit fc52cdd with merge b77b168...

…oss35 Rwlock downgrade Tracking Issue: rust-lang#128203 This PR adds a `downgrade` method for `RwLock` / `RwLockWriteGuard` on all currently supported platforms. Outstanding questions: - [x] ~~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...~~ - [x] ~~Should the SOLID platform implementation [be ported over](rust-lang#128219 (comment)) to the `queue.rs` implementation to allow it to support downgrades?~~

rust-log-analyzer · 2024-11-17T20:50:03Z

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

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

bors · 2024-11-17T20:50:43Z

💔 Test failed - checks-actions

Kobzol · 2024-11-17T21:19:06Z

@bors retry

bors · 2024-11-18T03:09:20Z

⌛ Testing commit fc52cdd with merge 04fda4e...

…oss35 Rwlock downgrade Tracking Issue: rust-lang#128203 This PR adds a `downgrade` method for `RwLock` / `RwLockWriteGuard` on all currently supported platforms. Outstanding questions: - [x] ~~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...~~ - [x] ~~Should the SOLID platform implementation [be ported over](rust-lang#128219 (comment)) to the `queue.rs` implementation to allow it to support downgrades?~~

rust-log-analyzer · 2024-11-18T04:15:22Z

The job i686-msvc failed! Check out the build log: (web) (plain)

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

---- [ui] tests\ui\stack-protector\warn-stack-protector-unsupported.rs#all stdout ----

error in revision `all`: test compilation failed although it shouldn't!
status: exit code: 1
command: PATH="C:\a\rust\rust\build\i686-pc-windows-msvc\stage2\bin;C:\Program Files (x86)\Windows Kits\10\bin\x64;C:\Program Files (x86)\Windows Kits\10\bin\10.0.26100.0\x64;C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.42.34433\bin\HostX64\x64;C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.42.34433\bin\HostX64\x86;C:\a\rust\rust\build\i686-pc-windows-msvc\stage0-bootstrap-tools\i686-pc-windows-msvc\release\deps;C:\a\rust\rust\build\i686-pc-windows-msvc\stage0\bin;C:\Program Files\PowerShell\7;C:\a\_temp\msys64\mingw32\bin;C:\a\_temp\msys64\usr\local\bin;C:\a\_temp\msys64\usr\bin;C:\a\_temp\msys64\usr\bin;C:\a\rust\rust\ninja;C:\a\rust\rust\sccache;C:\a\_temp\setup-msys2;C:\Program Files\MongoDB\Server\5.0\bin;C:\aliyun-cli;C:\vcpkg;C:\Program Files (x86)\NSIS;C:\tools\zstd;C:\Program Files\Mercurial;C:\hostedtoolcache\windows\stack\3.1.1\x64;C:\cabal\bin;C:\ghcup\bin;C:\mingw64\bin;C:\Program Files\dotnet;C:\Program Files\MySQL\MySQL Server 8.0\bin;C:\Program Files\R\R-4.4.2\bin\x64;C:\SeleniumWebDrivers\GeckoDriver;C:\SeleniumWebDrivers\EdgeDriver;C:\SeleniumWebDrivers\ChromeDriver;C:\Program Files (x86)\sbt\bin;C:\Program Files (x86)\GitHub CLI;C:\Program Files\Git\bin;C:\Program Files (x86)\pipx_bin;C:\npm\prefix;C:\hostedtoolcache\windows\go\1.21.13\x64\bin;C:\hostedtoolcache\windows\Python\3.9.13\x64\Scripts;C:\hostedtoolcache\windows\Python\3.9.13\x64;C:\hostedtoolcache\windows\Ruby\3.0.7\x64\bin;C:\Program Files\OpenSSL\bin;C:\tools\kotlinc\bin;C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\8.0.432-6\x64\bin;C:\Program Files\ImageMagick-7.1.1-Q16-HDRI;C:\Program Files\Microsoft SDKs\Azure\CLI2\wbin;C:\ProgramData\kind;C:\ProgramData\Chocolatey\bin;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Windows\System32\OpenSSH;C:\Program Files\dotnet;C:\Program Files\PowerShell\7;C:\Program Files\Microsoft\Web Platform Installer;C:\Program Files\TortoiseSVN\bin;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\170\Tools\Binn;C:\Program Files\Microsoft SQL Server\150\Tools\Binn;C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit;C:\Program Files (x86)\WiX Toolset v3.14\bin;C:\Program Files\Microsoft SQL Server\130\DTS\Binn;C:\Program Files\Microsoft SQL Server\140\DTS\Binn;C:\Program Files\Microsoft SQL Server\150\DTS\Binn;C:\Program Files\Microsoft SQL Server\160\DTS\Binn;C:\Strawberry\c\bin;C:\Strawberry\perl\site\bin;C:\Strawberry\perl\bin;C:\ProgramData\chocolatey\lib\pulumi\tools\Pulumi\bin;C:\Program Files\CMake\bin;C:\ProgramData\chocolatey\lib\maven\apache-maven-3.8.7\bin;C:\Program Files\Microsoft Service Fabric\bin\Fabric\Fabric.Code;C:\Program Files\Microsoft SDKs\Service Fabric\Tools\ServiceFabricLocalClusterManager;C:\Program Files\nodejs;C:\Program Files\Git\cmd;C:\Program Files\Git\mingw64\bin;C:\Program Files\Git\usr\bin;C:\Program Files\GitHub CLI;C:\tools\php;C:\Program Files (x86)\sbt\bin;C:\Program Files\Amazon\AWSCLIV2;C:\Program Files\Amazon\SessionManagerPlugin\bin;C:\Program Files\Amazon\AWSSAMCLI\bin;C:\Program Files\Microsoft SQL Server\130\Tools\Binn;C:\Program Files\LLVM\bin;C:\Users\runneradmin\.dotnet\tools;C:\Users\runneradmin\.cargo\bin;C:\Users\runneradmin\AppData\Local\Microsoft\WindowsApps;C:\a\_temp\msys64\usr\bin\site_perl;C:\a\_temp\msys64\usr\bin\vendor_perl;C:\a\_temp\msys64\usr\bin\core_perl" "C:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin\\rustc.exe" "C:\\a\\rust\\rust\\tests\\ui\\stack-protector\\warn-stack-protector-unsupported.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=C:\\Users\\runneradmin\\.cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=C:\\a\\rust\\rust\\vendor" "--sysroot" "C:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2" "--cfg" "all" "--check-cfg" "cfg(FALSE,all,strong,basic)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "-C" "prefer-dynamic" "--out-dir" "C:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\ui\\stack-protector\\warn-stack-protector-unsupported.all" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=C:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\native\\rust-test-helpers" "--target" "nvptx64-nvidia-cuda" "-Z" "stack-protector=all"
--- stderr -------------------------------
--- stderr -------------------------------
warning: `-Z stack-protector=all` is not supported for target nvptx64-nvidia-cuda and will be ignored

error: couldn't create a temp dir: Access is denied. (os error 5) at path "C:\\a\\_temp\\msys64\\tmp\\rustcDLd8oo"
error: aborting due to 1 previous error; 1 warning emitted
------------------------------------------


---
test result: FAILED. 17602 passed; 1 failed; 358 ignored; 0 measured; 2 filtered out; finished in 536.72s

Some tests failed in compiletest suite=ui mode=ui host=i686-pc-windows-msvc target=i686-pc-windows-msvc
Build completed unsuccessfully in 0:50:33
make: *** [Makefile:106: ci-msvc-ps1] Error 1
  network time: Mon, 18 Nov 2024 04:15:07 GMT
##[error]Process completed with exit code 2.
Post job cleanup.
[command]"C:\Program Files\Git\bin\git.exe" version

bors · 2024-11-18T04:16:04Z

💔 Test failed - checks-actions

tgross35 · 2024-11-18T04:26:46Z

That doesn't look like a failure from this change

@bors retry

bors · 2024-11-18T07:24:16Z

⌛ Testing commit fc52cdd with merge e83c45a...

bors · 2024-11-18T10:27:05Z

☀️ Test successful - checks-actions
Approved by: tgross35
Pushing e83c45a to master...

rust-timer · 2024-11-18T11:45:37Z

Finished benchmarking commit (e83c45a): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This 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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-1.5%, -1.5%]	1
All ❌✅ (primary)	-	-	0

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.2%	[5.2%, 5.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-2.9%, -2.9%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-1.5%, -1.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 791.022s -> 790.339s (-0.09%)
Artifact size: 335.04 MiB -> 335.02 MiB (-0.01%)

Rwlock downgrade Tracking Issue: #128203 This PR adds a `downgrade` method for `RwLock` / `RwLockWriteGuard` on all currently supported platforms. Outstanding questions: - [x] ~~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...~~ - [x] ~~Should the SOLID platform implementation [be ported over](rust-lang/rust#128219 (comment)) to the `queue.rs` implementation to allow it to support downgrades?~~

rustbot assigned Amanieu Jul 26, 2024

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 mentioned this pull request Jul 26, 2024

Tracking Issue for rwlock_downgrade #128203

Open

9 tasks

joboet reviewed Jul 26, 2024

View reviewed changes

library/std/src/sys/sync/rwlock/solid.rs Outdated Show resolved Hide resolved

joboet reviewed Jul 26, 2024

View reviewed changes

library/std/src/sync/rwlock.rs Outdated Show resolved Hide resolved

connortsui20 mentioned this pull request Jul 26, 2024

Add a RwLock downgrade method rust-lang/libs-team#392

Closed

rustbot assigned joboet and unassigned Amanieu Jul 27, 2024

This comment has been minimized.

Sign in to view

connortsui20 commented Jul 31, 2024

View reviewed changes

library/std/src/sys/sync/rwlock/queue.rs Outdated Show resolved Hide resolved

rustbot assigned Mark-Simulacrum and unassigned joboet Aug 16, 2024

connortsui20 marked this pull request as ready for review August 16, 2024 20:08

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

connortsui20 force-pushed the rwlock-downgrade branch from 5b1daa6 to e7d0803 Compare August 16, 2024 20:27

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

connortsui20 commented Aug 16, 2024

View reviewed changes

library/std/src/sys/sync/rwlock/futex.rs Outdated Show resolved Hide resolved

rustbot added the has-merge-commits PR has merge commits, merge with caution. label Aug 17, 2024

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 17, 2024

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 Nov 17, 2024

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 Nov 17, 2024

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 Nov 18, 2024

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 Nov 18, 2024

tgross35 added the CI-spurious-fail-msvc CI spurious failure: target env msvc label Nov 18, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 18, 2024

bors merged commit e83c45a into rust-lang:master Nov 18, 2024
13 checks passed

rustbot added this to the 1.84.0 milestone Nov 18, 2024

bors mentioned this pull request Nov 18, 2024

std: allow after-main use of synchronization primitives #132730

Merged

connortsui20 deleted the rwlock-downgrade branch November 18, 2024 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rwlock downgrade #128219

Rwlock downgrade #128219

connortsui20 commented Jul 26, 2024 •

edited

Loading

rustbot commented Jul 26, 2024

connortsui20 commented Jul 26, 2024

Amanieu commented Jul 27, 2024

bors commented Jul 28, 2024

bors commented Jul 29, 2024

kawadakk commented Jul 29, 2024 •

edited

Loading

connortsui20 commented Jul 29, 2024

This comment has been minimized.

joboet commented Aug 16, 2024

rustbot commented Aug 16, 2024

bors commented Nov 17, 2024

rust-log-analyzer commented Nov 17, 2024

bors commented Nov 17, 2024

Kobzol commented Nov 17, 2024

bors commented Nov 18, 2024

rust-log-analyzer commented Nov 18, 2024

bors commented Nov 18, 2024

tgross35 commented Nov 18, 2024

bors commented Nov 18, 2024

bors commented Nov 18, 2024

rust-timer commented Nov 18, 2024

Rwlock downgrade #128219

Rwlock downgrade #128219

Conversation

connortsui20 commented Jul 26, 2024 • edited Loading

rustbot commented Jul 26, 2024

connortsui20 commented Jul 26, 2024

Amanieu commented Jul 27, 2024

bors commented Jul 28, 2024

bors commented Jul 29, 2024

kawadakk commented Jul 29, 2024 • edited Loading

connortsui20 commented Jul 29, 2024

This comment has been minimized.

joboet commented Aug 16, 2024

rustbot commented Aug 16, 2024

bors commented Nov 17, 2024

rust-log-analyzer commented Nov 17, 2024

bors commented Nov 17, 2024

Kobzol commented Nov 17, 2024

bors commented Nov 18, 2024

rust-log-analyzer commented Nov 18, 2024

bors commented Nov 18, 2024

tgross35 commented Nov 18, 2024

bors commented Nov 18, 2024

bors commented Nov 18, 2024

rust-timer commented Nov 18, 2024

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

connortsui20 commented Jul 26, 2024 •

edited

Loading

kawadakk commented Jul 29, 2024 •

edited

Loading