Resolver: Parallelize the import resolution loop

[LorrensP-2158466]

View all comments

Follow up of #159440. This pr implements the parallel part of par_for_each_slice. And implements DynSend and DynSync for RefOrMut and CmCell to make the call to par_for_each_slice compile.

This is the bare minimum to make the parallel loop work and is not at all optimized, this will follow :).

r? @petrochenkov

[LorrensP-2158466]

Lets see what CI says.

I have tried to add comments to changes to show my thought process behind them, but i'll make another pass here as well to be sure.

[petrochenkov]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 3df1fed (3df1fed3aaf1081aa59cfe2228eed289a2ef744e)
Base parent: 3c00c96 (3c00c96d3af4d5b5e101e56cc161a608b21366ee)

[petrochenkov]

decided to create a CmToken instead of using &mut Resolver in these things, seems cleaner to me at least

Again, we'll need to benchmark whether eliding this boolean assert even matters, considering that it's not even atomic.

[LorrensP-2158466]

I'll split the CmRefCell changes as well (first without token). Okey with you?

[petrochenkov]

I'll split the CmRefCell changes as well (first without token). Okey with you?

Yes, after the ResolutionTable lands

[LorrensP-2158466]

Rebased to fix conflicts and cleanup the PR, it now contains:

• Parallel loop, together with impls of DynSync and usage of RwLock in some cases
• The RwLock union impl from Implement rustc_datastructures::RwLock as union over RefCell and parking_lot::RwLock. #159452

@rustbot ready.

[petrochenkov]

• The RwLock union impl from Implement rustc_datastructures::RwLock as union over RefCell and parking_lot::RwLock. #159452

I don't think we'll merge this part, given the unconvincing perf results from the linked PR and the amount of complexity/unsafety.
Could you remove it from this PR? We could benchmark it later separately (but probably still not merge).

[LorrensP-2158466]

Force-pushed to keep up to date (and remove RwLock change) and address both comments + the removal of Cache(Ref)Cell.

i'll wait on #158845 (comment), so i'll mark this @rustbot ready.

[petrochenkov]

Could you move the Cache(Ref)Cell parts to a separate PR so they could be benchmarked separately?
@rustbot author

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[LorrensP-2158466]

rebased to fix conflicts and squash to one commit. @rustbot ready

[petrochenkov]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: dccb2bf (dccb2bfebc75e0c44545ab9e2cd326c835d5a30b)
Base parent: e67cfe8 (e67cfe858e65fc23ae3c7ea118fb7178d6bc4b05)

[rust-timer]

Finished benchmarking commit (dccb2bf): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.9%, secondary 2.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.1%	[2.8%, 3.7%]	4
Regressions ❌ (secondary)	2.9%	[2.9%, 2.9%]	1
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.9%	[-2.5%, 3.7%]	5

Cycles

Results (primary -3.7%, secondary -4.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-3.7%	[-5.3%, -2.0%]	8
Improvements ✅ (secondary)	-4.8%	[-9.5%, -3.0%]	9
All ❌✅ (primary)	-3.7%	[-5.3%, -2.0%]	8

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 488.069s -> 491.874s (0.78%)
Artifact size: 406.51 MiB -> 406.53 MiB (0.00%)

[petrochenkov]

Ok, the implementation is good, now we need any benchmark showing that parallelizing import resolution is actually useful, and we can merge this.
@rustbot author

[LorrensP-2158466]

@petrochenkov Sorry for asking this... 😅

but by "any benchmark" do you mean any parallel benchmark of rustc-perf? or compiling some crates locally and showing that compile times improve with paralel import resolution and -threads>1?

[petrochenkov]

Not necessarily from rustc-perf, just any benchmark that shows green numbers from this change.
Perhaps even a synthetic one with lots of imports, although that would be a bit disappointing (from the standard rustc-perf benchmarks unused-warnings is one such benchmark).