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.