-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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.
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
Split up Definitions
and ResolverAstLowering
.
#98106
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
5c1245c
to
34e4d72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @cjgillot! Looks good to me.
To clarify, with this change we lose the ability to associated source_spans with items created during lowering, right? Since the PR did not have to update any tests, it looks like that doesn't lead to regression in diagnostics quality, so that should be fine.
r=me with the comment below addressed.
@@ -133,6 +133,10 @@ struct LoweringContext<'a, 'hir: 'a> { | |||
/// NodeIds that are lowered inside the current HIR owner. | |||
node_id_to_local_id: FxHashMap<NodeId, hir::ItemLocalId>, | |||
|
|||
// The next_node_id is reset for each item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to elaborate this comment. It's hard to tell what it means in the current form. Something like:
Note: We create a new LoweringContext for each item, and each time we initialize
next_node_id
withresolver.next_node_id
, which means that each time we start with the same value and the id ranges for new nodes created during lowering will overlap. This is fine because these newly created NodeIds don't escape the item's context and are immediately lowered into HirIds using the item-localnode_id_to_local_id
map.
fn next_node_id(&mut self) -> NodeId { | ||
let next = self | ||
.next_node_id | ||
.as_usize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this version didn't incorporate some recent changes in fn next_node_id
.
pub trait_map: NodeMap<Vec<hir::TraitCandidate>>, | ||
/// A small map keeping true kinds of built-in macros that appear to be fn-like on | ||
/// the surface (`macro` items in libcore), but are actually attributes or derives. | ||
pub builtin_macro_kinds: FxHashMap<LocalDefId, MacroKind>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember these changes from #90451.
The comments from #90451 (comment) are still relevant.
It doesn't seem right that all this data is only used during lowering, but then lives unused (or potentially accidentally used in the future) as long as tcx.
It should be better to move this data to a separate structure.
Then AST lowerer should be able to consume it by value.
pub fn into_outputs(self) -> ResolverOutputs { | ||
pub fn into_outputs( | ||
self, | ||
) -> (Definitions, Box<CrateStoreDyn>, ResolverOutputs, ty::ResolverAstLowering) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tuple got so large that it makes sense to turn it into a structure, named ResolverOutputs
for example 😄
The old ResolverOutputs
could get some more specific name in that case.
I like the updated version. It looks like they address @petrochenkov's comment. Let's do a perf run. But overall, I think this is ready to be merged (unless @petrochenkov has further comments). |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit ae5959f with merge 1270191ac8f3e413cacc6ad5e929641b86774c3b... |
☀️ Try build successful - checks-actions |
Queued 1270191ac8f3e413cacc6ad5e929641b86774c3b with parent 392d272, future comparison URL. |
Finished benchmarking commit (1270191ac8f3e413cacc6ad5e929641b86774c3b): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. 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 Footnotes |
OK, performance looks good. |
📌 Commit ae5959f has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (3a8b014): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
rustc_middle: Rearrange resolver outputs structures slightly Addresses rust-lang#98106 (comment). I also haven't seen the motivation for moving `cstore` from its old place, so I moved it back in this PR. r? `@cjgillot`
rustc_middle: Rearrange resolver outputs structures slightly Addresses rust-lang#98106 (comment). I also haven't seen the motivation for moving `cstore` from its old place, so I moved it back in this PR. r? ``@cjgillot``
rustc_middle: Rearrange resolver outputs structures slightly Addresses rust-lang#98106 (comment). I also haven't seen the motivation for moving `cstore` from its old place, so I moved it back in this PR. r? ```@cjgillot```
Split off #95573
r? @michaelwoerister