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.

[petrochenkov]

Maybe finding the max count and pre-allocating might improve it?

The issue may also be in TLS accesses rather than in the data structure.

[LorrensP-2158466]

The issue may also be in TLS accesses rather than in the data structure.

If Vec performs bad as well, we'll have to benchmark the argument way as well then, right?

[rust-bors]

☀️ Try build successful (CI)
Build commit: 3c16dbb (3c16dbb38420b621d7ee70f16a9b4c8f1cc8414f)
Base parent: a774017 (a7740170e5e2f733b32a8206d5bb439c8e8c2fce)

[rust-timer]

Finished benchmarking commit (3c16dbb): 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.6%]	116
Regressions ❌ (secondary)	0.5%	[0.1%, 1.4%]	48
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.1%, 0.6%]	116

Max RSS (memory usage)

Results (primary -4.3%, secondary -1.8%)

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)	-4.3%	[-5.0%, -3.6%]	2
Improvements ✅ (secondary)	-1.8%	[-3.0%, -0.6%]	2
All ❌✅ (primary)	-4.3%	[-5.0%, -3.6%]	2

Cycles

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

Binary size

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

Bootstrap: 482.353s -> 481.03s (-0.27%)
Artifact size: 390.76 MiB -> 390.77 MiB (0.00%)

[LorrensP-2158466]

Lets do the explicit argument way as well, just to be sure.

@rustbot author

[LorrensP-2158466]

Lets do the explicit argument way as well, just to be sure.

Just a heads up, there are a lot of places where we need to insert an explicit cycle detector if we assume a cycle can happen from anywhere.

[petrochenkov]

Would it be correct to not add some NameResolution entries to the cycle detector?
E.g. if the entry is from an external module (we may want to block this on #158207), or if the entry is not an import (not sure what is the precise condition).

[LorrensP-2158466]

That would reduce the amount of entries in the detector, thus reducing the amount of times we access the TLS.

or if the entry is not an import (not sure what is the precise condition)

Going of my knowledge of the language, i think only imports can cause cycles. So one would assume only checking cycles due to imports to be enough, instead of all resolutions.

If you want, I can stash the "detector in argument" changes and see if the above works and then improves the bench results.

[LorrensP-2158466]

Current state of PR is a CycleDetector with Vec as the underlying datastructure (it seemed to suffes less in TLS, maybe indicating it is the best option?)

Some things:

• locally resolver related tests pass, i want to see if CI does the same
• I think the LateResolutionVisitor can probably have a "global" cycle detector as a field like: &mut CycleDetector<'ra>. now i manually checked where the top-level calls originated.
• I feel this solution is pretty fragile, because if you accidentally reset the cycle detector (e.g. recreating it) cycles will be missed.
• local/external split is not done
• only tracking imports is not done.

[petrochenkov]

Let's benchmark this, and then probably revert to another solution.
@bors try @rust-timer queue

[petrochenkov]

Blocked on #158207.
@rustbot blocked

[rust-bors]

☀️ Try build successful (CI)
Build commit: 494adc3 (494adc3519a32b752b84421154f5d59062ed5b49)
Base parent: cddcbec (cddcbec198760511240bf0e728193bf4d700acb4)

[rust-timer]

Finished benchmarking commit (494adc3): 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.8%]	138
Regressions ❌ (secondary)	0.3%	[0.1%, 0.8%]	61
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.1%, 0.8%]	138

Max RSS (memory usage)

Results (primary 0.1%, secondary 3.4%)

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

	mean	range	count
Regressions ❌ (primary)	1.5%	[1.3%, 1.6%]	2
Regressions ❌ (secondary)	3.4%	[3.4%, 3.4%]	1
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-2.5%, 1.6%]	3

Cycles

Results (primary 1.2%, secondary -4.8%)

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.2%, 2.4%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-4.8%	[-4.8%, -4.8%]	1
All ❌✅ (primary)	1.2%	[-2.1%, 2.4%]	4

Binary size

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

Bootstrap: 505.379s -> 504.315s (-0.21%)
Artifact size: 353.06 MiB -> 353.02 MiB (-0.01%)

[LorrensP-2158466]

Almost the same as with Vec in TLS, so I assume it is the extra tracking overhead.

Want to try BTreeSet in this version as well, just to be sure?

Either way, if we would only track local imports, we should get that number down.

[petrochenkov]

Want to try BTreeSet in this version as well, just to be sure?

No, what we already measured is enough