Resolver: Batched Import Resolution

[LorrensP-2158466]

View all comments

Transforms the current algorithm for resolving imports to a batched algorithm. Every import in the indeterminate_imports set is resolved in isolation. This is the only real difference from the current algorithm.

r? petrochenkov

[LorrensP-2158466]

Code looks a little better now and should be more correct than the previous commits, but I have yet to find a way to resolve the current test failures.

[LorrensP-2158466]

Okey, I did fix core not being built, but those other errors happened again. We now resolve the prelude import path when we create such an import at build_reduced_graph phase.

[petrochenkov]

Could you update the tests to make CI green, so I can see the difference?
(Even if the changes do not seem correct.)

[petrochenkov]

Moving prelude_import processing to build_reduced_graph may also be necessary for #139493.

[LorrensP-2158466]

I'll create a pr for it.

[petrochenkov]

Unblocked 🎉
@rustbot ready

[petrochenkov]

Could you stash away the RustEmbed hack temporarily?
We'll first run crater without it.

[LorrensP-2158466]

I stashed RustEmbed and addressed most of the comments, there is just a slight hiccup that i am not getting past atm. I'll explain on the relevant code.

[LorrensP-2158466]

Here in ident.rs during speculative resolution:

rust/compiler/rustc_resolve/src/ident.rs

Lines 1067 to 1086 in b6bed6f

	fn resolve_ident_in_module_globs_unadjusted<'r>(
	mut self: CmResolver<'r, 'ra, 'tcx>,
	module: Module<'ra>,
	ident: IdentKey,
	orig_ident_span: Span,
	ns: Namespace,
	parent_scope: &ParentScope<'ra>,
	shadowing: Shadowing,
	finalize: Option<Finalize>,
	ignore_decl: Option<Decl<'ra>>,
	ignore_import: Option<Import<'ra>>,
	) -> Result<Decl<'ra>, ControlFlow<Determinacy, Determinacy>> {
	let key = BindingKey::new(ident, ns);
	// `try_borrow_mut` is required to ensure exclusive access, even if the resulting binding
	// doesn't need to be mutable. It will fail when there is a cycle of imports, and without
	// the exclusive access infinite recursion will crash the compiler with stack overflow.
	let resolution = &*self
	.resolution_or_default(module, key, orig_ident_span)
	.try_borrow_mut_unchecked()
	.map_err(|_| ControlFlow::Continue(Determined))?;

we introduce a "cycle detector" by getting an exclusive access on the NameResolution (no mutability happens here). So we if ever try to resolve key again, we know it's a cycle and stop.

In the case that the key exists in the Resolutions of the module, I have a very ugly hack to still get the exclusive access without mutably borrowing the CmRefCell. But when it is not present, i don't know what to do.

This function is used here:

rust/compiler/rustc_resolve/src/lib.rs

Lines 2076 to 2086 in b6bed6f

	fn resolution_or_default(
	&self,
	module: Module<'ra>,
	key: BindingKey,
	orig_ident_span: Span,
	) -> &'ra CmRefCell<NameResolution<'ra>> {
	self.resolutions(module)
	.borrow_mut_unchecked()
	.entry(key)
	.or_insert_with(|| self.arenas.alloc_name_resolution(orig_ident_span))
	}

Which inserts an empty name resolution when it's not yet present, then combined with the exclusive access, we thus check cycles for new resolutions. But during speculative resolution, you can't mutably access the modules resolutions because it's wrapped in CmRefCell (local modules, for external we'll need a different strategy), which panicks when mutating the underlying value during speculative resolution.

In serial execution, inserting an empty resolution doesn't really impact anything, but in parallel execution, this will cause a lot of locking. So question is, what should we do here? Because I don't know anything without introducing another structure that tracks which keys are being resolved (which in turn, introduces mutability and locking).

Before your review, I also tried making this all work in parallel, where I stumbled on the exact same issue. I'll try and find a way out, let me know if you think of something.

[petrochenkov]

Let's keep an unchecked update there for now.
I was going to think about that case, it may be nontrivial to fix.

[LorrensP-2158466]

Alright! Rebased to fix conflict and addressed review comments. @rustbot ready

[LorrensP-2158466]

@rustbot ready

[petrochenkov]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: fa4dd1f (fa4dd1f19bfeb3fd235dc7f1406c42404436bef5, parent: 59fd4ef94daa991e6797b5aa6127e824f3067def)

[rust-timer]

Finished benchmarking commit (fa4dd1f): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@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.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	0.3%	[0.2%, 0.7%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.8%	[-0.8%, -0.8%]	1
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	1

Max RSS (memory usage)

Results (primary -0.0%, secondary 3.2%)

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

	mean	range	count
Regressions ❌ (primary)	1.6%	[0.7%, 3.2%]	3
Regressions ❌ (secondary)	3.2%	[2.1%, 4.8%]	3
Improvements ✅ (primary)	-2.4%	[-2.9%, -2.0%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-2.9%, 3.2%]	5

Cycles

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

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 481.762s -> 496.521s (3.06%)
Artifact size: 397.93 MiB -> 397.97 MiB (0.01%)

[petrochenkov]

Nice.
@craterbot check

[craterbot]

👌 Experiment pr-145108-2 created and queued.
🤖 Automatically detected try build fa4dd1f
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-145108-2 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-145108-2 is completed!
📊 161 regressed and 1 fixed (821749 total)
📊 2472 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-145108-2/retry-regressed-list.txt ↩

[petrochenkov]

Looks like rust-embed is still a problem.
Could you rebase, squash commits and re-apply the rust-embed hack commit on top of the other changes?
@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]

Fixed conflicts, squashed and re-applied rust-embed fix. @rustbot ready

[petrochenkov]

I don't see why the intermediate macro_vis_hack_map is necessary, we can check all the conditions right here.

If

• binding is an import that refers to another import X
• X is from macro_use_prelude and has name RustEmbed
• module has a macro namespace binding Y with name RustEmbed in it
• Y is a glob import with public visibility

Then

• bump the binding's visibility to public

[rust-bors]

☔ The latest upstream changes (presumably #153260) made this pull request unmergeable. Please resolve the merge conflicts.