Add FreezeLock type and use it to store Definitions

[Zoxc]

This adds a FreezeLock type which allows mutation using a lock until the value is frozen where it can be accessed lock-free. It's used to store Definitions in Untracked instead of a RwLock. Unlike the current scheme of leaking read guards this doesn't deadlock if definitions is written to after no mutation are expected.

[oli-obk]

Have you benchmarked using the std RwLock unconditionally for Definitions?

[Zoxc]

RwLock is currently unconditionally parking_lot::RwLock for the parallel compiler. I wouldn't expect the std one to be better.

[oli-obk]

I thought this PR used Freeze in the nonparallel compiler, too? I meant trying out using RwLock there, too.

[oli-obk]

The new datastructure is fancy enough that I think it should have a test suite with various possible executions, and it should pass miri and loom.

But before we go there, it may be good to actually show that this is going to be faster than a lock or rwlock with some benchmarks. If this is meant to get the perf of the non-parallel compiler, it may be good to try out switching to std::sync::RwLock on the non-parallel compiler and running it through the perf suite.

[Zoxc]

I'm still a bit confused about your RwLock suggestions.

The main benefit here and in #115418 is the nicer semantics over RwLock. In particular trying to write once frozen does not deadlock, unlike the current scheme of leaking read guards. I plan to apply it to CStore too. That might be hot enough to show a performance benefit on multiple threads.

There does seem to be a bit of lock contention on Definitions before freezing, so this should probably use RwLock inside. I wonder where that contention is, maybe they end up frozen a bit late currently.

[oli-obk]

That makes a lot of sense. Please include this information in the main post, or even better the documentation on Freeze, comparing it with the RwLock usage.

Let's see what perf says about it in the single threaded case

@bors try @rust-timer queue

[bors]

⌛ Trying commit 4b90650cfed7714649dd4fce2026911d73dddc56 with merge fbe9d41f54b8ad93a882dc9bb0d841c032925d5c...

[bors]

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

[bors]

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

[vacuus]

Out of curiosity, since I'm struggling to understand memory ordering: in the case where read observes that frozen is true, does the Acquire load happen-before the Release store in freeze, which connects with the happens-before sequence of the lock accesses? That would ensure that read returns the proper frozen value after all writes are done even without locking, right?

Thank you very much for your time!

[oli-obk]

Using RwLock in non-parallel rustc does have a performance overhead

I tried this out in #115557. Looks like incremental encoding invokes these functions a lot after the data structure gets frozen.

[oli-obk]

The new datastructure is fancy enough that I think it should have a test suite with various possible executions, and it should pass miri and loom.

I still believe this, but it's simple enough, so 🤷

r=me after a rebase

[Zoxc]

I rebased it and changed the guards to pass a reference to the lock to deal with the issue of passing guards as function parameters.

[Zoxc]

@vacuus The store happens before the load, otherwise that sounds right.

[oli-obk]

@bors r+

[bors]

📌 Commit 35e8b67 has been approved by oli-obk

It is now in the queue for this repository.

[bors]

⌛ Testing commit 35e8b67 with merge a0c28cd...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing a0c28cd to master...

[rust-timer]

Finished benchmarking commit (a0c28cd): 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

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.3%	[0.2%, 0.4%]	11
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.4%]	11

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.1%	[2.7%, 3.5%]	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.2%	[1.2%, 1.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[1.2%, 1.2%]	1

Binary size

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

Bootstrap: 627.8s -> 627.776s (-0.00%)
Artifact size: 317.88 MiB -> 317.73 MiB (-0.05%)

[Zoxc]

There's some incremental regressions, so I tested the parallel compiler in that scenario:

Benchmark with 2 threads	Before	After
	Time	Time	%
🟣 diesel:check:unchanged	2.0623s	2.0533s	-0.43%
🟣 syn:check:unchanged	0.6457s	0.6443s	-0.22%
Total	2.7080s	2.6976s	-0.38%
Summary	1.0000s	0.9967s	-0.33%

Benchmark with 6 threads	Before	After
	Time	Time	%
🟣 diesel:check:unchanged	2.2563s	2.2439s	-0.55%
🟣 syn:check:unchanged	0.7443s	0.7397s	-0.62%
Total	3.0005s	2.9836s	-0.56%
Summary	1.0000s	0.9942s	-0.58%

It seems like this more of a serial / parallel compiler tradeoff, performance wise.

[pnkfelix]

• The impact here is hypothesized to be due to serial/parallel trade-off; we benchmark the serial case and observe a small regression, while the parallel case is observing an improvement of roughly the same caliber.

@rustbot label: +perf-regression-triaged