gather body owners #98203

kckeiks · 2022-06-17T15:58:08Z

Issue #96341

rust-highfive · 2022-06-17T15:58:13Z

r? @nagisa

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

kckeiks · 2022-06-17T15:59:48Z

r? @cjgillot

cjgillot · 2022-06-17T20:02:48Z

compiler/rustc_middle/src/hir/map/mod.rs

@@ -1337,6 +1368,10 @@ pub(crate) fn hir_crate_items(tcx: TyCtxt<'_>, _: ()) -> ModuleItems {
 }

 fn visit_item(&mut self, item: &'hir Item<'hir>) {
+ if self.tcx.hir().is_body_owner(item.def_id) {


What about this?

Suggested change

if self.tcx.hir().is_body_owner(item.def_id) {

if associated_body(Node::Item(item)).is_some() {

cjgillot · 2022-06-18T08:23:31Z

compiler/rustc_middle/src/hir/map/mod.rs

+ if matches!(ex.kind, ExprKind::Closure { .. })
+ && associated_body(Node::Expr(ex)).is_some()
+ {
+ self.body_owners.push(ex.hir_id.owner);


We want to record the LocalDefId of the closure. hir_id.owner is the one of the closure's surrounding item-like (that we have already recorded in visit_*item).

Suggested change

self.body_owners.push(ex.hir_id.owner);

self.body_owners.push(self.tcx.hir().local_def_id(ex.hir_id));

Likewise for anon consts.

cjgillot · 2022-06-18T08:24:11Z

compiler/rustc_middle/src/hir/map/mod.rs

 self.items.push(item.item_id());
+
+ if self.tcx.hir().is_body_owner(item.def_id) {
+ self.body_owners.push(item.def_id);


This probably pushes the same LocalDefId twice.

Thanks, that was the problem.

cjgillot

Sorry for the delay @kckeiks, I had not realized you were waiting for me.

cjgillot · 2022-07-05T15:46:50Z

compiler/rustc_middle/src/hir/map/mod.rs

+ }
+
+ fn visit_anon_const(&mut self, c: &'hir AnonConst) {
+ if associated_body(Node::AnonConst(c)).is_some() {


This is always true.

cjgillot · 2022-07-05T15:46:57Z

compiler/rustc_middle/src/hir/map/mod.rs

+
+ fn visit_expr(&mut self, ex: &'hir Expr<'hir>) {
+ if matches!(ex.kind, ExprKind::Closure { .. })
+ && associated_body(Node::Expr(ex)).is_some()


associated_body(...).is_some() is always true on closures.

cjgillot · 2022-07-05T15:47:39Z

compiler/rustc_middle/src/hir/map/mod.rs

 self.impl_items.push(item.impl_item_id());
 intravisit::walk_impl_item(self, item)
 }

 fn visit_foreign_item(&mut self, item: &'hir ForeignItem<'hir>) {
+ if associated_body(Node::ForeignItem(item)).is_some() {


This is always false.

cjgillot · 2022-07-05T15:48:02Z

compiler/rustc_middle/src/hir/map/mod.rs

 self.trait_items.push(item.trait_item_id());
 intravisit::walk_trait_item(self, item)
 }

 fn visit_impl_item(&mut self, item: &'hir ImplItem<'hir>) {
+ if associated_body(Node::ImplItem(item)).is_some() {


Could you inline the definition of associated_body here?

cjgillot · 2022-07-05T15:48:06Z

compiler/rustc_middle/src/hir/map/mod.rs

@@ -1281,19 +1291,47 @@ pub(super) fn hir_module_items(tcx: TyCtxt<'_>, module_id: LocalDefId) -> Module
 }

 fn visit_trait_item(&mut self, item: &'hir TraitItem<'hir>) {
+ if associated_body(Node::TraitItem(item)).is_some() {


Could you inline the definition of associated_body here?

cjgillot · 2022-07-05T15:48:10Z

compiler/rustc_middle/src/hir/map/mod.rs

@@ -1271,7 +1276,12 @@ pub(super) fn hir_module_items(tcx: TyCtxt<'_>, module_id: LocalDefId) -> Module
 }

 fn visit_item(&mut self, item: &'hir Item<'hir>) {
+ if associated_body(Node::Item(item)).is_some() {


Could you inline the definition of associated_body here?

cjgillot · 2022-07-05T15:48:46Z

compiler/rustc_middle/src/hir/map/mod.rs

@@ -1327,6 +1374,7 @@ pub(crate) fn hir_crate_items(tcx: TyCtxt<'_>, _: ()) -> ModuleItems {
 trait_items: Vec<TraitItemId>,
 impl_items: Vec<ImplItemId>,
 foreign_items: Vec<ForeignItemId>,
+ body_owners: Vec<LocalDefId>,
 }

 impl<'hir> Visitor<'hir> for CrateCollector<'hir> {


I wonder if we can merge this visitor and the one for hir_module_items using some flag to switch behaviour.

cjgillot · 2022-07-05T15:50:28Z

compiler/rustc_middle/src/hir/map/mod.rs

@@ -464,6 +464,23 @@ impl<'hir> Map<'hir> {
 }
 }

+ /// Returns true if the `LocalDefId` corresponds to a body owner and false otherwise.
+ pub fn is_body_owner(self, def_id: LocalDefId) -> bool {


Is this used anywhere?

Sorry, I forgot to remove this.

cjgillot · 2022-07-05T15:50:42Z

compiler/rustc_middle/src/hir/map/mod.rs

+ | DefKind::AnonConst
+ | DefKind::Ctor(..)
+ | DefKind::Fn
+ | DefKind::AssocFn


This case is possibly wrong.

cjgillot · 2022-07-05T15:50:50Z

compiler/rustc_middle/src/hir/map/mod.rs

+ | DefKind::AssocFn
+ | DefKind::Closure
+ | DefKind::Generator
+ | DefKind::Static(..)


This one too.

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

bors · 2022-07-13T01:58:56Z

💔 Test failed - checks-actions

cjgillot · 2022-07-13T08:30:06Z

@bors retry

bors · 2022-07-13T14:14:47Z

⌛ Testing commit 75ffbb021cc96d6032fe5cad46153e401dc51802 with merge adc213b9b01134a4bcfc3efdac1e293946e6ab2d...

bors · 2022-07-13T14:32:28Z

💔 Test failed - checks-actions

cjgillot · 2022-07-13T15:33:42Z

compiler/rustc_middle/src/hir/map/mod.rs

- }
- });
+
+ par_iter(self.tcx.hir_crate_items(()).body_owners.to_vec()).for_each(|def_id| f(def_id));


Suggested change

par_iter(self.tcx.hir_crate_items(()).body_owners.to_vec()).for_each(|def_id| f(def_id));

par_iter(&self.tcx.hir_crate_items(()).body_owners[..]).for_each(|&def_id| f(def_id));

?

cjgillot · 2022-07-13T15:36:08Z

compiler/rustc_middle/src/hir/map/mod.rs

- self.submodules.push(n.owner);
- intravisit::walk_mod(self, m, n);
- }
+ fn visit_mod(&mut self, m: &'hir Mod<'hir>, _s: Span, n: HirId) {


This will insert a module as its own submodule. We should probably handle all ItemKind::Mod in visit_item.

I think the if-else in visit_item would prevent that from happening? visit_mod gets called only by walk_item.

if !self.crate_collector && let ItemKind::Mod(..) = item.kind { // If this declares another module, do not recurse inside it. self.submodules.push(item.def_id); } else { intravisit::walk_item(self, item) }

visit_mod is the entry point called by hir_module_items.

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

cjgillot · 2022-07-14T09:15:35Z

@bors r+

bors · 2022-07-14T09:15:37Z

📌 Commit 2d265b6 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2022-07-15T01:03:58Z

⌛ Testing commit 2d265b6 with merge 30243dd...

bors · 2022-07-15T03:44:47Z

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

rust-timer · 2022-07-15T05:01:59Z

Finished benchmarking commit (30243dd): comparison url.

Instruction count

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

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.6%	0.6%	1
Improvements 🎉 (primary)	-0.4%	-0.7%	30
Improvements 🎉 (secondary)	-0.5%	-0.8%	18
All 😿🎉 (primary)	-0.4%	-0.7%	30

Max RSS (memory usage)

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.7%	-2.7%	1
Improvements 🎉 (secondary)	-3.2%	-4.3%	4
All 😿🎉 (primary)	-2.7%	-2.7%	1

Cycles

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.0%	2.0%	1
Improvements 🎉 (primary)	-1.2%	-1.2%	1
Improvements 🎉 (secondary)	-2.9%	-2.9%	1
All 😿🎉 (primary)	-1.2%	-1.2%	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 ↩ ↩² ↩³

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

rust-highfive assigned nagisa Jun 17, 2022

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

rust-highfive assigned cjgillot and unassigned nagisa Jun 17, 2022

This comment has been minimized.

Sign in to view

cjgillot reviewed Jun 17, 2022

View reviewed changes

kckeiks force-pushed the gather-body-owners-in-hir-item-queries branch 2 times, most recently from 0366bfc to 327a469 Compare June 17, 2022 21:58

This comment has been minimized.

Sign in to view

kckeiks force-pushed the gather-body-owners-in-hir-item-queries branch 2 times, most recently from 8343258 to b535db3 Compare June 17, 2022 23:02

This comment has been minimized.

Sign in to view

kckeiks force-pushed the gather-body-owners-in-hir-item-queries branch from b535db3 to 1160496 Compare June 17, 2022 23:29

This comment has been minimized.

Sign in to view

cjgillot reviewed Jun 18, 2022

View reviewed changes

cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 21, 2022

kckeiks force-pushed the gather-body-owners-in-hir-item-queries branch 3 times, most recently from bbc27a9 to 17734a6 Compare June 24, 2022 16:39

kckeiks marked this pull request as ready for review June 24, 2022 17:19

cjgillot reviewed Jul 5, 2022

View reviewed changes

kckeiks added 3 commits July 6, 2022 19:17

gather body owners

b53dd71

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

record LocalDefId of closure

01a957e

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

update test

e11862f

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

kckeiks force-pushed the gather-body-owners-in-hir-item-queries branch from 4d38f6e to 75ffbb0 Compare July 6, 2022 23:17

This comment has been minimized.

Sign in to view

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 Jul 13, 2022

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

This comment has been minimized.

Sign in to view

cjgillot reviewed Jul 13, 2022

View reviewed changes

kckeiks added 3 commits July 13, 2022 12:22

use gathered body_owners in par_body_owners

c6e7c05

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

inline associated_body

b599cf4

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

merge visitors in queries

275497c

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

kckeiks force-pushed the gather-body-owners-in-hir-item-queries branch from 75ffbb0 to ad76362 Compare July 13, 2022 17:26

collect module item-likes in visit_items

2d265b6

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

kckeiks force-pushed the gather-body-owners-in-hir-item-queries branch from ad76362 to 2d265b6 Compare July 13, 2022 17:55

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 Jul 14, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 15, 2022

bors merged commit 30243dd into rust-lang:master Jul 15, 2022

rustbot added this to the 1.64.0 milestone Jul 15, 2022

kckeiks mentioned this pull request Jul 16, 2022

change maybe_body_owned_by to take local def id #99311

Merged

JohnTitor mentioned this pull request Jul 16, 2022

ICE with async functions inside extern block: No HirId for DefId #95829

Closed

	if self.tcx.hir().is_body_owner(item.def_id) {
	if associated_body(Node::Item(item)).is_some() {

	self.body_owners.push(ex.hir_id.owner);
	self.body_owners.push(self.tcx.hir().local_def_id(ex.hir_id));

	par_iter(self.tcx.hir_crate_items(()).body_owners.to_vec()).for_each(\|def_id\| f(def_id));
	par_iter(&self.tcx.hir_crate_items(()).body_owners[..]).for_each(\|&def_id\| f(def_id));

gather body owners #98203

gather body owners #98203

Conversation

kckeiks commented Jun 17, 2022 • edited Loading

rust-highfive commented Jun 17, 2022

kckeiks commented Jun 17, 2022

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

bors commented Jul 13, 2022

This comment has been minimized.

cjgillot commented Jul 13, 2022

bors commented Jul 13, 2022

bors commented Jul 13, 2022

This comment has been minimized.

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

cjgillot commented Jul 14, 2022

bors commented Jul 14, 2022

bors commented Jul 15, 2022

bors commented Jul 15, 2022

rust-timer commented Jul 15, 2022

Footnotes

kckeiks commented Jun 17, 2022 •

edited

Loading