rustc_metadata: Switch module children decoding to an iterator #104730

petrochenkov · 2022-11-22T18:13:47Z

Previously #103578, #103524 and previous PRs simplified it as much as possible.

A couple of cleanup commits is also added.
r? @cjgillot

This is more a cleanup than optimization

to unify proc-macro and non-proc-macro cases in particular.

petrochenkov · 2022-11-22T18:14:07Z

@bors try @rust-timer queue

bors · 2022-11-22T18:14:16Z

⌛ Trying commit 24f2ee1 with merge 5de2b9695133b9722f45ad02503f8b6a85a29a2b...

cjgillot

2 questions, and r=me

cjgillot · 2022-11-22T18:58:10Z

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -204,7 +204,9 @@ impl<'a> Resolver<'a> {
    }

    pub(crate) fn build_reduced_graph_external(&mut self, module: Module<'a>) {
-        for child in self.cstore().module_children_untracked(module.def_id(), self.session) {
+        for child in
+            Vec::from_iter(self.cstore().module_children_untracked(module.def_id(), self.session))


Why do we need the from_iter first?

Due to borrow checker, self is mutably borrowed inside the loop.

cjgillot · 2022-11-22T18:59:49Z

compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -1305,6 +1298,11 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
                    }
                }
            }));
+
+            if let Some(reexports) = tcx.module_reexports(local_def_id) {
+                assert!(!reexports.is_empty());


Suggested change

assert!(!reexports.is_empty());

debug_assert!(!reexports.is_empty());

?

This assert is very cheap (both in absolute value, and relatively to everything else happening here), so I think we can always enable it.

bors · 2022-11-22T20:37:26Z

☀️ Try build successful - checks-actions
Build commit: 5de2b9695133b9722f45ad02503f8b6a85a29a2b (5de2b9695133b9722f45ad02503f8b6a85a29a2b)

rust-timer · 2022-11-22T21:55:54Z

Finished benchmarking commit (5de2b9695133b9722f45ad02503f8b6a85a29a2b): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.6%, -0.3%]	9
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	1
All ❌✅ (primary)	-0.5%	[-0.6%, -0.3%]	9

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-3.2%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

petrochenkov · 2022-11-23T08:38:03Z

@bors r=cjgillot

bors · 2022-11-23T08:38:05Z

📌 Commit 24f2ee1 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2022-11-26T05:41:37Z

⌛ Testing commit 24f2ee1 with merge f8a2e49...

bors · 2022-11-26T08:34:11Z

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

rust-timer · 2022-11-26T10:00:43Z

Finished benchmarking commit (f8a2e49): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

This benchmark run did not return any relevant results for this metric.

petrochenkov added 3 commits November 22, 2022 20:43

rustc_metadata: Do not encode empty reexport lists

f3b5791

This is more a cleanup than optimization

rustc_metadata: Switch module children decoding to an iterator

6a233b5

rustc_metadata: Cleanup to get_module_children

24f2ee1

to unify proc-macro and non-proc-macro cases in particular.

rustbot assigned cjgillot Nov 22, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 22, 2022

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 22, 2022

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2022

cjgillot approved these changes Nov 22, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 22, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 23, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 26, 2022

bors merged commit f8a2e49 into rust-lang:master Nov 26, 2022

rustbot added this to the 1.67.0 milestone Nov 26, 2022

matthiaskrgr mentioned this pull request Jul 27, 2023

ICE: const evaluatable failed for non-unevaluated const #114151

Closed

matthiaskrgr mentioned this pull request Oct 11, 2023

ICE with generic_const_exprs: expected bits of usize, got (Sub: 2_usize, 1_usize): usize #116637

Open

matthiaskrgr mentioned this pull request Sep 1, 2024

ICE: Invalid 'Const' during codegen: UnevaluatedConst {..} #129857

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc_metadata: Switch module children decoding to an iterator #104730

rustc_metadata: Switch module children decoding to an iterator #104730

petrochenkov commented Nov 22, 2022

petrochenkov commented Nov 22, 2022

This comment has been minimized.

bors commented Nov 22, 2022

cjgillot left a comment

cjgillot Nov 22, 2022

petrochenkov Nov 22, 2022

cjgillot Nov 22, 2022

petrochenkov Nov 22, 2022

bors commented Nov 22, 2022

This comment has been minimized.

rust-timer commented Nov 22, 2022

petrochenkov commented Nov 23, 2022

bors commented Nov 23, 2022

bors commented Nov 26, 2022

bors commented Nov 26, 2022

rust-timer commented Nov 26, 2022

	assert!(!reexports.is_empty());
	debug_assert!(!reexports.is_empty());

rustc_metadata: Switch module children decoding to an iterator #104730

rustc_metadata: Switch module children decoding to an iterator #104730

Conversation

petrochenkov commented Nov 22, 2022

petrochenkov commented Nov 22, 2022

This comment has been minimized.

bors commented Nov 22, 2022

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 22, 2022

This comment has been minimized.

rust-timer commented Nov 22, 2022

Overall result: ✅ improvements - no action needed

petrochenkov commented Nov 23, 2022

bors commented Nov 23, 2022

bors commented Nov 26, 2022

bors commented Nov 26, 2022

rust-timer commented Nov 26, 2022

Overall result: no relevant changes - no action needed