Use conditional synchronization for Lock #111713

Zoxc · 2023-05-18T07:29:47Z

This changes Lock to use synchronization only if mode::is_dyn_thread_safe could be true. This reduces overhead for the parallel compiler running with 1 thread.

The emitters are changed to use DynSend instead of Send so they can still use Lock.

A Rayon thread pool is not used with 1 thread anymore, as session globals contains Locks which are no longer Sync.

Performance improvement with 1 thread and cfg(parallel_compiler):

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check	1.7665s	1.7336s	💚 -1.86%
🟣 hyper:check	0.2780s	0.2736s	💚 -1.61%
🟣 regex:check	0.9994s	0.9824s	💚 -1.70%
🟣 syn:check	1.5875s	1.5656s	💚 -1.38%
🟣 syntex_syntax:check	6.0682s	5.9532s	💚 -1.90%
Total	10.6997s	10.5083s	💚 -1.79%
Summary	1.0000s	0.9831s	💚 -1.69%

cc @SparrowLii

rustbot · 2023-05-18T07:29:53Z

r? @davidtwco

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

rustbot · 2023-05-18T07:29:56Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

compiler/rustc_data_structures/src/sync/lock.rs

Cargo.lock

SparrowLii · 2023-05-23T01:05:13Z

It looks good to me. cc @cjgillot @nnethercote

davidtwco

Apologies for the delay in reviewing this - looks good to me, but I'll re-assign to someone involved in compiler performance/parallel compiler to double check.

davidtwco · 2023-06-27T12:52:30Z

r? @nnethercote

nnethercote

I'm starting to get really uncomfortable with the amount of extra complexity being added for the whole single-thread vs multi-threaded split for the parallel front-end.

More specifically, for this PR, there is an entirely new file, lock.rs, which is 242 lines, implementing a critical low-level type, involving UnsafeCell and a bunch of unsafe blocks, and it has just one two-line comment. That's not enough. Without additional documentation I can't understand the code well enough to review it and give an r+ in good conscience.

I suggest adding:

A top-level comment to lock.rs explaining at a high-level what is going on.
Comments on more of the types, like LockRawUnion and LockRaw.
Comments on and/or within some of the more complex functions.
Comments on unsafe blocks, explaining why they're safe.
Any other comments above and beyond these would also be fine.

nnethercote · 2023-06-28T23:31:09Z

compiler/rustc_data_structures/src/sync/worker_local.rs

@@ -1,4 +1,4 @@
-use crate::sync::Lock;
+use parking_lot::Mutex;


Why the Lock to Mutex changes in this file? Is this code not used in the serial front-end?

It's not used in the serial compiler and it's quite cold.

nnethercote · 2023-06-28T23:34:49Z

compiler/rustc_data_structures/src/sync/lock.rs

+    type Target = T;
+    #[inline]
+    fn deref(&self) -> &T {
+        unsafe { &*self.lock.data.get() }


Why is this unsafe?

The get method on UnsafeCell is not safe.

bors · 2023-07-20T03:55:50Z

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

Dylan-DPC · 2023-08-13T07:16:37Z

@Zoxc any updates on this?

Zoxc · 2023-08-14T16:58:34Z

I've added some documentation to this now.

bors · 2023-08-30T00:51:30Z

⌛ Testing commit 16eae91781eef660e19400d451ce1bab3a1dacf7 with merge e507b3d655f189b6382af2000caf319468f847b4...

bors · 2023-08-30T02:40:26Z

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing e507b3d655f189b6382af2000caf319468f847b4 to master...

bors · 2023-08-30T02:40:30Z

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

Zoxc · 2023-08-30T02:47:45Z

@bors retry

SparrowLii · 2023-08-30T03:18:30Z

@bors retry

nnethercote · 2023-08-30T03:24:45Z

@bors retry

nnethercote · 2023-08-30T04:12:05Z

@bors r+

bors · 2023-08-30T04:12:07Z

📌 Commit d35179f has been approved by nnethercote

It is now in the queue for this repository.

rust-timer · 2023-08-30T07:47:13Z

Finished benchmarking commit (e507b3d655f189b6382af2000caf319468f847b4): 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.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.5%, 0.5%]	1

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)	3.1%	[1.6%, 4.0%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 630.625s -> 631.483s (0.14%)
Artifact size: 315.99 MiB -> 316.06 MiB (0.02%)

bors · 2023-08-30T08:03:47Z

⌛ Testing commit d35179f with merge 61efe9d...

bors · 2023-08-30T09:52:13Z

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing 61efe9d to master...

rust-timer · 2023-08-31T00:35:41Z

Finished benchmarking commit (61efe9d): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Warning ⚠: The following benchmark(s) failed to build:

syn-1.0.89
exa-0.10.1

cc @rust-lang/wg-compiler-performance

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

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)	2.5%	[0.5%, 4.9%]	3
Regressions ❌ (secondary)	2.5%	[1.0%, 4.5%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[0.5%, 4.9%]	3

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

Binary size

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

Bootstrap: 630.791s -> 632.146s (0.21%)
Artifact size: 316.62 MiB -> 316.62 MiB (0.00%)

nnethercote · 2023-08-31T03:11:57Z

We have possible infinite loops for two rustc-perf benchmarks: exa-0.10.1 and syn-1.0.89. If they persist this PR will have to be backed out.

SparrowLii · 2023-08-31T03:17:55Z

@nnethercote How can we re-product this error?

Zoxc · 2023-08-31T03:21:22Z

The changes are quite small without cfg(parallel_compiler) and it passed perf just before merging. It doesn't seem likely that this PR is the cause.

lqd · 2023-08-31T22:01:36Z

The latest results have 13 benchmarks for syn, while this PR has 11, so it seems like whatever happened was a transient issue maybe?

pnkfelix · 2023-09-05T15:34:28Z

The instruction-count regression(s) here appear spurious from what I can see.

@rustbot label: +perf-regression-triaged

rustbot assigned davidtwco May 18, 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 18, 2023

This comment has been minimized.

Sign in to view

Zoxc force-pushed the lock-switch branch from 3d2e234 to 49994f5 Compare May 18, 2023 07:50

SparrowLii reviewed May 18, 2023

View reviewed changes

compiler/rustc_data_structures/src/sync/lock.rs Outdated Show resolved Hide resolved

compiler/rustc_data_structures/src/sync/lock.rs Show resolved Hide resolved

SparrowLii reviewed May 18, 2023

View reviewed changes

compiler/rustc_data_structures/src/sync/lock.rs Outdated Show resolved Hide resolved

compiler/rustc_data_structures/src/sync/lock.rs Show resolved Hide resolved

Cargo.lock Outdated Show resolved Hide resolved

SparrowLii mentioned this pull request May 19, 2023

Use only one shard with a single thread #111755

Merged

Zoxc force-pushed the lock-switch branch from 49994f5 to c70125d Compare May 19, 2023 11:56

Zoxc force-pushed the lock-switch branch from c70125d to 9fa7f3c Compare May 30, 2023 20:52

davidtwco approved these changes Jun 27, 2023

View reviewed changes

rustbot assigned nnethercote and unassigned davidtwco Jun 27, 2023

nnethercote reviewed Jun 28, 2023

View reviewed changes

nnethercote added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2023

SparrowLii mentioned this pull request Jul 5, 2023

Tracking Issue for Parallel Rustc Front-end #113349

Open

28 tasks

SparrowLii mentioned this pull request Jul 12, 2023

Don't require each rustc_interface tool to opt-in to parallel_compiler #113606

Merged

This was referenced Aug 3, 2023

use RefLock in GatedSpans of Session #107439

Closed

refactor Lock for parallel compiler #109467

Closed

Zoxc force-pushed the lock-switch branch from 9fa7f3c to bf0540e Compare August 14, 2023 15:09

Zoxc mentioned this pull request Aug 16, 2023

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

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 30, 2023

Zoxc added 2 commits August 30, 2023 06:10

Use conditional synchronization for Lock

5739349

Don't use wait_for_query without the Rayon thread pool

d35179f

Zoxc force-pushed the lock-switch branch from 16eae91 to d35179f Compare August 30, 2023 04:10

bors merged commit 61efe9d into rust-lang:master Aug 30, 2023

rustbot added this to the 1.74.0 milestone Aug 30, 2023

bors mentioned this pull request Aug 30, 2023

Add a CurrentGcx type to let the deadlock handler access TyCtxt #115220

Merged

Zoxc deleted the lock-switch branch August 30, 2023 16:09

rustbot added the perf-regression Performance regression. label Aug 31, 2023

SparrowLii mentioned this pull request Aug 31, 2023

test for parallel compiler by default #115235

Closed

Zoxc mentioned this pull request Sep 4, 2023

Add FreezeLock type and use it to store Definitions #115401

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use conditional synchronization for Lock #111713

Use conditional synchronization for Lock #111713

Zoxc commented May 18, 2023

rustbot commented May 18, 2023

rustbot commented May 18, 2023

This comment has been minimized.

SparrowLii commented May 23, 2023

davidtwco left a comment

davidtwco commented Jun 27, 2023

nnethercote left a comment

nnethercote Jun 28, 2023

Zoxc Aug 14, 2023

nnethercote Jun 28, 2023

Zoxc Aug 14, 2023

bors commented Jul 20, 2023

Dylan-DPC commented Aug 13, 2023

Zoxc commented Aug 14, 2023

bors commented Aug 30, 2023

bors commented Aug 30, 2023

bors commented Aug 30, 2023

Zoxc commented Aug 30, 2023

SparrowLii commented Aug 30, 2023

nnethercote commented Aug 30, 2023

nnethercote commented Aug 30, 2023

bors commented Aug 30, 2023

rust-timer commented Aug 30, 2023

bors commented Aug 30, 2023

bors commented Aug 30, 2023

rust-timer commented Aug 31, 2023

nnethercote commented Aug 31, 2023

SparrowLii commented Aug 31, 2023

Zoxc commented Aug 31, 2023

lqd commented Aug 31, 2023

pnkfelix commented Sep 5, 2023

		@@ -1,4 +1,4 @@
		use crate::sync::Lock;
		use parking_lot::Mutex;

Use conditional synchronization for Lock #111713

Use conditional synchronization for Lock #111713

Conversation

Zoxc commented May 18, 2023

rustbot commented May 18, 2023

rustbot commented May 18, 2023

This comment has been minimized.

SparrowLii commented May 23, 2023

davidtwco left a comment

Choose a reason for hiding this comment

davidtwco commented Jun 27, 2023

nnethercote left a comment

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

Choose a reason for hiding this comment

bors commented Jul 20, 2023

Dylan-DPC commented Aug 13, 2023

Zoxc commented Aug 14, 2023

bors commented Aug 30, 2023

bors commented Aug 30, 2023

bors commented Aug 30, 2023

Zoxc commented Aug 30, 2023

SparrowLii commented Aug 30, 2023

nnethercote commented Aug 30, 2023

nnethercote commented Aug 30, 2023

bors commented Aug 30, 2023

rust-timer commented Aug 30, 2023

Overall result: no relevant changes - no action needed

bors commented Aug 30, 2023

bors commented Aug 30, 2023

rust-timer commented Aug 31, 2023

Overall result: ❌ regressions - ACTION NEEDED

nnethercote commented Aug 31, 2023

SparrowLii commented Aug 31, 2023

Zoxc commented Aug 31, 2023

lqd commented Aug 31, 2023

pnkfelix commented Sep 5, 2023