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

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

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 · 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.

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

@bors try @rust-timer queue

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

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

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

petrochenkov · 2026-07-27T15:21:25Z

is this something that you were thinking of as well?

Same as #158845 (comment), I cannot answer without repeating the work and trying to implement in myself, but the high-level design seems plausible.

petrochenkov · 2026-07-27T15:27:03Z

I've finally read the safety comments in detail, they look more convincing now.
@rustbot author

LorrensP-2158466 · 2026-07-27T17:39:36Z

I've finally read the safety comments in detail, they look more convincing now.

I'll think that I will extend them a bit and add a comment to the ref_mut module as to why we want these (weird) data structures.

LorrensP-2158466 · 2026-07-27T19:30:27Z

Updated the comments and used RefCell instead of the manual impl, @rustbot ready.

petrochenkov · 2026-07-28T11:09:40Z

Benchmarking the changes in #160064.

LorrensP-2158466 · 2026-07-28T12:20:33Z

I'm also tempted to add extra methods for CmCell and CmRefCell that accept a &mut Resolver as a "token" that we can do mutations without checking assert_speculative.

LorrensP-2158466 · 2026-07-28T14:08:33Z

https://github.com/LorrensP-2158466/ref_mut/tree/main has the code for the miri tests, as you will see (and the comments in this pr explain) CmRefCell is not entirely safe.

LorrensP-2158466 · 2026-07-28T15:53:08Z

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

petrochenkov · 2026-07-28T16:10:16Z

#160064 already showed regressions, so let's benchmark and land this piece by piece, starting from ResolutionTable.

Also blocked on #160090.

petrochenkov · 2026-07-28T16:28:25Z

the assert_speculative flag (which needs to be atomic, most likely).

At least right now it probably doesn't need to be atomic though.
assert_speculative = true, par_for_each_slice start, par_for_each_slice end, and assert_speculative = false all happens on the same thread, so there's happens-before relationship between them.
And par_for_each_slice's start and end are supposed to synchronize with everything that happens between them on all the used threads, as I understand.

petrochenkov · 2026-07-28T16:30:29Z

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 · 2026-07-29T14:15:59Z

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

petrochenkov · 2026-07-29T14:20:37Z

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

Yes, after the ResolutionTable lands

rustbot · 2026-08-19T08:01:41Z

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.

… parallel loop for import resolution. + impl `DynSend`/`DynSync` for all relevant data structures + safety comments.

…ot::RwLock`. Implementation uses the same technique as with `Lock` by using a C enum together with a union around the locking primitives.

LorrensP-2158466 · 2026-08-19T08:13:17Z

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 · 2026-08-19T08:41:44Z

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

petrochenkov · 2026-08-19T10:03:52Z

+    // SAFETY: a shared `RefOrMut` only ever yields `&T`, so letting multiple threads
+    // read through it concurrently is just ordinary shared-reference aliasing; no
+    // thread can obtain `&mut T` through it, so there is no data race to worry about.
+    unsafe impl<'a, T: DynSync> DynSync for RefOrMut<'a, T> {}


This impl is unnecessary and can be removed, I think we discussed this somewhere above.

View changes since the review

petrochenkov · 2026-08-19T10:14:38Z

-                            scope = next.get();
-                            macro_rules_scope.set(scope);
+                            scope = *next.borrow();
+                            *macro_rules_scope.borrow_mut() = scope;


I'm not sure this is algorithmically correct.
We may need to protect a larger piece of logic by a lock, maybe the whole loop.
I need to think (but I'm busy this and next week).

View changes since the review

I am not entirely familiar with the chained scopes of macro_rules!, but I can assume there are no cycles, so this should be correct.

Example chain:

A -> B -> C -> D -> E

If multiple threads try to compress the path A -> E starting at A, then no matter the ordering, they will always end up at E.

t1 reads node A and writes on that node B, then t2 will read B.

t1 and t2 read node A, both will then write B, which is the result we expect.

I see it as a recursive property as well, so it will hold until both threads read E in their last step.

If multiple threads are compressing on the same path, their results will be the same as well. For example: t1 compresses A -> E and t2 will compress C -> E.
Since both only overwrite their respective nodes, A for t1 and C for t2, they can never interfere with each other. It doesn't matter what t2 is doing on its path, because for t1 the full path still "exists", if t2 managed to overwrite C before t1 gets to it, it just does one step less.

petrochenkov · 2026-08-19T10:15:46Z

                }
            };
-            flag_decl.set((PendingDecl::Ready(decl), finalize || finalized, is_open));
+            *flag_decl.write() = (PendingDecl::Ready(decl), finalize || finalized, is_open);


I'm not sure this is algorithmically correct either.
This case is similar to extern module loading and we may need to protect the whole loading process by a single lock.

View changes since the review

Yeah, this needs take a lock for the entire process. Depening on wether the extern crate is resolved do we do work or not.

We should then probably use a Mutex instead of a RwLock.

At this point the Cache(Ref)Cell aliases can be removed and RwLocks and Mutexes can be used directly.

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