resolve: Explicit Set for detecting resolution cycles

[LorrensP-2158466]

View all comments

Instead of using the borrow_mut counter of a RefCell for a NameResolution for detecting cyclic imports during import resolution, we use an explicit recursion stack that keeps track of the current used NameResolutions.

Because of the upcoming parallelisation of the import resolution algorithm, the current way cannot used in a parallel context.

r? @petrochenkov

[petrochenkov]

It's clear that using a thread-local is more compact, but is it technically possible to pass the "active resolution" set explicitly as a parameter through all the relevant functions? We are already doing that for ignore_decl and ignore_import.

[LorrensP-2158466]

but is it technically possible to pass the "active resolution" set explicitly as a parameter through all the relevant functions?

I will try it and see how it looks.

[LorrensP-2158466]

To be honest, this gets quite cumbersome. There are a lot of functions that use the maybe_resolve_ident_... function. Currently i am just inserting that extra argument into everything where its needed, but its difficult for me to differentiate when it is needed and when not. Even then, like I said, a lot of places need this change then (not that we already did this with CmResolver).

So now i have:

pub(crate) fn maybe_resolve_ident_in_module<'r>(
    // ...
    cycle_detector: &mut ImportCycleDetector<'ra>,
){ ... }

(&mut ...) because of loops, otherwise we need cloning.

But this detecting is not needed everywhere, so we could do this:

pub(crate) fn maybe_resolve_ident_in_module<'r>(
    // ...
    cycle_detector: Option<&mut ImportCycleDetector<'ra>>,
){ ... }

But then we require reborrowing in closures and loops, which is the same story as with CmResolver, regardless of the &mut ... or a &mut BTreeSet inside of the Detector (which requires and extra lifetime parameter.

So before I complete this big refactor i have 2 questions:

• do you think this should be done? Because its technically possible as i see it now.
• if so, should i track everything that uses it, thus using a bare &mut ImportCycleDetector<'ra>. Or letting the caller decide when to track cycles in name resolutions, thus using Option<&mut ...>.

[petrochenkov]

There are a lot of functions that use the maybe_resolve_ident_... function.

I think we need the cycle detector in exactly the same cases as ignore_import: Option<Import<'ra>>, (14 functions have this argument).
Actually, both should be combinable into the same (optional) argument, unless I'm missing something.
If you merge the cycle detector into ignore_import, then you'll only need to change ignore_import's type in signatures and 2-3 places where it's actually constructed or inspected, and not just passed through, I think it's worth trying.

[LorrensP-2158466]

@rustbot ready

The cycle detector was also needed in macro resolution.

[LorrensP-2158466]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[LorrensP-2158466]

So i reproduced that failing CI job locally to try and find the stack trace, here is the minimal version:

// ... recursive behaviour of stack
resolve_ident_in_scope_set_inner at ident.rs:450:33 [opt]
resolve_ident_in_scope_set at ident.rs:397:14 [opt] [inlined]
resolve_path_with_ribs at ident.rs:2018:33 [opt]
maybe_resolve_path at ident.rs:1797:14 [opt]
path_accessible at macros.rs:1284:29 [opt]
expr_to_spanned_string at util.rs:87:24 [opt]
make_format_args at format.rs:188:40 [opt]
expand_format_args_impl at format.rs:1137:44 [opt]
expand_invoc at expand.rs:748:53 [opt] [inlined]
fully_expand_fragment at expand.rs:541:24 [opt]
// ... to start of stack

So it does seem that infinite recursion can happen anywhere, anytime. If you think that the cycle_detector should still be an argument to these resolve calls, then I think its best to just have a separate argument, instead of combining it with ignore_import. Otherwise we could just go with the TLS approach again.

[petrochenkov]

Let's return to the (scoped) TLS approach and just keep doing what we do on the main branch.

[LorrensP-2158466]

I am not quite sure what benefit a scoped TLS would give here, so i am missing something.

With a scoped TLS, do you mean to do this at every top-level resolve call (scoped-thread-local crate)?:

TLS.set(&Detector::default(), || {
    self.resolve(...)
}

Because recursive set will insert a new value and replace it after the closure ends. i can use is_set to conditionally check, but that seems to be the same as a normal TLS.

[petrochenkov]

If scoped TLS is inconvenient, then let's use the regular TLS.
The scoped one just fits the semantics better - we don't use it to pass data between arbitrary function calls, only inside specific call subtrees.
But I agree that it may be hard to identify all the top-level calls.

Because recursive set will insert a new value and replace it after the closure ends. i can use is_set to conditionally check, but that seems to be the same as a normal TLS.

I'd expect set at top level, and with everywhere else.

[LorrensP-2158466]

@rustbot ready

[petrochenkov]

r=me after addressing the remaining comments.
@rustbot author

[LorrensP-2158466]

r=me after

Don't have those privileges, @rustbot ready.

[petrochenkov]

I'll run benchmarks just in case.
@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 6e82f9e (6e82f9e895aeecb8614a122b7dc97abf236ffed0)
Base parent: d806608 (d80660864fab37b060d052e01fd3ec40e4d33d11)

[rust-timer]

Finished benchmarking commit (6e82f9e): 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.5%	[0.2%, 1.2%]	164
Regressions ❌ (secondary)	0.7%	[0.1%, 2.8%]	79
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.2%, 1.2%]	164

Max RSS (memory usage)

Results (primary 0.7%)

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

Cycles

Results (primary 2.3%, secondary 4.0%)

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

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.1%, 2.5%]	2
Regressions ❌ (secondary)	4.0%	[3.0%, 5.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[2.1%, 2.5%]	2

Binary size

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

Bootstrap: 481.965s -> 480.413s (-0.32%)
Artifact size: 390.67 MiB -> 391.11 MiB (0.11%)

[rust-bors]

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