Store HIR ForeignItem in a side table #79318

cjgillot · 2020-11-22T21:15:15Z

In a similar fashion to Item, ImplItem and TraitItem.

rust-highfive · 2020-11-22T21:15:18Z

(rust_highfive has picked a reviewer for you, use r? to override)

petrochenkov · 2020-11-22T21:40:50Z

cc @lcnr https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/visit.20.60ForeignItem.60s.20in.20.60ItemLikeVisitor.60

lcnr · 2020-11-22T21:54:01Z

didn't look through it completely yet, but from what I can see this is really awesome and what I should have done a few months ago while I was looking into this 👍

good job

cjgillot · 2020-11-23T22:36:20Z

Is this enough to close #37713?

bors · 2020-11-25T12:20:51Z

☔ The latest upstream changes (presumably #79388) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

varkor · 2020-11-26T13:20:38Z

It may take me a little while to get to this, as it's quite busy for me at the moment, so if someone else is happy to review, do go ahead. Otherwise I'll get to it as soon as I can!

lcnr · 2020-11-26T15:01:36Z

@bors try @rust-timer queue

rust-timer · 2020-11-26T15:01:38Z

Awaiting bors try build completion

bors · 2020-11-26T15:01:48Z

⌛ Trying commit 624f07fed26127b737495cfe15719ecd9d273a34 with merge 30a62b3f272e155d4aba84177ea7147d78f85afc...

bors · 2020-11-26T15:44:33Z

☀️ Try build successful - checks-actions
Build commit: 30a62b3f272e155d4aba84177ea7147d78f85afc (30a62b3f272e155d4aba84177ea7147d78f85afc)

rust-timer · 2020-11-26T15:44:35Z

Queued 30a62b3f272e155d4aba84177ea7147d78f85afc with parent aefcf1f, future comparison URL.

lcnr

lgtm

some small nits

lcnr · 2020-11-26T15:07:05Z

compiler/rustc_hir/src/arena.rs

@@ -29,6 +29,7 @@ macro_rules! arena_types {
            [] field_pat: rustc_hir::FieldPat<$tcx>,
            [] fn_decl: rustc_hir::FnDecl<$tcx>,
            [] foreign_item: rustc_hir::ForeignItem<$tcx>,
+            [] foreign_item_ref: rustc_hir::ForeignItemRef<$tcx>,


would it make sense to use [few] here?

Don't know how big of an impact this has but foreign items are probably quite rare.

compiler/rustc_hir/src/hir.rs

lcnr · 2020-11-26T15:13:47Z

compiler/rustc_hir/src/intravisit.rs

@@ -277,6 +289,14 @@ pub trait Visitor<'v>: Sized {
        walk_list!(self, visit_impl_item, opt_item);
    }

+    /// Like `visit_nested_item()`, but for impl items. See


Suggested change

/// Like `visit_nested_item()`, but for impl items. See

/// Like `visit_nested_item()`, but for foreign items. See

lcnr · 2020-11-26T15:19:43Z

compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -1225,7 +1225,7 @@ impl EncodeContext<'a, 'tcx> {
            hir::ItemKind::Mod(ref m) => {
                return self.encode_info_for_mod(item.hir_id, m, &item.attrs);
            }
-            hir::ItemKind::ForeignMod(_) => EntryKind::ForeignMod,
+            hir::ItemKind::ForeignMod{..} => EntryKind::ForeignMod,


Suggested change

hir::ItemKind::ForeignMod{..} => EntryKind::ForeignMod,

hir::ItemKind::ForeignMod { .. } => EntryKind::ForeignMod,

auto formatting inside of macros

lcnr · 2020-11-26T15:31:32Z

compiler/rustc_passes/src/dead.rs

@@ -447,6 +447,8 @@ impl<'v, 'k, 'tcx> ItemLikeVisitor<'v> for LifeSeeder<'k, 'tcx> {
    fn visit_impl_item(&mut self, _item: &hir::ImplItem<'_>) {
        // ignore: we are handling this in `visit_item` above
    }
+
+    fn visit_foreign_item(&mut self, _item: &'v hir::ForeignItem<'v>) {}


This is currently incorrect, but already was so before this PR.

#[repr(C)] struct Type(u8); extern "C" { #[allow(dead_code)] fn hey(t: Type); }

shouldn't warn for Type because type is used by fn hey.
Should probably open a separate issue for this unless you quickly want to fix this in a followup PR

Investigating in a follow-up PR.

lcnr · 2020-11-26T15:34:45Z

compiler/rustc_passes/src/diagnostic_items.rs

@@ -37,6 +37,8 @@ impl<'v, 'tcx> ItemLikeVisitor<'v> for DiagnosticItemCollector<'tcx> {
    fn visit_impl_item(&mut self, impl_item: &hir::ImplItem<'_>) {
        self.observe_item(&impl_item.attrs, impl_item.hir_id);
    }
+
+    fn visit_foreign_item(&mut self, _foreign_item: &hir::ForeignItem<'_>) {}


Suggested change

fn visit_foreign_item(&mut self, _foreign_item: &hir::ForeignItem<'_>) {}

fn visit_foreign_item(&mut self, foreign_item: &hir::ForeignItem<'_>) {

self.observe_item(foreign_item.attrs, foreign_item.hir_id);

}

we currently do this manually in line 107, but should just do this here.

lcnr · 2020-11-26T15:35:29Z

compiler/rustc_passes/src/entry.rs

@@ -45,6 +45,8 @@ impl<'a, 'tcx> ItemLikeVisitor<'tcx> for EntryContext<'a, 'tcx> {
    fn visit_impl_item(&mut self, _impl_item: &'tcx ImplItem<'tcx>) {
        // Entry fn is never a trait item.
    }
+
+    fn visit_foreign_item(&mut self, _: &'tcx ForeignItem<'tcx>) {}


Suggested change

fn visit_foreign_item(&mut self, _: &'tcx ForeignItem<'tcx>) {}

fn visit_foreign_item(&mut self, _: &'tcx ForeignItem<'tcx>) {

// Entry fn is never a foreign item.

}

comments are fun 🤷

lcnr · 2020-11-26T15:43:17Z

compiler/rustc_passes/src/reachable.rs

@@ -378,6 +378,8 @@ impl<'a, 'tcx> ItemLikeVisitor<'tcx> for CollectPrivateImplItemsVisitor<'a, 'tcx
    fn visit_impl_item(&mut self, _impl_item: &hir::ImplItem<'_>) {
        // processed in visit_item above
    }
+
+    fn visit_foreign_item(&mut self, _foreign_item: &hir::ForeignItem<'_>) {}


Suggested change

fn visit_foreign_item(&mut self, _foreign_item: &hir::ForeignItem<'_>) {}

fn visit_foreign_item(&mut self, _foreign_item: &hir::ForeignItem<'_>) {

// We never export foreign functions as they have no body to export.

}

At least that's how I understand this, not completely sure about that 😆

lcnr · 2020-11-26T15:43:50Z

compiler/rustc_passes/src/stability.rs

@@ -499,7 +499,7 @@ impl<'tcx> Visitor<'tcx> for MissingStabilityAnnotations<'tcx> {
        // optional. They inherit stability from their parents when unannotated.
        if !matches!(
            i.kind,
-            hir::ItemKind::Impl { of_trait: None, .. } | hir::ItemKind::ForeignMod(..)
+            hir::ItemKind::Impl { of_trait: None, .. } | hir::ItemKind::ForeignMod{..}


Suggested change

hir::ItemKind::Impl { of_trait: None, .. } | hir::ItemKind::ForeignMod{..}

hir::ItemKind::Impl { of_trait: None, .. } | hir::ItemKind::ForeignMod { .. }

formatting 🤷

lcnr · 2020-11-26T15:54:59Z

r? @lcnr

rust-timer · 2020-11-26T17:28:49Z

Finished benchmarking try commit (30a62b3f272e155d4aba84177ea7147d78f85afc): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

lcnr · 2020-11-27T13:39:39Z

thanks 👍

@bors r+ rollup=never

bors · 2020-11-27T13:39:40Z

📌 Commit d6b22fa has been approved by lcnr

bors · 2020-11-27T13:45:28Z

⌛ Testing commit d6b22fa with merge c922857...

bors · 2020-11-27T15:58:19Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing c922857 to master...

rylev · 2020-12-03T12:14:16Z

@cjgillot @lcnr this change caused a ~4% regression in the extern stress test. It might be worth thinking about why that is since that perf test is likely to exercise this change quite a bit.

lcnr · 2020-12-03T12:44:49Z

With this PR we now have an additional allocation for each extern item as well as a BTreeMap which stores all of them.

Building the additional tree in hir lowering is probably the cause of the slowdown of the extern stress test. I do not think this regression is something to worry about because that benchmark is pretty much the worst case by defining a lot of extern items without doing anything else with them.

This PR reduces the amount of false edges in the query graph, meaning that changing the definition of an extern item shouldn't invalidate as many queries as before as seen in the changes to our incremental tests. That should result in perf improvements in some cases. Would it make sense add an incremental test changing the definition of an extern item to the extern stress test?

It is also a improvement to the way we deal with extern items, which can be seen in #79318 (comment) where it allowed us to easily detect a bug.

rylev · 2020-12-03T13:04:36Z

changing the definition of an extern item

Can you explain the nature of the change to the definition that you'd want to see? I'm not sure I fully follow.

lcnr · 2020-12-03T13:15:49Z

The relevant changes would change ForeignItemKind without changing ForeignItemRef.

So afaict possible changes would be

adding an attribute
changing the generics or function signature of an extern function
changing the mutability of an extern static
going from an extern static to an extern function with the same name and visibility.

cc @cjgillot we store the Span of the whole extern item in the ForeignItemRef, does this mean that incremental gets dirtied whenever we change the size of the extern item definition?

cjgillot · 2020-12-03T13:32:31Z

Yes. AFAICT, the whole HIR is dirtied by spans changes. I had started #72015 to remove dirtying by spans. It is blocked on perf regressions (~few %). For attributes, I have #79519 with a similar idea, blocked on a bug.

Store HIR ForeignItem in a side table In a similar fashion to Item, ImplItem and TraitItem.

Visit ForeignItems when marking dead code Follow-up to rust-lang#79318 r? `@lcnr`

rust-highfive assigned varkor Nov 22, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2020

cjgillot force-pushed the fitem branch from 253296f to 8163ae7 Compare November 25, 2020 22:37

lcnr approved these changes Nov 26, 2020

View reviewed changes

rust-highfive assigned lcnr and unassigned varkor Nov 26, 2020

cjgillot added 6 commits November 26, 2020 21:29

Store ForeignItem in a side table.

419a918

Remove ForeignMod struct.

032f68d

Update test.

044a986

Bless test.

51d1024

Formatting.

60c3bbd

Use visitor for its very purpose.

12a3024

cjgillot force-pushed the fitem branch 2 times, most recently from 30aa3f7 to 9679fa2 Compare November 26, 2020 21:47

Rebase fallout.

d6b22fa

cjgillot force-pushed the fitem branch from 9679fa2 to d6b22fa Compare November 26, 2020 22:56

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 27, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 27, 2020

bors merged commit c922857 into rust-lang:master Nov 27, 2020

rustbot added this to the 1.50.0 milestone Nov 27, 2020

cjgillot deleted the fitem branch November 27, 2020 16:25

LeSeulArtichaut mentioned this pull request Nov 28, 2020

Internal Compiler Error when compiling the ring crate after updating to the 11-27 nightly #79487

Closed

cjgillot mentioned this pull request Dec 5, 2020

Visit ForeignItems when marking dead code #79752

Merged

flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 6, 2020

Auto merge of rust-lang#79318 - cjgillot:fitem, r=lcnr

5d585ec

Store HIR ForeignItem in a side table In a similar fashion to Item, ImplItem and TraitItem.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2020

Auto merge of rust-lang#79752 - cjgillot:dead-alien, r=lcnr

5019791

Visit ForeignItems when marking dead code Follow-up to rust-lang#79318 r? `@lcnr`

cjgillot mentioned this pull request Jan 23, 2021

Separate foreign items in HIR #37713

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store HIR ForeignItem in a side table #79318

Store HIR ForeignItem in a side table #79318

cjgillot commented Nov 22, 2020

rust-highfive commented Nov 22, 2020

petrochenkov commented Nov 22, 2020

lcnr commented Nov 22, 2020

cjgillot commented Nov 23, 2020

bors commented Nov 25, 2020

varkor commented Nov 26, 2020

lcnr commented Nov 26, 2020

rust-timer commented Nov 26, 2020

bors commented Nov 26, 2020

bors commented Nov 26, 2020

rust-timer commented Nov 26, 2020

lcnr left a comment

lcnr Nov 26, 2020

lcnr Nov 26, 2020

lcnr Nov 26, 2020

lcnr Nov 26, 2020

cjgillot Nov 26, 2020

lcnr Nov 26, 2020

lcnr Nov 26, 2020

lcnr Nov 26, 2020

lcnr Nov 26, 2020

lcnr commented Nov 26, 2020

rust-timer commented Nov 26, 2020

lcnr commented Nov 27, 2020

bors commented Nov 27, 2020

bors commented Nov 27, 2020

bors commented Nov 27, 2020

rylev commented Dec 3, 2020

lcnr commented Dec 3, 2020

rylev commented Dec 3, 2020

lcnr commented Dec 3, 2020

cjgillot commented Dec 3, 2020

	/// Like `visit_nested_item()`, but for impl items. See
	/// Like `visit_nested_item()`, but for foreign items. See

	hir::ItemKind::ForeignMod{..} => EntryKind::ForeignMod,
	hir::ItemKind::ForeignMod { .. } => EntryKind::ForeignMod,

-    fn visit_foreign_item(&mut self, _foreign_item: &hir::ForeignItem<'_>) {}
+    fn visit_foreign_item(&mut self, foreign_item: &hir::ForeignItem<'_>) {
+        self.observe_item(foreign_item.attrs, foreign_item.hir_id);
+    }

	hir::ItemKind::Impl { of_trait: None, .. } \| hir::ItemKind::ForeignMod{..}
	hir::ItemKind::Impl { of_trait: None, .. } \| hir::ItemKind::ForeignMod { .. }

Store HIR ForeignItem in a side table #79318

Store HIR ForeignItem in a side table #79318

Conversation

cjgillot commented Nov 22, 2020

rust-highfive commented Nov 22, 2020

petrochenkov commented Nov 22, 2020

lcnr commented Nov 22, 2020

cjgillot commented Nov 23, 2020

bors commented Nov 25, 2020

varkor commented Nov 26, 2020

lcnr commented Nov 26, 2020

rust-timer commented Nov 26, 2020

bors commented Nov 26, 2020

bors commented Nov 26, 2020

rust-timer commented Nov 26, 2020

lcnr 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

lcnr commented Nov 26, 2020

rust-timer commented Nov 26, 2020

lcnr commented Nov 27, 2020

bors commented Nov 27, 2020

bors commented Nov 27, 2020

bors commented Nov 27, 2020

rylev commented Dec 3, 2020

lcnr commented Dec 3, 2020

rylev commented Dec 3, 2020

lcnr commented Dec 3, 2020

cjgillot commented Dec 3, 2020