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

Make Sharded an enum and specialize it for the single thread case #114860

Merged
merged 3 commits into from
Aug 24, 2023

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Aug 15, 2023

This changes Sharded to use a single shard by an enum, reducing the size of Sharded for greater cache efficiency.

Performance improvement with 1 thread and cfg(parallel_compiler):

BenchmarkBeforeAfter
TimeTime%
🟣 clap:check1.7009s1.6748s💚 -1.53%
🟣 hyper:check0.2525s0.2451s💚 -2.90%
🟣 regex:check0.9519s0.9353s💚 -1.74%
🟣 syn:check1.5504s1.5280s💚 -1.45%
🟣 syntex_syntax:check5.9536s5.8873s💚 -1.11%
Total10.4092s10.2706s💚 -1.33%
Summary1.0000s0.9825s💚 -1.75%

I did see an unexpected 0.23% change for the serial compiler, so this could use a perf run to see if that reproduces.

cc @SparrowLii

@rustbot
Copy link
Collaborator

rustbot commented Aug 15, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 15, 2023
@compiler-errors
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 15, 2023

⌛ Trying commit c737c62 with merge c2631968523852a0815358b7f3e17eb91a677173...

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 15, 2023
@bors
Copy link
Contributor

bors commented Aug 15, 2023

☀️ Try build successful - checks-actions
Build commit: c2631968523852a0815358b7f3e17eb91a677173 (c2631968523852a0815358b7f3e17eb91a677173)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c2631968523852a0815358b7f3e17eb91a677173): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

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)
3.6% [2.0%, 5.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

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)
1.5% [1.5%, 1.5%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.5% [1.5%, 1.5%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 632.472s -> 632.68s (0.03%)
Artifact size: 346.76 MiB -> 346.72 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 15, 2023
/// a single shard to be used for greater cache efficiency.
/// A single field is used when the compiler uses only one thread.
pub enum Sharded<T> {
Single(Lock<T>),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use RefCell directly if it brought visible performeance benefit. Then we need to add Sharded to marker.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work as we need to return a single type for both cases. With #111713 landed however we can add a lock operation to Sharded that combines the branches on is_dyn_thread_safe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I mean Single(RefCell<T>)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh fn lock_shards requires the return type Lock<T>. Just forget it :)

@rustbot rustbot added the A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) label Aug 16, 2023
@rust-log-analyzer

This comment has been minimized.

@SparrowLii
Copy link
Member

Thanks! It looks good to me. cc @nnethercote @cjgillot

use std::borrow::Borrow;
use std::collections::hash_map::RawEntryMut;
use std::hash::{Hash, Hasher};
use std::mem;

#[cfg(parallel_compiler)]
// 32 shards is sufficient to reduce contention on an 8-core Ryzen 7 1700,
// but this should be tested on higher core count CPUs. How the `Sharded` type gets used
// may also affect the ideal number of shards.
const SHARD_BITS: usize = 5;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still used by get_shard_hash. Should it keep both possible values? Or should get_shard_hash be refactored out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_shard_hash isn't actually used for anything with cfg(not(parallel_compiler)).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, should it be cfg(parallel_compiler) to avoid accidental use?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is still used with cfg(not(parallel_compiler)), but it's value doesn't end up used with optimizations applied as get_shard_by_index ignores the hash then.

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 22, 2023

📌 Commit 0823f0c has been approved by cjgillot

It is now in the queue for this repository.

@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 Aug 22, 2023
@bors
Copy link
Contributor

bors commented Aug 22, 2023

⌛ Testing commit 0823f0c with merge 9cd8d80c729f199c56bac370baa2684a7235ae8e...

@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
6 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
7 


The actual run.stderr differed from the expected run.stderr.
Actual run.stderr saved to C:\a\rust\rust\build\i686-pc-windows-gnu\test\ui\panics\runtime-switch.v0\runtime-switch.v0.run.stderr

error in revision `v0`: 1 errors occurred comparing run output.
status: exit code: 101
command: PATH="C:\a\rust\rust\build\i686-pc-windows-gnu\stage2\lib\rustlib\i686-pc-windows-gnu\lib;C:\a\rust\rust\build\i686-pc-windows-gnu\stage0-bootstrap-tools\i686-pc-windows-gnu\release\deps;C:\a\rust\rust\build\i686-pc-windows-gnu\stage0\bin;C:\a\rust\rust\ninja;C:\a\rust\rust\mingw32\bin;C:\hostedtoolcache\windows\Python\3.11.4\x64\Scripts;C:\hostedtoolcache\windows\Python\3.11.4\x64;C:\msys64\usr\bin;C:\a\rust\rust\sccache;C:\Program Files\MongoDB\Server\5.0\bin;C:\aliyun-cli;C:\vcpkg;C:\cf-cli;C:\Program Files (x86)\NSIS;C:\tools\zstd;C:\Program Files\Mercurial;C:\hostedtoolcache\windows\stack\2.11.1\x64;C:\cabal\bin;C:\ghcup\bin;C:\Program Files\dotnet;C:\mysql\bin;C:\Program Files\R\R-4.3.1\bin\x64;C:\SeleniumWebDrivers\GeckoDriver;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.20.7\x64\bin;C:\hostedtoolcache\windows\Python\3.7.9\x64\Scripts;C:\hostedtoolcache\windows\Python\3.7.9\x64;C:\hostedtoolcache\windows\Ruby\2.5.9\x64\bin;C:\Program Files\OpenSSL\bin;C:\tools\kotlinc\bin;C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\8.0.382-5\x64\bin;C:\Program Files\ImageMagick-7.1.1-Q16-HDRI;C:\Program Files (x86)\Microsoft SDKs\Azure\CLI2\wbin;C:\ProgramData\kind;C:\Program Files\Eclipse Foundation\jdk-8.0.302.8-hotspot\bin;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Windows\System32\OpenSSH;C:\ProgramData\Chocolatey\bin;C:\Program Files\PowerShell\7;C:\Program Files\Microsoft\Web Platform Installer;C:\Program Files\Microsoft SQL Server\130\Tools\Binn;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\170\Tools\Binn;C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit;C:\Program Files (x86)\Microsoft SQL Server\110\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\120\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\130\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\140\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\150\DTS\Binn;C:\Program Files (x86)\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\TortoiseSVN\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:\SeleniumWebDrivers\ChromeDriver;C:\SeleniumWebDrivers\EdgeDriver;C:\Program Files\Amazon\AWSCLIV2;C:\Program Files\Amazon\SessionManagerPlugin\bin;C:\Program Files\Amazon\AWSSAMCLI\bin;C:\Program Files (x86)\Google\Cloud SDK\google-cloud-sdk\bin;C:\Program Files (x86)\Microsoft BizTalk Server;C:\Program Files\LLVM\bin;C:\Users\runneradmin\.dotnet\tools;C:\Users\runneradmin\.cargo\bin;C:\Users\runneradmin\AppData\Local\Microsoft\WindowsApps" "C:\\a\\rust\\rust\\build\\i686-pc-windows-gnu\\test\\ui\\panics\\runtime-switch.v0\\a.exe"
--- stderr -------------------------------
thread 'main' panicked at C:\a\rust\rust\tests\ui\panics\runtime-switch.rs:26:5:
explicit panic
stack backtrace:

@bors
Copy link
Contributor

bors commented Aug 22, 2023

💔 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 Aug 22, 2023
@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 22, 2023

That test failure looks unrelated to this PR. It's also ignored on MSVC. I wonder why MINGW is left out?

@mati865
Copy link
Contributor

mati865 commented Aug 22, 2023

It's also ignored on MSVC. I wonder why MINGW is left out?

It uses different debuginfo and unwinding so often unaffected by many MSVC issues regarding unwinding.
Dunno about the problem here though, looks like backtrace could not be obtained but only with v0 manging scheme. That could be some kind of issue with debuginfo but I don't see how this PR could have caused it.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 23, 2023

Let's do a retry to see if the test is flaky?

@SparrowLii
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 24, 2023

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Aug 24, 2023

📌 Commit 0823f0c has been approved by SparrowLii

It is now in the queue for this repository.

@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 Aug 24, 2023
@bors
Copy link
Contributor

bors commented Aug 24, 2023

⌛ Testing commit 0823f0c with merge 840ed5d...

@bors
Copy link
Contributor

bors commented Aug 24, 2023

☀️ Test successful - checks-actions
Approved by: SparrowLii
Pushing 840ed5d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 24, 2023
@bors bors merged commit 840ed5d into rust-lang:master Aug 24, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Aug 24, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (840ed5d): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.4% [-0.4%, -0.4%] 2
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

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)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.2% [-3.2%, -3.2%] 1
All ❌✅ (primary) - - 0

Cycles

Results

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)
-3.3% [-4.7%, -1.2%] 8
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.3% [-4.7%, -1.2%] 8

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 636.596s -> 632.475s (-0.65%)
Artifact size: 347.02 MiB -> 346.61 MiB (-0.12%)

@Zoxc Zoxc deleted the sharded-layout branch August 24, 2023 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler 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