Resolver: Parallelize the import resolution loop by LorrensP-2158466 · Pull Request #158845 · rust-lang/rust

LorrensP-2158466 · 2026-07-06T08:22:15Z

View all comments

Parallelize the import resolution loop using par_filter_map (introduced in this pr).

With the accompanying changes to make datastructures concurrent:

CmRefCell is now CmRwLock because of the borrow counters (nothing changes in single-threaded mode)
Cache(Ref)Cells are now changed into atomics/(rw)locks
populating external resolution tables now use Once to ensure only 1 thread builds the table and all others wait if they need it

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

r? @petrochenkov

LorrensP-2158466 · 2026-07-06T08:22:55Z

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.

LorrensP-2158466 · 2026-07-06T08:23:49Z

    })
 }

+pub fn par_filter_map<I: DynSend, T: IntoIterator<Item = I>, R: DynSend, C: FromIterator<R>>(


almost a mirror of filter_map just with flatten for the nested Option.

View changes since the review

Can we keep the Nones in the map to avoid introducing this?
write_import_resolutions (or whatever place consumes the resulting table) can then skip them.

But ideally we'd do everything in place with par_slice, of course.

LorrensP-2158466 · 2026-07-06T08:25:18Z

+                    match self.get_extern_module_with_lock(parent_id, extern_map_lock) {
+                        Some(module) => break module.expect_extern(),
+                        None => parent_id = self.tcx.parent(parent_id),
+                    }


this previously whent through get_module, but since we dont have reentrant RwLocks and this expects an external module, I decided to do an immediate recursive call with the lock (see line 130 on why)

View changes since the review

LorrensP-2158466 · 2026-07-06T08:27:00Z

+            let determined_imports = Lock::new(Vec::new());
+            let indeterminate_imports = Lock::new(Vec::new());


We could also collect all imports from the parallel loop below with their determinacy and optional resolution and then split them in write_import_resolution.

View changes since the review

LorrensP-2158466 · 2026-07-06T08:32:09Z

    use crate::Resolver;

    /// A wrapper around a mutable reference that conditionally allows mutable access.
    pub(crate) struct RefOrMut<'a, T> {


in principle, anything that returns a mutable reference should now be unsafe because of the DynSync/Send because the caller must guarentee that we are in single-threaded mode. (resolver does that but still).

View changes since the review

LorrensP-2158466 · 2026-07-06T08:33:28Z

        // FIXME: this should be eliminated in the process of migration
        // to parallel name resolution.


we'll probably require it untill be have 2 seperate fields for resolutions in modules. But I couldn't create a nice way of doing that and then having the 3 resolutions methods work.

View changes since the review

petrochenkov · 2026-07-06T16:24:33Z

@bors try @rust-timer queue

…r=<try> Resolver: Parallelize the import resolution loop

petrochenkov · 2026-07-06T16:03:01Z

    }

    #[inline(always)]
+    #[track_caller]


These track callers can cause unintended regressions in other places in the compiler, it's better to benchmark this change separately.

View changes since the review

petrochenkov · 2026-07-06T16:15:05Z

 ) {
-    let arenas = Resolver::arenas();
+    let arenas = Resolver::new_arenas();
+    let extern_arenas = Resolver::new_arenas();


I expected having new locked fields inside the existing ResolverArenas, because we don't need to lock the whole ResolverArenas to do something with one field.
But perhaps this is indeed more convenient for a start.

View changes since the review

Yeah, this was just to make it work.

petrochenkov · 2026-07-06T16:23:56Z


-// FIXME: These are cells for caches that can be populated even during speculative resolution,
-// and should be replaced with mutexes, atomics, or other synchronized data when migrating to
-// parallel name resolution.


Let's keep all the old names (CacheCell, CacheRefCell and also CmRefCell and methods like borrow_mut_unchecked) to avoid mass renaming things in this PR.

View changes since the review

petrochenkov · 2026-07-06T16:26:17Z

    /// A wrapper around a mutable reference that conditionally allows mutable access.
    pub(crate) struct RefOrMut<'a, T> {
-        p: &'a mut T,
+        p: *mut T,


What breaks if we don't make this change?

View changes since the review

Because of reborrow_ref there is no way to "copy" the &mut T through &self.

petrochenkov · 2026-07-06T16:31:36Z

+    pub(crate) struct CmRwLock<T>(RwLock<T>);
+
+    /// SAFETY: We wrap a `RwLock`.
+    unsafe impl<T> DynSync for CmRwLock<T> {}


Is it necessary to add this? Won't this impl be derived automatically?

View changes since the review

rust-bors · 2026-07-06T18:42:55Z

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

rust-timer · 2026-07-06T20:11:45Z

Finished benchmarking commit (3df1fed): comparison URL.

Overall result: ❌ regressions - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.1%, 0.5%]	47
Regressions ❌ (secondary)	1.7%	[0.1%, 10.4%]	19
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.5%]	1
All ❌✅ (primary)	0.3%	[0.1%, 0.5%]	47

Max RSS (memory usage)

Results (primary -0.4%, secondary -0.5%)

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

	mean	range	count
Regressions ❌ (primary)	1.3%	[1.3%, 1.3%]	1
Regressions ❌ (secondary)	2.9%	[1.8%, 3.7%]	5
Improvements ✅ (primary)	-1.2%	[-1.7%, -0.6%]	2
Improvements ✅ (secondary)	-6.1%	[-6.9%, -4.6%]	3
All ❌✅ (primary)	-0.4%	[-1.7%, 1.3%]	3

Cycles

Results (secondary 21.9%)

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)	21.9%	[12.0%, 28.5%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (secondary 0.0%)

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

Bootstrap: 491.018s -> 490.555s (-0.09%)
Artifact size: 388.45 MiB -> 388.55 MiB (0.03%)

petrochenkov · 2026-07-06T20:18:36Z

Yeah, a small instr count regression for import-heavy crates like libc.

What is more interesting that it's a (wall time) regression for serde-1.0.219-threads4, the only benchmark that it's supposed to improve.

LorrensP-2158466 · 2026-07-09T14:44:42Z

Using #159019 for the arena work.

also handled comments of the reviewer.

@rustbot ready.

petrochenkov · 2026-07-09T15:43:12Z

Blocked on #159019.
@rustbot blocked

@petrochenkov

…chenkov Resolver: Wrap arenas in `WorkerLocal` In preperation of parallel import resolution #158845 this pr wraps the resolver arenas in `WorkerLocal` to ensure we don't need any synchronization to access an arena. This is common in the compiler. Also, when building the resolution table of an external module, we first create a seperate table and fill that one, after which we replace the empty table with that new table. (Prep Work for #158845) And a manual inline of `define_extern`. r? @petrochenkov

With the accompanying changes to make datastructures concurrent: - CmRefCell now wraps a `rustc_datastructures::sync::RwLock` because of the borrow counters (does introduce synchronized access in single-threaded mode) - Cache(Ref)Cells are now changed into atomics/(rw)locks - populating external resolution tables now use `Once` to ensure only 1 thread builds the table and all others wait if they need it This is the bare minimum to make the loop work and is not at all optimized, this will follow :).

@petrochenkov

…chenkov Resolver: Wrap arenas in `WorkerLocal` In preperation of parallel import resolution rust-lang/rust#158845 this pr wraps the resolver arenas in `WorkerLocal` to ensure we don't need any synchronization to access an arena. This is common in the compiler. Also, when building the resolution table of an external module, we first create a seperate table and fill that one, after which we replace the empty table with that new table. (Prep Work for rust-lang/rust#158845) And a manual inline of `define_extern`. r? @petrochenkov

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 6, 2026

rustbot assigned petrochenkov Jul 6, 2026

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 6, 2026

LorrensP-2158466 commented Jul 6, 2026

View reviewed changes

Comment thread compiler/rustc_resolve/src/imports.rs

LorrensP-2158466 commented Jul 6, 2026

View reviewed changes

Comment thread compiler/rustc_resolve/src/imports.rs Outdated

LorrensP-2158466 commented Jul 6, 2026

View reviewed changes

Comment thread compiler/rustc_resolve/src/imports.rs Outdated