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.

[rust-bors]

☀️ Try build successful (CI)
Build commit: b4f9537 (b4f9537fb47874f97164f0e216d412ec42846adf)
Base parent: 63f05e3 (63f05e3635171e7ac3f9ca78bad6c71052cda5a3)

[rust-timer]

Finished benchmarking commit (b4f9537): 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.2%	[0.2%, 0.3%]	8
Regressions ❌ (secondary)	0.4%	[0.3%, 0.6%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	8

Max RSS (memory usage)

Results (primary 2.2%, secondary -3.8%)

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

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

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

Binary size

Results (primary -0.1%, secondary -0.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)	-0.1%	[-0.1%, -0.0%]	47
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	22
All ❌✅ (primary)	-0.1%	[-0.1%, -0.0%]	47

Bootstrap: 486.982s -> 487.285s (0.06%)
Artifact size: 393.22 MiB -> 393.55 MiB (0.08%)

[petrochenkov]

It looks acceptable now, unused-warnings is a stress benchmark with thousands of imports.
r=me after squashing commits.
@rustbot author

[LorrensP-2158466]

Much better, indeed. squashed to 1 commit, @rustbot ready.

r=me after squashing commits.

Can't do that :)

[petrochenkov]

@bors r+

[rust-bors]

📌 Commit 3056b8e has been approved by petrochenkov

It is now in the queue for this repository.

[JonathanBrouwer]

@bors p=1
@bors retry

[rust-bors]

❗ You can only retry pull requests that are approved and have a previously failed auto build.

[rust-bors]

☀️ Test successful - CI
Approved by: petrochenkov
Duration: 3h 19m 28s
Pushing 0966944 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 7dc2c16 (parent) -> 0966944 (this PR)

Test differences

Show 12 test diffs

12 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 096694416a41840709140eb0fd0ca193d1a3e6ba --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-x86_64-apple: 2h 31m -> 1h 22m (-45.4%)
2. dist-aarch64-apple: 1h 29m -> 1h 53m (+26.4%)
3. dist-x86_64-freebsd: 1h 32m -> 1h 9m (-24.6%)
4. x86_64-gnu-debug: 1h 58m -> 1h 29m (-24.6%)
5. armhf-gnu: 1h 28m -> 1h 8m (-22.1%)
6. dist-powerpc-linux: 1h 30m -> 1h 10m (-21.4%)
7. x86_64-gnu-llvm-21-2: 1h 35m -> 1h 15m (-20.4%)
8. tidy: 2m 11s -> 2m 35s (+18.2%)
9. x86_64-msvc-ext1: 1h 46m -> 2h 4m (+17.2%)
10. dist-x86_64-illumos: 1h 40m -> 1h 53m (+13.6%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (0966944): comparison URL.

Overall result: ❌ regressions - please read:

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.1%, 0.3%]	7
Regressions ❌ (secondary)	0.4%	[0.3%, 0.6%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.1%, 0.3%]	7

Max RSS (memory usage)

Results (primary 3.2%, secondary 1.4%)

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

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

Cycles

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

Binary size

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

Bootstrap: 511.037s -> 485.385s (-5.02%)
Artifact size: 393.19 MiB -> 393.64 MiB (0.12%)