Skip to content

Generate metadata by iterating on DefId instead of traversing the HIR tree 1/N #80919

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jan 24, 2021
Merged

Generate metadata by iterating on DefId instead of traversing the HIR tree 1/N #80919

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
064a351
Infallible version of def_span.
cjgillot Dec 23, 2020
0b6c9e9
Encode DefKind directly.
cjgillot Nov 15, 2020
c58a6fa
Iterate DefId to encode spans.
cjgillot Nov 28, 2020
a185cdb
Iterate to encode def_kind.
cjgillot Jan 16, 2021
f1bf6d0
Iterate DefId to encode visibility.
cjgillot Nov 28, 2020
293e885
Iterate DefId to encode attributes.
cjgillot Nov 28, 2020
7a5620c
Iterate DefId to encode expn_that_defined.
cjgillot Nov 28, 2020
f1a7e1d
Iterate on stability.
cjgillot Dec 6, 2020
4468473
Iterate on const_stability.
cjgillot Dec 6, 2020
4e60ecf
Iterate on deprecation.
cjgillot Dec 6, 2020
4eb0bbd
Filter stability.
cjgillot Jan 16, 2021
8936093
Fix proc macro crate encoding.
cjgillot Jan 16, 2021
97ee7c7
Allow to query the HIR crate node.
cjgillot Jan 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Iterate DefId to encode visibility.
  • Loading branch information
@cjgillot
cjgillot committed Jan 23, 2021
commit f1bf6d0e48fdf5be3c3a1264b5d0ef8f1f272e5f
48 changes: 38 additions & 10 deletions compiler/rustc_metadata/src/rmeta/encoder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,41 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
}
}

fn should_encode_visibility(def_kind: DefKind) -> bool {
match def_kind {
DefKind::Mod
| DefKind::Struct
| DefKind::Union
| DefKind::Enum
| DefKind::Variant
| DefKind::Trait
| DefKind::TyAlias
| DefKind::ForeignTy
| DefKind::TraitAlias
| DefKind::AssocTy
| DefKind::Fn
| DefKind::Const
| DefKind::Static
| DefKind::Ctor(..)
| DefKind::AssocFn
| DefKind::AssocConst
| DefKind::Macro(..)
| DefKind::Use
| DefKind::ForeignMod
| DefKind::OpaqueTy
| DefKind::Impl
| DefKind::Field => true,
DefKind::TyParam
| DefKind::ConstParam
| DefKind::LifetimeParam
| DefKind::AnonConst
| DefKind::GlobalAsm
| DefKind::Closure
| DefKind::Generator
| DefKind::ExternCrate => false,
}
}

impl EncodeContext<'a, 'tcx> {
fn encode_def_ids(&mut self) {
if self.is_proc_macro {
Expand All @@ -734,6 +769,9 @@ impl EncodeContext<'a, 'tcx> {
def_kind => def_kind,
});
record!(self.tables.span[def_id] <- tcx.def_span(def_id));
if should_encode_visibility(def_kind) {
record!(self.tables.visibility[def_id] <- self.tcx.visibility(def_id));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possibility would be to add an intermediate visibilities query which is just a map from local def ids to visibilities and have the visibility query fetch its information from that instead of from the HIR directly. Then we could iterate that visibilities query result here instead of invoking visibility on a certain set of visibilities. That would duplicate all visibility entries though, so it's not ideal either.

For the reduce of complexity, I'm fine merging the PR as it is and taking the up to 1% perf regression

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. Isn't that what the visibility query does already? It fetches the visibilities computed by resolving, and corrects the missing elements. I can investigate doing this eagerly during HIR indexing.

If you are ok with the large if let, I will remove the visibility -> try_visibility rename before merging.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... I guess then you can remove the visibiliy query check entirely and just iterate the tcx.visibilites (instead of all def ids) to find out which visibility queries to run? Not sure if that is too hacky, maybe we should uplift tcx.visibilities to a query that just returns the field... cc @petrochenkov

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tcx.visibilities table is incomplete, it's not enough to iterate it to encode all visibilities.
In theory is can be completed during AST lowering, but filling the missing entries on the fly in the query was simpler.
If the table is completed during AST lowering, then iterating it will be enough to encode all visibilities.

}
}
}

Expand Down Expand Up @@ -761,7 +799,6 @@ impl EncodeContext<'a, 'tcx> {
};

record!(self.tables.kind[def_id] <- EntryKind::Variant(self.lazy(data)));
record!(self.tables.visibility[def_id] <- self.tcx.visibility(def_id));
record!(self.tables.attributes[def_id] <- &self.tcx.get_attrs(def_id)[..]);
record!(self.tables.expn_that_defined[def_id] <- self.tcx.expansion_that_defined(def_id));
record!(self.tables.children[def_id] <- variant.fields.iter().map(|f| {
Expand Down Expand Up @@ -800,7 +837,6 @@ impl EncodeContext<'a, 'tcx> {
};

record!(self.tables.kind[def_id] <- EntryKind::Variant(self.lazy(data)));
record!(self.tables.visibility[def_id] <- self.tcx.visibility(def_id));
self.encode_stability(def_id);
self.encode_deprecation(def_id);
self.encode_item_type(def_id);
Expand Down Expand Up @@ -851,7 +887,6 @@ impl EncodeContext<'a, 'tcx> {
};

record!(self.tables.kind[def_id] <- EntryKind::Mod(self.lazy(data)));
record!(self.tables.visibility[def_id] <- self.tcx.visibility(def_id));
record!(self.tables.attributes[def_id] <- attrs);
if self.is_proc_macro {
record!(self.tables.children[def_id] <- &[]);
Expand Down Expand Up @@ -881,7 +916,6 @@ impl EncodeContext<'a, 'tcx> {
let variant_data = tcx.hir().expect_variant_data(variant_id);

record!(self.tables.kind[def_id] <- EntryKind::Field);
record!(self.tables.visibility[def_id] <- self.tcx.visibility(def_id));
record!(self.tables.attributes[def_id] <- variant_data.fields()[field_index].attrs);
record!(self.tables.expn_that_defined[def_id] <- self.tcx.expansion_that_defined(def_id));
self.encode_ident_span(def_id, field.ident);
Expand All @@ -906,7 +940,6 @@ impl EncodeContext<'a, 'tcx> {
};

record!(self.tables.kind[def_id] <- EntryKind::Struct(self.lazy(data), adt_def.repr));
record!(self.tables.visibility[def_id] <- self.tcx.visibility(def_id));
record!(self.tables.expn_that_defined[def_id] <- self.tcx.expansion_that_defined(def_id));
self.encode_stability(def_id);
self.encode_deprecation(def_id);
Expand Down Expand Up @@ -1010,7 +1043,6 @@ impl EncodeContext<'a, 'tcx> {
record!(self.tables.kind[def_id] <- EntryKind::AssocType(container));
}
}
record!(self.tables.visibility[def_id] <- self.tcx.visibility(def_id));
record!(self.tables.attributes[def_id] <- ast_item.attrs);
self.encode_ident_span(def_id, ast_item.ident);
self.encode_stability(def_id);
Expand Down Expand Up @@ -1113,7 +1145,6 @@ impl EncodeContext<'a, 'tcx> {
record!(self.tables.kind[def_id] <- EntryKind::AssocType(container));
}
}
record!(self.tables.visibility[def_id] <- self.tcx.visibility(def_id));
record!(self.tables.attributes[def_id] <- ast_item.attrs);
self.encode_ident_span(def_id, impl_item.ident);
self.encode_stability(def_id);
Expand Down Expand Up @@ -1358,7 +1389,6 @@ impl EncodeContext<'a, 'tcx> {
}
};
record!(self.tables.kind[def_id] <- entry_kind);
record!(self.tables.visibility[def_id] <- self.tcx.visibility(def_id));
record!(self.tables.attributes[def_id] <- item.attrs);
record!(self.tables.expn_that_defined[def_id] <- self.tcx.expansion_that_defined(def_id));
// FIXME(eddyb) there should be a nicer way to do this.
Expand Down Expand Up @@ -1477,7 +1507,6 @@ impl EncodeContext<'a, 'tcx> {
fn encode_info_for_macro_def(&mut self, macro_def: &hir::MacroDef<'_>) {
let def_id = self.tcx.hir().local_def_id(macro_def.hir_id).to_def_id();
record!(self.tables.kind[def_id] <- EntryKind::MacroDef(self.lazy(macro_def.ast.clone())));
record!(self.tables.visibility[def_id] <- self.tcx.visibility(def_id));
record!(self.tables.attributes[def_id] <- macro_def.attrs);
self.encode_ident_span(def_id, macro_def.ident);
self.encode_stability(def_id);
Expand Down Expand Up @@ -1808,7 +1837,6 @@ impl EncodeContext<'a, 'tcx> {
record!(self.tables.kind[def_id] <- EntryKind::ForeignType);
}
}
record!(self.tables.visibility[def_id] <- self.tcx.visibility(def_id));
record!(self.tables.attributes[def_id] <- nitem.attrs);
self.encode_ident_span(def_id, nitem.ident);
self.encode_stability(def_id);
Expand Down