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:27:43Z

-                            PendingDecl::Ready(Some(import_decl)) => {
+                            PendingDecl::Ready(Some(decl)) => {
+                                // We need the `target`, `source` can be extracted
+                                let import_decl = this.new_import_decl(decl, import);


This was previously in resolve_import causing arena allocations in parellel loop on a non-concurrent arena.

View changes since the review

This change can also be merged before the main parallelization PR.

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:20:52Z

+    // Safety check to make sure we don't allocate during speculative resolution.
+    // The arenas are not concurrency safe.
+    #[track_caller]
+    fn arenas(&self) -> &'ra ResolverArenas<'ra> {


Hm, how does this improve safety if the arenas field is still available for use without any checks?

View changes since the review

I have no idea how I could enforce that. I just changed every direct use to use this method instead.

I first made a seperate struct CondAccess which required the resolver as an argument to access the underlying T. But the code started to look very weird:

self.arenas.get(self)...

I wonder why accesses to arenas compile if the arenas are not actually thread safe.
Note that the same arena types are actively used from other parts of the compiler where the parallelism is already enabled.

Because I unsafely implemented DynSend and DynSync for CmResolver. otherwise this all won't work. THere are a lot of fields in the Resolver which are not thread safe, but we only allow mutable access to those fields if specified by the creator of CmResolver (so safe unsafe...). Arena can be mutated through &self.

also (i think):

unsafe impl<T: Send> Send for TypedArena<T> {}

Note that the same arena types are actively used from other parts of the compiler where the parallelism is already enabled.

pub arena: &'tcx WorkerLocal<Arena<'tcx>>, pub hir_arena: &'tcx WorkerLocal<hir::Arena<'tcx>>,

Other parts of the compiler use per-thread arenas.

That should do the trick indeed. Did not know of that.

We could probably use it for the cycle_detector as well, no?

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

petrochenkov · 2026-07-06T16:44:49Z

+    pub(crate) fn build_reduced_graph_external(
+        &self,
+        module: ExternModule<'ra>,
+    ) -> FxIndexMap<BindingKey, NameResolutionRef<'ra>> {


Changes like this can be merged before the main parallelization PR.

View changes since the review

petrochenkov · 2026-07-06T16:47:12Z

            let used = self.process_macro_use_imports(item, module);
-            let decl = self.r.arenas.new_pub_def_decl(module.res().unwrap(), sp, expansion);
+            let decl =
+                self.r.extern_arenas.lock().new_pub_def_decl(module.res().unwrap(), sp, expansion);


build_reduced_graph_for_extern_crate is never called from speculative import resolution, not sure why extern_arenas are used here.

View changes since the review

Because of the *_extern_crate. I wanted it to be uniform.

We should probably change the extern_arenas to speculative_arena (or smth else) then?

Or sync_arena (if the separate arena is necessary at all), I'd rather use it only where the locking is actually necessary.

it would be nice to have a datastructure that only does synchronised access during speculative resolution. That would allow us to use only 1 arena.

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.

…l import resolution.

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

Using #159019 for the arena work.

also handled comments of the reviewer.

@rustbot ready.

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 :).

rust-log-analyzer · 2026-07-09T14:55:14Z

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

checking C++ file formatting
spellchecking files
building external tool typos from package typos-cli@1.38.1
finished building tool typos
error: `recurisve` should be `recursive`
    ╭▸ compiler/rustc_resolve/src/build_reduced_graph.rs:126:59
    │
126 │                 // No reentrant locking possible, so do a recurisve call with lock
    ╰╴                                                          ━━━━━━━━━
rerun with `--bless` to fix typos: `./x.py test tidy --extra-checks=spellcheck --bless`
tidy [extra_checks:spellcheck]: checks with external tool 'typos' failed
tidy [extra_checks:spellcheck]: FAIL
yarn install v1.22.22
warning package.json: No license field
warning ../../package.json: License should be a valid SPDX license expression
warning No license field
[1/4] Resolving packages...
---
Running eslint on rustdoc JS files
info: ES-Check: checking 7 files...
info: ✓ ES-Check passed! All files are ES10 compatible.
typechecking javascript files
tidy: The following check failed: extra_checks:spellcheck
Bootstrap failed while executing `test src/tools/tidy tidyselftest --extra-checks=py,cpp,js,spellcheck`
Command `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools-bin/rust-tidy --root-path=/checkout --cargo-path=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo --output-dir=/checkout/obj/build --concurrency=4 --npm-path=/node/bin/yarn --ci=true --extra-checks=py,cpp,js,spellcheck` failed with exit code 1
Created at: src/bootstrap/src/core/build_steps/tool.rs:1625:23
Executed at: src/bootstrap/src/core/build_steps/test.rs:1646:29

--- BACKTRACE vvv
   0: <bootstrap::utils::exec::DeferredCommand>::finish_process
             at /checkout/src/bootstrap/src/utils/exec.rs:939:17
   1: <bootstrap::utils::exec::DeferredCommand>::wait_for_output::<&bootstrap::utils::exec::ExecutionContext>
             at /checkout/src/bootstrap/src/utils/exec.rs:831:21
   2: <bootstrap::utils::exec::ExecutionContext>::run
             at /checkout/src/bootstrap/src/utils/exec.rs:741:45
   3: <bootstrap::utils::exec::BootstrapCommand>::run::<&bootstrap::core::builder::Builder>
             at /checkout/src/bootstrap/src/utils/exec.rs:339:27
   4: <bootstrap::core::build_steps::test::Tidy as bootstrap::core::builder::Step>::run
             at /checkout/src/bootstrap/src/core/build_steps/test.rs:1646:29
   5: <bootstrap::core::builder::Builder>::ensure::<bootstrap::core::build_steps::test::Tidy>
             at /checkout/src/bootstrap/src/core/builder/mod.rs:1607:36
   6: <bootstrap::core::build_steps::test::Tidy as bootstrap::core::builder::Step>::make_run
             at /checkout/src/bootstrap/src/core/build_steps/test.rs:1568:21
   7: <bootstrap::core::builder::StepDescription>::maybe_run
             at /checkout/src/bootstrap/src/core/builder/mod.rs:476:13
   8: bootstrap::core::builder::cli_paths::match_paths_to_steps_and_run
             at /checkout/src/bootstrap/src/core/builder/cli_paths.rs:232:18
   9: <bootstrap::core::builder::Builder>::run_step_descriptions
             at /checkout/src/bootstrap/src/core/builder/mod.rs:1140:9
  10: <bootstrap::core::builder::Builder>::execute_cli
             at /checkout/src/bootstrap/src/core/builder/mod.rs:1119:14
  11: <bootstrap::Build>::build
             at /checkout/src/bootstrap/src/lib.rs:803:25
  12: bootstrap::main
             at /checkout/src/bootstrap/src/bin/main.rs:130:11
  13: <fn() as core::ops::function::FnOnce<()>>::call_once
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/core/src/ops/function.rs:250:5
  14: std::sys::backtrace::__rust_begin_short_backtrace::<fn(), ()>
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/std/src/sys/backtrace.rs:166:18
  15: std::rt::lang_start::<()>::{closure#0}
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/std/src/rt.rs:206:18
  16: <&dyn core::ops::function::Fn<(), Output = i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe as core::ops::function::FnOnce<()>>::call_once
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/core/src/ops/function.rs:287:21
  17: std::panicking::catch_unwind::do_call::<&dyn core::ops::function::Fn<(), Output = i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe, i32>
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/std/src/panicking.rs:581:40
  18: std::panicking::catch_unwind::<i32, &dyn core::ops::function::Fn<(), Output = i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe>
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/std/src/panicking.rs:544:19
  19: std::panic::catch_unwind::<&dyn core::ops::function::Fn<(), Output = i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe, i32>
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/std/src/panic.rs:359:14
  20: std::rt::lang_start_internal::{closure#0}
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/std/src/rt.rs:175:24
  21: std::panicking::catch_unwind::do_call::<std::rt::lang_start_internal::{closure#0}, isize>
             at /rustc/0417c25868d6dfbd1c291dfeae950504faa6f790/library/std/src/panicking.rs:581:40
---
  28: __libc_start_main
  29: _start


Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:01:56
  local time: Thu Jul  9 14:51:41 UTC 2026
  network time: Thu, 09 Jul 2026 14:51:41 GMT
##[error]Process completed with exit code 1.
##[group]Run echo "disk usage:"

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

Blocked on #159019.
@rustbot blocked

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 Outdated

LorrensP-2158466 commented Jul 6, 2026

View reviewed changes

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