Use only one shard with a single thread #111755

Zoxc · 2023-05-19T09:23:11Z

This changes Sharded to only access a single shard using a mask set to 0 when a single thread is used, which leads to cache utilization improvements.

Performance improvement with 1 thread and cfg(parallel_compiler):

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check	1.7402s	1.7004s	💚 -2.29%
🟣 hyper:check	0.2633s	0.2550s	💚 -3.12%
🟣 regex:check	0.9716s	0.9482s	💚 -2.41%
🟣 syn:check	1.5679s	1.5358s	💚 -2.05%
🟣 syntex_syntax:check	6.0569s	5.9272s	💚 -2.14%
Total	10.5999s	10.3666s	💚 -2.20%
Summary	1.0000s	0.9760s	💚 -2.40%

cc @SparrowLii

rustbot · 2023-05-19T09:23:17Z

r? @petrochenkov

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

SparrowLii · 2023-05-19T09:31:15Z

Sharded uses Lock internal. I'm thinking if it uses Mutex when is_dyn_thread_safe() == true and RefCell otherwise directly, will it be faster?

This needs to be considered in conjunction with #111713.

SparrowLii · 2023-05-19T09:45:23Z

compiler/rustc_data_structures/src/sharded.rs

    }

    /// The shard is selected by hashing `val` with `FxHasher`.
    #[inline]
    pub fn get_shard_by_value<K: Hash + ?Sized>(&self, val: &K) -> &Lock<T> {
-        if SHARDS == 1 { &self.shards[0].0 } else { self.get_shard_by_hash(make_hash(val)) }
+        self.get_shard_by_hash(if SHARDS == 1 { 0 } else { make_hash(val) })


Should we return &self.shards.get_unchecked(0).0 directly if SHARDS == 1?

It gets masked by 0 later, so it doesn't matter.

Could you make this a branch over cfg(debug_assertions)? I'd rather avoid having some logic over the cfg and some other over the value of SHARDS, even if they are equivalent.

Assuming you mean cfg(parallel_compiler) here, it's still useful to disable just sharding by setting SHARDS to 1 to evaluate how effective sharding is.

petrochenkov · 2023-05-22T17:53:14Z

r? @SparrowLii

SparrowLii · 2023-05-23T01:00:46Z

It looks good to me. I have no privileges so r? @cjgillot

nnethercote · 2023-05-23T04:16:04Z

compiler/rustc_data_structures/src/sharded.rs

-        Sharded { shards: [(); SHARDS].map(|()| CacheAligned(Lock::new(value()))) }
+        Sharded {
+            #[cfg(parallel_compiler)]
+            mask: if is_dyn_thread_safe() { SHARDS - 1 } else { 0 },


mask needs a comment explaining what it's for.

cjgillot

What is the reason for the perf improvement? Is it just a cache effect, or do we gain from skipping some key hashing?

cjgillot · 2023-05-24T17:04:39Z

compiler/rustc_data_structures/src/sharded.rs

+
+    #[inline(always)]
+    fn count(&self) -> usize {
+        self.mask() + 1


This needs a reminder that mask is 2^n - 1, so adding 1 is correct.

cjgillot · 2023-05-24T17:08:32Z

compiler/rustc_data_structures/src/sharded.rs

    }

    /// The shard is selected by hashing `val` with `FxHasher`.
    #[inline]
    pub fn get_shard_by_value<K: Hash + ?Sized>(&self, val: &K) -> &Lock<T> {
-        if SHARDS == 1 { &self.shards[0].0 } else { self.get_shard_by_hash(make_hash(val)) }
+        self.get_shard_by_hash(if SHARDS == 1 { 0 } else { make_hash(val) })


Could you make this a branch over cfg(debug_assertions)? I'd rather avoid having some logic over the cfg and some other over the value of SHARDS, even if they are equivalent.

cjgillot · 2023-05-24T17:10:14Z

@bors try @rust-timer queue

bors · 2023-05-24T17:10:23Z

⌛ Trying commit 4d0150d81e6bd2b63caec8aa801ce8098c7d66eb with merge 0224514e56967ce45ee3658e8baf806c75f63c53...

bors · 2023-05-24T19:26:38Z

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

rust-timer · 2023-05-24T21:37:32Z

Finished benchmarking commit (0224514e56967ce45ee3658e8baf806c75f63c53): 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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.2%	[-5.2%, -5.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)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.9%, -2.3%]	4
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 645.989s -> 646.069s (0.01%)

Zoxc · 2023-05-25T13:34:55Z

What is the reason for the perf improvement? Is it just a cache effect, or do we gain from skipping some key hashing?

It probably just the cache effect and the replacement of the bounds check by a masking operation. Not sure there's any place where hashing can be skipped atm.

cjgillot · 2023-05-28T14:46:40Z

@bors r+

bors · 2023-05-28T14:46:42Z

📌 Commit 8abafd0 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-05-28T15:07:36Z

⌛ Testing commit 8abafd0 with merge 3fae1b9...

bors · 2023-05-28T17:45:18Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 3fae1b9 to master...

rust-timer · 2023-05-28T20:11:47Z

Finished benchmarking commit (3fae1b9): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	-	-	0

Cycles

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

Binary size

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

Bootstrap: 646.068s -> 646.98s (0.14%)

rustbot assigned petrochenkov May 19, 2023

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 May 19, 2023

SparrowLii reviewed May 19, 2023

View reviewed changes

rustbot assigned SparrowLii and unassigned petrochenkov May 22, 2023

rustbot assigned cjgillot and unassigned SparrowLii May 23, 2023

nnethercote reviewed May 23, 2023

View reviewed changes

cjgillot reviewed May 24, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 24, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 24, 2023

Zoxc added 3 commits May 28, 2023 14:08

Use only one shard with a single thread

6620882

Don't access self.mask with a single shard

5843858

Add some comments

8abafd0

Zoxc force-pushed the sharded-switch branch from 4d0150d to 8abafd0 Compare May 28, 2023 13:55

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 May 28, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label May 28, 2023

bors merged commit 3fae1b9 into rust-lang:master May 28, 2023

rustbot added this to the 1.72.0 milestone May 28, 2023

Zoxc deleted the sharded-switch branch May 28, 2023 18:14

SparrowLii mentioned this pull request Jul 5, 2023

Tracking Issue for Parallel Rustc Front-end #113349

Open

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use only one shard with a single thread #111755

Use only one shard with a single thread #111755

Zoxc commented May 19, 2023

rustbot commented May 19, 2023

SparrowLii commented May 19, 2023 •

edited

Loading

SparrowLii May 19, 2023

Zoxc May 19, 2023

cjgillot May 24, 2023

Zoxc May 25, 2023

petrochenkov commented May 22, 2023

SparrowLii commented May 23, 2023

nnethercote May 23, 2023

cjgillot left a comment

cjgillot May 24, 2023

cjgillot May 24, 2023

cjgillot commented May 24, 2023

This comment has been minimized.

bors commented May 24, 2023

bors commented May 24, 2023

This comment has been minimized.

rust-timer commented May 24, 2023

Zoxc commented May 25, 2023

cjgillot commented May 28, 2023

bors commented May 28, 2023

bors commented May 28, 2023

bors commented May 28, 2023

rust-timer commented May 28, 2023

Use only one shard with a single thread #111755

Use only one shard with a single thread #111755

Conversation

Zoxc commented May 19, 2023

rustbot commented May 19, 2023

SparrowLii commented May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented May 22, 2023

SparrowLii commented May 23, 2023

Choose a reason for hiding this comment

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented May 24, 2023

This comment has been minimized.

bors commented May 24, 2023

bors commented May 24, 2023

This comment has been minimized.

rust-timer commented May 24, 2023

Overall result: no relevant changes - no action needed

Zoxc commented May 25, 2023

cjgillot commented May 28, 2023

bors commented May 28, 2023

bors commented May 28, 2023

bors commented May 28, 2023

rust-timer commented May 28, 2023

Overall result: no relevant changes - no action needed

SparrowLii commented May 19, 2023 •

edited

Loading