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)

[rust-timer]

Finished benchmarking commit (b4cc008): comparison URL.

Overall result: ❌✅ regressions and improvements - 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 rustc-perf
@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.6%]	83
Regressions ❌ (secondary)	0.3%	[0.1%, 0.8%]	55
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.6%, -0.3%]	6
All ❌✅ (primary)	0.3%	[0.1%, 0.6%]	83

Max RSS (memory usage)

Results (primary 2.2%, secondary 3.0%)

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

	mean	range	count
Regressions ❌ (primary)	2.2%	[0.8%, 3.9%]	4
Regressions ❌ (secondary)	4.2%	[1.5%, 5.8%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	2.2%	[0.8%, 3.9%]	4

Cycles

Results (primary 3.6%, secondary -4.5%)

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

	mean	range	count
Regressions ❌ (primary)	3.6%	[3.6%, 3.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.5%	[-4.5%, -4.5%]	1
All ❌✅ (primary)	3.6%	[3.6%, 3.6%]	1

Binary size

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

Bootstrap: 487.635s -> 488.963s (0.27%)
Artifact size: 387.74 MiB -> 387.89 MiB (0.04%)

[petrochenkov]

Nice, the conditional RwLock shows smaller regressions here.

[petrochenkov]

Wait, why does CmRefCell need to use RwLock?
We can only modify its data if we have exclusive access to it (speculative is disabled), that's the whole point.

[LorrensP-2158466]

Wait, why does CmRefCell need to use RwLock?
We can only modify its data if we have exclusive access to it (speculative is disabled), that's the whole point.

I think I already explained why we need this, though I can't find. Basically, in the functions concerned with retrieving a resolution, we use borrow:

fn resolutions(&self, module: Module<'ra>) -> &'ra Resolutions<'ra> {
    if !module.is_local() {
        // as long as 1 thread is building this external table, all other threads will wait
        module.populate_on_access.call_once(|| {
            *module.lazy_resolutions.borrow_mut_unchecked() =
                self.build_reduced_graph_external(module.expect_extern());
        });
    }
    &module.0.0.lazy_resolutions
}

fn resolution(
    &self,
    module: Module<'ra>,
    key: BindingKey,
) -> Option<ReadGuard<'ra, NameResolution<'ra>>> {
    self.resolutions(module).borrow().get(&key).map(|resolution| resolution.0.borrow())
}

These get called during speculative resolution, so the borrow counter needs to be synchronized. We use CmRefCell for both Resolutions of a module and NameResolution:

type Resolutions<'ra> = CmRefCell<FxIndexMap<BindingKey, NameResolutionRef<'ra>>>;


pub(crate) type NameResolutionRef<'ra> = Interned<'ra, CmRefCell<NameResolution<'ra>>>;

[petrochenkov]

I see, it is due to the remaining *_unchecked methods.
I think getting rid of the unchecked methods and the rwlock in cmrefcell is a hard pre-requisite for landing this.

Could you also extract the structures from mod ref_mut to a small crate, together with a minimized resolver (like struct Resolver { speculative: bool, data: CmRefCell<i32> }), and run various scenarios similar to what the real resolver does through miri? (Involving both reborrow and reborrow_ref in particular.)
I should've probably been paying more attention to the unsafe code changes in #159440.

[LorrensP-2158466]

I see, it is due to the remaining *_unchecked methods.
I think getting rid of the unchecked methods and the rwlock in cmrefcell is a hard pre-requisite for landing this.

Yeah, but we still need interior mutability for those 2 structures during speculative resolution, not to mention the other places it is used for ModuleData:

// Not all fields, only relevant.
struct ModuleData<'ra> {
    /// Mapping between names and their (possibly in-progress) resolutions in this module.
    /// Resolutions in modules from other crates are not populated until accessed.
    lazy_resolutions: Resolutions<'ra>,

    /// Macro invocations that can expand into items in this module.
    unexpanded_invocations: CmRefCell<FxHashSet<LocalExpnId>>,

    glob_importers: CmRefCell<Vec<Import<'ra>>>,
    globs: CmRefCell<Vec<Import<'ra>>>,

    /// Used to memoize the traits in this module for faster searches through all traits in scope.
    traits: CmRefCell<
        Option<Box<[(Symbol, Decl<'ra>, Option<Module<'ra>>, bool /* lint ambiguous */)]>>,
    >,

}

So we need some structure that only allows reads during speculative resolution, and somewhow switches to interior mutability when not in speculative resolution.

I think for some of the fields, something like a CondAccess<T> will work. for example: CondAccess<CacheRefCell> which only grants access to the underlying T if Resolver::assert_speculative is false.

[petrochenkov]

we still need interior mutability for those 2 structures during speculative resolution

If something needs mutability during speculative resolution, it doesn't use Cm(Ref)Cell.
The solution is still to split lazy_resolutions into two versions, for local and external modules, they have very different needs for both synchronization and data that they need to keep.

[LorrensP-2158466]

I am sorry, I worded it wrong:

we still need interior mutability for those 2 structures during speculative resolution

If something needs mutability during speculative resolution, it doesn't use Cm(Ref)Cell.

I meant that we need read access during speculative resolution and interior mutability elsewhere for things wrapped in CmRefCell. This is true for almost every field of Module which has interior mutability besides traits:

• globs: we read from it in resolve_ident_in_module_globs_unadjusted
• unexpanded_invocations: to check if there are any macros still unexpanded to determine if a name can be present in the future
• and local resolutions

So:

If something needs mutability during speculative resolution, it doesn't use Cm(Ref)Cell.

While this is true, that is thus only half of it.

So we need to find some very smart datastructure that can be synchronized or not depending the use of it. Which is very unsafe

[petrochenkov]

I meant that we need read access during speculative resolution and interior mutability elsewhere for things wrapped in CmRefCell.

Yes, but it doesn't mean the data in CmRefCell needs to be additionally protected by a mutex or rwlock.
It is already protected by the assert_speculative flag (which needs to be atomic, most likely).
I guess the main question is how to express it in Rust terms, without plaguing the whole resolver with unsafe.

[LorrensP-2158466]

After a lot of brainstorming I came up with an API like this (not detailed though):

/// A cell that supports:
/// - mutable access in normal resolution (single-threaded)
/// - shared access in speculative resolution (multi-threaded)
struct CmRefCell<T> {
    // mimic RefCell but with some tricks.
}

enum CmBorrow<'a, T> {
    /// Normal resolution: RefCell-like borrow tracking
    Normal(Ref<'a, T>),

    /// Speculative resolution: plain shared reference
    Speculative(&'a T),
}

impl<T> CmRefCell<T> {
    fn borrow(&self, resolver: &CmResolver<'_, '_, '_>) -> CmBorrow<'_, T> {
        if resolver.is_speculative() {
            CmBorrow::Speculative(/* shared access */)
        } else {
            CmBorrow::Normal(/* RefCell borrow */)
        }
    }

    fn borrow_mut(&self, resolver: &CmResolver<'_, '_, '_>) -> RefMut<'_, T> {
        assert!(!resolver.is_speculative());
        /* mutable access */
    }
}

Which has no synchronization because we do not need to mutate anything during speculative resolution, but we can still read. During non-speculative resolution we have the semantics we always had and Deref to &T for CmBorrow should be "zero-cost" if the &T are stored in the same place in both variants (otherwise a union?)

I have a feeling this could work, is this something that you were thinking of as well? Though unsafe is required here.

[LorrensP-2158466]

I am currently merging it over to a seperate crate to check with miri, but in the case you want to take a look and maybe do a perf run, @rustbot ready.

Note it is not done, i am still working on it.

[petrochenkov]

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]

This is temporary, I assume?
unexpanded_invocations should never be modified from speculative resolution.

View changes since the review

[LorrensP-2158466]

Yes, I missed this one.

[petrochenkov]

Is it possible to avoid copypasting std::cell internals, and instead wrap the standard cells and their refs?

View changes since the review

[LorrensP-2158466]

Not really without paying branch costs for every deref.

This:

enum CmBorrow<'b>{
   Speculative(&'b T),
   Normal(Ref<'b, T>),
}

Would have conditionals on every deref (even fully optimized), which seems undesirable to me if its "pretty easy" to not have to do that, especially for deref/as_ref things.

It would be nice if we could reuse std::cell logic, but that is not possible.

[petrochenkov]

Let's maybe try it with the branch first, and then benchmark whether avoiding this branch matters.

[LorrensP-2158466]

Okey (thought that I could "look ahead" :D)

[LorrensP-2158466]

Oops, I totally forgot (sorry) that this:

It would be nice if we could reuse std::cell logic, but that is not possible.

means we can't create a Ref(Mut), so because I had to replicate it, I thought that I might as well share the pointer

[petrochenkov]

Is the second part ("must only ever be used to obtain shared (&T) access") something that the user must maintain? Or it will happen automatically due to the mutable flag checks?

View changes since the review

[LorrensP-2158466]

If the user intends to send RefOrMut across threads, it must pass in mutable = false, to ensure that a &mut T can never be obtained.

[petrochenkov]

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

[LorrensP-2158466]

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]

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

[LorrensP-2158466]

Though I am really inclined to just switch between isize and AtomicIsize. That way we can never go wrong with aliased borrows

View changes since the review