Avoid unnecessary work in finalize_resolutions_in.

[nnethercote]

If module.opt_def_id() returns None, we can skip most of the work.

r? @lqd

[nnethercote]

This is a 1% win on the c2-chacha-0.3.3 crate (check full build) and has little effect on anything else.

[lqd]

I'm not super familiar with the resolver, and the change seems OK, but I wonder if this could impact resolutions to the modules where it's done lazily, e.g. their possible errors / ambiguities ?

I would trust tests (esp with #[macro_export]) to tell us that if that's the case, but I wonder if we do have tests exercizing this code path, do you happen to know that ?

r=me if that's the case, and if we ever need this PR to be easy to find for bisection:

@bors rollup=never

[nnethercote]

The idea here is that functionality shouldn't be changed.

The current code creates reexports. It doesn't modify anything else while doing so. Then, if if let Some(def_id) = module.opt_def_id() succeeds, it does something with reexports. Otherwise, it does nothing.

I just rearranged things so that reexports isn't built in the "it does nothing" case.

That's why I asked a non-resolve-expert for the review, because it's just a minor code rearrangement :)

[lqd]

To clarify, I meant that for_each_child seemed to be able to mutate resolutions, and was wondering whether we knew of modules "populated on access" and if so, whether finalize_resolutions_id could be involved in populating them ?

rust/compiler/rustc_resolve/src/lib.rs

Lines 550 to 560 in 8308806

	fn for_each_child<R, F>(&'a self, resolver: &mut R, mut f: F)
	where
	R: AsMut<Resolver<'a>>,
	F: FnMut(&mut R, Ident, Namespace, &'a NameBinding<'a>),
	{
	for (key, name_resolution) in resolver.as_mut().resolutions(self).borrow().iter() {
	if let Some(binding) = name_resolution.borrow().binding {
	f(resolver, key.ident, key.ns, binding);
	}
	}
	}

and in turn

rust/compiler/rustc_resolve/src/lib.rs

Lines 1658 to 1664 in 8308806

	fn resolutions(&mut self, module: Module<'a>) -> &'a Resolutions<'a> {
	if module.populate_on_access.get() {
	module.populate_on_access.set(false);
	self.build_reduced_graph_external(module);
	}
	&module.lazy_resolutions
	}

[nnethercote]

r? @cjgillot

[petrochenkov]

whether finalize_resolutions_id could be involved in populating them

Nah, only modules from other crates are lazily populated, and the less of them is populated the better.
Modules without a def-id cannot be extern though, so in this PR is shouldn't make any difference.

[cjgillot]

@bors r+

[bors]

📌 Commit 0e475b5 has been approved by cjgillot

[bors]

⌛ Testing commit 0e475b5 with merge 5f98537...

[bors]

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 5f98537 to master...

[rust-timer]

Finished benchmarking commit (5f98537): comparison url.

Instruction count

• Primary benchmarks: mixed results
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions 😿 (primary)	0.3%	0.5%	7
Regressions 😿 (secondary)	0.5%	0.7%	7
Improvements 🎉 (primary)	-1.0%	-1.0%	2
Improvements 🎉 (secondary)	-2.1%	-2.4%	6
All 😿🎉 (primary)	0.0%	-1.0%	9

Max RSS (memory usage)

Results

• Primary benchmarks: 😿 relevant regression found
• Secondary benchmarks: 😿 relevant regression found

	mean1	max	count2
Regressions 😿 (primary)	2.4%	2.4%	1
Regressions 😿 (secondary)	3.3%	3.3%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.4%	2.4%	1

Cycles

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: no relevant changes found

	mean1	max	count2
Regressions 😿 (primary)	2.2%	2.2%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-3.4%	-4.7%	2
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-1.5%	-4.7%	3

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[nnethercote]

Slightly unexpected perf results, but the wins and losses more or less balance out so I don't think we need to worry about it.

@rustbot label: +perf-regression-triaged