resolve: no allocation in resolve_ident_in(_local)_module_*

[LorrensP-2158466]

View all comments

Somewhat of a follow up of #158207 and #158035. Remove the or_default call of NameResolutions which does an arena allocation. Prep work for parallel import resolution.

• cycle detection now works on module and bindingkey instead of the NameResolution. This should have no impact as NameResolutions are found by that pair.
• Now that we get an Option<NameResolution> the logic changed a bit to work with None. Did this by following the logic of the functions to see what would happen with a completely empty resolution (or_default).

r? @petrochenkov

[petrochenkov]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: c6760d2 (c6760d2c5da41dcbb7ee9b8cd185fa2a9049695b)
Base parent: 3456328 (345632878cffcb4c8e90750e943296b43d16c76e)

[rust-timer]

Finished benchmarking commit (c6760d2): comparison URL.

Overall result: ✅ improvements - no action needed

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.

@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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.4%, -0.2%]	33
Improvements ✅ (secondary)	-0.1%	[-0.2%, -0.1%]	5
All ❌✅ (primary)	-0.2%	[-0.4%, -0.2%]	33

Max RSS (memory usage)

Results (primary 1.0%, secondary -2.3%)

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

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.5%, 1.5%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	1.0%	[0.5%, 1.5%]	2

Cycles

Results (secondary 6.2%)

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)	6.2%	[3.0%, 10.6%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 490.57s -> 486.678s (-0.79%)
Artifact size: 393.68 MiB -> 393.25 MiB (-0.11%)

[LorrensP-2158466]

Nice perf, however both kinds of asserts fail:
kind 1:

let Some(resolution) = self.resolution(module.to_module(), key) else {
    assert!(finalize.is_some() == !module.has_unexpanded_invocations());
    return Err(ControlFlow::Continue(Determinacy::determined(
        !module.has_unexpanded_invocations(),
    )));
};

and kind 2:

if let Some(finalize) = finalize {
    assert!(!module.has_unexpanded_invocations());
    return self.get_mut().finalize_module_binding(
        ident,
        orig_ident_span,
        binding,
        parent_scope,
        finalize,
        shadowing,
    );
}

Kind 1 is a bit fragile, if we are not in finalize but there are unexpanded invocations, it also fails. Kind 2 implies that some modules have unexpanded invocations during finalize, which is weird?

In the early return, I noticed that this assertion does not trigger, which seems a bit more logical:

// finalize implies no unexpanded invocations
assert!(!finalize.is_some() || !module.has_unexpanded_invocations());

[petrochenkov]

Then let's do #158604 (comment) to preserve the existing behavior.

some modules have unexpanded invocations during finalize, which is weird?

That's indeed weird and needs to be investigated, but not necessarily as a part of this PR.

finalize implies no unexpanded invocations

Yeah, "implies" is of course the right condition, not "equals".

[LorrensP-2158466]

Then let's do #158604 (comment) to preserve the existing behavior.

Did this plus the other comments. @rustbot ready

That's indeed weird and needs to be investigated, but not necessarily as a part of this PR.

So it was pretty easy to track, when we collect invocations in the expansion algorithm, we enter build_reduced_graph_for_item, which at the beginning does this:

// ... 
let vis = self.resolve_visibility(&item.vis);
// ...

Which uses try_resolve_visibility with finalize = true:

match self.r.try_resolve_visibility(&self.parent_scope, vis, true) {

pub(crate) fn try_resolve_visibility(
    &mut self,
    parent_scope: &ParentScope<'ra>,
    vis: &ast::Visibility,
    finalize: bool,
) -> Result<Visibility, VisResolutionError> {

so it seems there are multiple levels of finalize? But the above seems to contradict the Finalize invariant:

/// Invariant: if `Finalize` is used, expansion and import resolution must be complete.
#[derive(Copy, Clone, Debug)]
struct Finalize { ... }

Should i open an issue?

[petrochenkov]

The current version doesn't mirror resolve_ident_in_module_globs_unadjusted and doesn't preserve the existing behavior, on main branch finalize_module_binding is called even if resolution is None.
As you found, it causes differences when resolving visibilities.

[petrochenkov]

Which uses try_resolve_visibility with finalize = true:

The visibility resolution is sort of hacky, it has to produce some definite result very early when nothing is ready, and cannot delay resolution. But due to restrictions on visibility paths it probably works correctly.
I'm not sure what will happen if we run the visibility resolution without finalize = true, you could try.

[LorrensP-2158466]

The current version doesn't mirror resolve_ident_in_module_globs_unadjusted and doesn't preserve the existing behavior, on main branch finalize_module_binding is called even if resolution is None.

Indeed, i was a bit fast there, oops. @rustbot ready

I'm not sure what will happen if we run the visibility resolution without finalize = true, you could try.

Alright!

[petrochenkov]

r=me after squashing commits.
@rustbot author
@bors delegate+

[rust-bors]

✌️ @LorrensP-2158466, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, then please make that change and post @bors r=petrochenkov.

View changes since this delegation.

[rustbot]

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

[LorrensP-2158466]

@bors r=petrochenkov

[rust-bors]

📌 Commit d737d29 has been approved by petrochenkov

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.

[rust-log-analyzer]

The job aarch64-apple-macos-26 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Important

For more information how to resolve CI failures of this job, visit this link.

[rust-bors]

💔 Test for dffa685 failed: CI. Failed job:

• auto - aarch64-apple-macos-26 (web logs, enhanced plaintext logs)

[petrochenkov]

@bors retry

[rust-bors]

☀️ Test successful - CI
Approved by: petrochenkov
Duration: 3h 21m 34s
Pushing e2b71ad 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 7c5dac9 (parent) -> e2b71ad (this PR)

Test differences

Show 2 test diffs

2 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 e2b71ade2db4ea263ab0d561d889f3e3795a500d --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: 1h 31m -> 2h 17m (+50.1%)
2. dist-powerpc64le-linux-musl: 1h 8m -> 1h 38m (+43.4%)
3. dist-x86_64-msvc-alt: 2h 5m -> 2h 44m (+30.9%)
4. dist-ohos-x86_64: 1h 2m -> 1h 18m (+25.1%)
5. x86_64-gnu-tools: 1h 7m -> 50m 39s (-24.8%)
6. aarch64-apple: 3h 39m -> 2h 48m (-23.2%)
7. dist-armv7-linux: 1h 15m -> 1h 29m (+18.5%)
8. dist-apple-various: 2h 5m -> 1h 43m (-17.7%)
9. dist-x86_64-musl: 2h 17m -> 1h 54m (-16.6%)
10. pr-check-2: 39m 56s -> 34m 19s (-14.1%)

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 (e2b71ad): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.3%	[0.2%, 0.3%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.4%, -0.2%]	26
Improvements ✅ (secondary)	-0.2%	[-0.3%, -0.1%]	5
All ❌✅ (primary)	-0.2%	[-0.4%, 0.3%]	29

Max RSS (memory usage)

Results (primary 0.9%, secondary 3.6%)

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

	mean	range	count
Regressions ❌ (primary)	1.8%	[1.2%, 2.3%]	2
Regressions ❌ (secondary)	3.6%	[1.5%, 9.0%]	6
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[-0.8%, 2.3%]	3

Cycles

Results (primary 2.2%, secondary 2.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%	[1.8%, 2.4%]	5
Regressions ❌ (secondary)	2.8%	[2.6%, 3.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.2%	[1.8%, 2.4%]	5

Binary size

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

Bootstrap: 488.078s -> 490.049s (0.40%)
Artifact size: 393.70 MiB -> 393.28 MiB (-0.11%)