use `RefLock` in `GatedSpans` of Session #107439

SparrowLii · 2023-01-29T09:40:52Z

RefLock is a thread-safe data structure because it can only be accessed within the thread in which it was created.

The GatedSpans in the Session will only be accessed in a single thread, so we can reduce the access cost in a multi-threaded environment by using RefCell instead of Lock.

rustbot · 2023-01-29T09:40:58Z

r? @estebank

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

Noratrieb · 2023-01-29T10:07:12Z

compiler/rustc_data_structures/src/sync.rs

@@ -460,6 +463,91 @@ impl<T: Clone> Clone for Lock<T> {
    }
 }

+fn next() -> NonZeroUsize {
+    static COUNTER: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(1);
+    NonZeroUsize::new(COUNTER.fetch_add(1, Ordering::SeqCst)).expect("more than usize::MAX threads")


Suggested change

NonZeroUsize::new(COUNTER.fetch_add(1, Ordering::SeqCst)).expect("more than usize::MAX threads")

NonZeroUsize::new(COUNTER.fetch_add(1, Ordering::Relaxed)).expect("more than usize::MAX threads")

cjgillot · 2023-01-29T10:13:32Z

@bors try @rust-timer queue

bors · 2023-01-29T10:13:40Z

⌛ Trying commit 67689b3 with merge 1eb77231c061a947484dbf4d8393427fd2d23ac0...

cjgillot · 2023-01-29T10:11:50Z

compiler/rustc_data_structures/src/sync.rs

+// `RefLock` is a thread-safe data structure because it can
+// only be used within the thread in which it was created.


This comment probably deserves to be on the unsafe impl Sync.

cjgillot · 2023-01-29T10:12:35Z

compiler/rustc_data_structures/src/sync.rs

+
+// `RefLock` is a thread-safe data structure because it can
+// only be used within the thread in which it was created.
+pub struct RefLock<T> {


The name is not very descriptive.
SingleThreadAccess?

cjgillot · 2023-01-29T10:15:33Z

compiler/rustc_data_structures/src/sync.rs

+pub(crate) fn get_thread_id() -> NonZeroUsize {
+    thread_local!(static THREAD_ID: NonZeroUsize = next());
+    THREAD_ID.with(|&x| x)
+}


Don't we have something in std::thread or in rayon for this?

Yea, it looks like we can use std::thread::ThreadId here

Zoxc · 2023-01-29T10:39:04Z

compiler/rustc_data_structures/src/sync.rs

+
+    #[inline(always)]
+    fn assert_thread(&self) {
+        assert_eq!(get_thread_id(), self.thread_id);


This check, the drop check and the Sync impl could be disabled when the parallel compiler is also disabled.

Zoxc · 2023-01-29T10:39:38Z

compiler/rustc_session/src/parse.rs

@@ -32,7 +32,7 @@ pub type CrateCheckConfig = CheckCfg<Symbol>;
 /// used and should be feature gated accordingly in `check_crate`.
 #[derive(Default)]
 pub struct GatedSpans {
-    pub spans: Lock<FxHashMap<Symbol, Vec<Span>>>,
+    pub spans: RefLock<FxHashMap<Symbol, Vec<Span>>>,


Would this be a probably for say, parsing files in parallel?

Zoxc · 2023-01-29T10:45:11Z

This seems to overlap with my OneThread type, basically being OneThread<RefCell<T>>. That's missing the check on drop though.

bors · 2023-01-29T13:00:21Z

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

rust-timer · 2023-01-29T15:40:38Z

Finished benchmarking commit (1eb77231c061a947484dbf4d8393427fd2d23ac0): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +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.8%	[0.3%, 1.2%]	12
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.3%, 1.2%]	12

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.3%	[1.2%, 4.5%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Cycles

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

SparrowLii · 2023-01-30T00:45:17Z

Thanks for reviews! It looks like this runtime check still comes with a large performance penalty. I'll try some other things and in the meantime turn this PR into a draft.

We can still test the performance again by turning off the runtime checks as @Zoxc said.

Dylan-DPC · 2023-08-03T11:13:07Z

@SparrowLii any updates on this? thanks

SparrowLii · 2023-08-03T11:31:10Z

Yea I think this is an out-of-date strategy now. RefLock is almost same with OneThread, and we prefer to use Lock which is more efficient in #111713. As we solved the issue about performance under multi-threads recently, I think we would address this soon.

So I think this draft can be closed now. Thanks for your reminding!

use RefLock in GatedSpans of Session

67689b3

rustbot assigned estebank Jan 29, 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 Jan 29, 2023

Noratrieb reviewed Jan 29, 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 Jan 29, 2023

cjgillot reviewed Jan 29, 2023

View reviewed changes

Zoxc reviewed Jan 29, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 29, 2023

SparrowLii marked this pull request as draft January 30, 2023 00:46

disable thread check in non-parallel mode

6eded0d

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

SparrowLii closed this Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use `RefLock` in `GatedSpans` of Session #107439

use `RefLock` in `GatedSpans` of Session #107439

SparrowLii commented Jan 29, 2023

rustbot commented Jan 29, 2023

Noratrieb Jan 29, 2023

cjgillot commented Jan 29, 2023

This comment has been minimized.

bors commented Jan 29, 2023

cjgillot Jan 29, 2023

cjgillot Jan 29, 2023

cjgillot Jan 29, 2023

SparrowLii Jan 29, 2023

Zoxc Jan 29, 2023

Zoxc Jan 29, 2023

Zoxc commented Jan 29, 2023

bors commented Jan 29, 2023

This comment has been minimized.

rust-timer commented Jan 29, 2023

SparrowLii commented Jan 30, 2023 •

edited

Loading

Dylan-DPC commented Aug 3, 2023

SparrowLii commented Aug 3, 2023 •

edited

Loading

	NonZeroUsize::new(COUNTER.fetch_add(1, Ordering::SeqCst)).expect("more than usize::MAX threads")
	NonZeroUsize::new(COUNTER.fetch_add(1, Ordering::Relaxed)).expect("more than usize::MAX threads")

		// `RefLock` is a thread-safe data structure because it can
		// only be used within the thread in which it was created.

use RefLock in GatedSpans of Session #107439

use RefLock in GatedSpans of Session #107439

Conversation

SparrowLii commented Jan 29, 2023

rustbot commented Jan 29, 2023

Choose a reason for hiding this comment

cjgillot commented Jan 29, 2023

This comment has been minimized.

bors commented Jan 29, 2023

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

Choose a reason for hiding this comment

Zoxc commented Jan 29, 2023

bors commented Jan 29, 2023

This comment has been minimized.

rust-timer commented Jan 29, 2023

Overall result: ❌ regressions - ACTION NEEDED

SparrowLii commented Jan 30, 2023 • edited Loading

Dylan-DPC commented Aug 3, 2023

SparrowLii commented Aug 3, 2023 • edited Loading

use `RefLock` in `GatedSpans` of Session #107439

use `RefLock` in `GatedSpans` of Session #107439

SparrowLii commented Jan 30, 2023 •

edited

Loading

SparrowLii commented Aug 3, 2023 •

edited

Loading