omit `record_accesses` function when collecting `MonoItem`s #97168

SparrowLii · 2022-05-19T07:22:16Z

This PR fixes the FIXME in the impl of record_accesses function.
[Edit] We can call instantiation_mode when push the MonoItem into neighbors. This avoids extra local variables accesses: SmallVec<[_; 128]>

rust-highfive · 2022-05-19T07:22:19Z

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

cjgillot · 2022-05-22T08:53:42Z

IIUC, the point was to avoid calling instanciation_mode from inside the lock. Why can we now do it?

SparrowLii · 2022-05-23T04:41:08Z

IIUC, the point was to avoid calling instanciation_mode from inside the lock. Why can we now do it?

It makes sense! Sorry for mistaking the meaning of this FIXME earlier. I have made the corresponding changes.

cjgillot · 2022-05-23T12:09:52Z

compiler/rustc_monomorphize/src/collector.rs

+    fn record_accesses<'a>(
+        &mut self,
+        source: MonoItem<'tcx>,
+        new_targets: impl Iterator<Item = &'a (Spanned<MonoItem<'tcx>>, bool)> + ExactSizeIterator,


This can take a simple slice, no need to be more general than necessary.

cjgillot · 2022-05-23T12:10:33Z

compiler/rustc_monomorphize/src/collector.rs

-        for (i, (target, inline)) in new_targets.iter().enumerate() {
-            self.targets.push(*target);
-            if *inline {
+        for (i, target) in new_targets.enumerate() {


Please prefer destructuring the tuple, the intent is clearer.

cjgillot · 2022-05-23T12:11:03Z

compiler/rustc_monomorphize/src/collector.rs

        .into_iter()
-        .filter_map(|root| root.node.is_instantiable(tcx).then_some(root.node))
+        .filter_map(|root| root.0.node.is_instantiable(tcx).then_some(root.0.node))


Can we destructure the tuple here?

cjgillot · 2022-05-23T12:11:26Z

compiler/rustc_monomorphize/src/collector.rs

-            output.extend(methods);
+                .map(|item| {
+                    let item = create_fn_mono_item(tcx, item, source);
+                    let inlined = if is_root {


Where does this logic come from?

Judging inlining does seem a little out of place here. I added the extend method to the MonoItems so it looks more reasonable

cjgillot · 2022-05-23T12:11:57Z

compiler/rustc_monomorphize/src/collector.rs

@@ -226,6 +225,24 @@ pub struct InliningMap<'tcx> {
    inlines: GrowableBitSet<usize>,
 }

+struct MonoItems<'tcx> {


Could you document this struct and its fields?

cjgillot · 2022-05-23T12:13:25Z

compiler/rustc_monomorphize/src/collector.rs

-        collect_items_rec(tcx, neighbour, visited, recursion_depths, recursion_limit, inlining_map);
+    inlining_map.lock_mut().record_accesses(starting_point.node, neighbors.items.iter());
+
+    for neighbour in neighbors.items {


Please destructure the tuple here too.

SparrowLii · 2022-05-24T02:28:04Z

Thanks for reviewing! I made the corresponding changes and it looks clearer

cjgillot

Thanks @SparrowLii! Looking good.
Two nits, and then r=me.

cjgillot · 2022-05-25T13:20:18Z

compiler/rustc_monomorphize/src/collector.rs

+        } else {
+            item.node.instantiation_mode(self.tcx) == InstantiationMode::LocalCopy
+        };
+        self.items.push((item, inlined));


This function can be implemented as self.extend([item]).

cjgillot · 2022-05-25T13:21:56Z

compiler/rustc_monomorphize/src/collector.rs

+struct MonoItems<'tcx> {
+    // Whether the collecting start from the root. The mono
+    // items will not be inlined if so.
+    is_root: bool,


This comment is not accurate. The questions is whether we need to compute instantiation_mode, not about inlining.
Proposed redaction: "When collecting from the root, we do not need to compute whether items will need to be inlined."
Should this be named compute_inlining?

Thanks! Such a comment is indeed more accurate

SparrowLii · 2022-05-26T02:25:54Z

@bors r=cjgillot

bors · 2022-05-26T02:25:57Z

@SparrowLii: 🔑 Insufficient privileges: Not in reviewers

SparrowLii · 2022-05-26T02:31:44Z

@cjgillot It seems that we need you to r+ here

cjgillot · 2022-05-26T06:51:23Z

@bors r+

bors · 2022-05-26T06:51:24Z

📌 Commit 480e603 has been approved by cjgillot

bors · 2022-05-26T11:15:19Z

⌛ Testing commit 480e603 with merge 1ab9893...

bors · 2022-05-26T13:37:29Z

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

rust-timer · 2022-05-26T14:55:59Z

Finished benchmarking commit (1ab9893): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.7%	1.1%	5
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

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

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.4%	2.4%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-1.9%	-2.3%	2
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.9%	-2.9%	1
Improvements 🎉 (secondary)	-3.2%	-3.7%	2
All 😿🎉 (primary)	-2.9%	-2.9%	1

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

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

PR rust-lang#97168 was created to address this comment in `record_accesses`: // We collect this into a `SmallVec` to avoid calling `is_inlining_candidate` in the lock. // FIXME: Call `is_inlining_candidate` when pushing to `neighbors` in `collect_items_rec` // instead to avoid creating this `SmallVec`. It removed the need for the `SmallVec` (a good thing) by introducing `MonoItems`, which introduced some additional complexity (a bad thing). However, the reason the `SmallVec` was being used in the first place -- "to avoid calling `is_inlining_candidate` in the lock" -- isn't necessary, as far as I can tell. Perhaps it was at one point. So this commit just moves what is left of the call to `is_inlining_candidate` -- a call the innocuous `instantiation_mode` -- inside the lock. This avoids the complexity of `MonoItems` sometimes computing inlining data and sometimes not.

rust-highfive assigned cjgillot May 19, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 19, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2022

SparrowLii force-pushed the accesses branch from 916c677 to 4aaeeb3 Compare May 23, 2022 04:40

cjgillot reviewed May 23, 2022

View reviewed changes

SparrowLii force-pushed the accesses branch from 4aaeeb3 to 554e111 Compare May 24, 2022 02:26

SparrowLii requested a review from cjgillot May 25, 2022 13:15

cjgillot approved these changes May 25, 2022

View reviewed changes

call instantiation_mode when pushing a new mono_item

480e603

SparrowLii force-pushed the accesses branch from 554e111 to 480e603 Compare May 26, 2022 01:37

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 May 26, 2022

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

bors merged commit 1ab9893 into rust-lang:master May 26, 2022

rustbot added this to the 1.63.0 milestone May 26, 2022

nnethercote mentioned this pull request May 31, 2023

Remove MonoItems. #112126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

omit `record_accesses` function when collecting `MonoItem`s #97168

omit `record_accesses` function when collecting `MonoItem`s #97168

SparrowLii commented May 19, 2022 •

edited

Loading

rust-highfive commented May 19, 2022

cjgillot commented May 22, 2022

SparrowLii commented May 23, 2022

cjgillot May 23, 2022

cjgillot May 23, 2022

cjgillot May 23, 2022

cjgillot May 23, 2022

SparrowLii May 24, 2022

cjgillot May 23, 2022

SparrowLii May 24, 2022

cjgillot May 23, 2022

SparrowLii commented May 24, 2022

cjgillot left a comment

cjgillot May 25, 2022

cjgillot May 25, 2022

SparrowLii May 26, 2022

SparrowLii commented May 26, 2022

bors commented May 26, 2022

SparrowLii commented May 26, 2022

cjgillot commented May 26, 2022

bors commented May 26, 2022

bors commented May 26, 2022

bors commented May 26, 2022

rust-timer commented May 26, 2022

omit record_accesses function when collecting MonoItems #97168

omit record_accesses function when collecting MonoItems #97168

Conversation

SparrowLii commented May 19, 2022 • edited Loading

rust-highfive commented May 19, 2022

cjgillot commented May 22, 2022

SparrowLii commented May 23, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparrowLii commented May 24, 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

SparrowLii commented May 26, 2022

bors commented May 26, 2022

SparrowLii commented May 26, 2022

cjgillot commented May 26, 2022

bors commented May 26, 2022

bors commented May 26, 2022

bors commented May 26, 2022

rust-timer commented May 26, 2022

Footnotes

omit `record_accesses` function when collecting `MonoItem`s #97168

omit `record_accesses` function when collecting `MonoItem`s #97168

SparrowLii commented May 19, 2022 •

edited

Loading