Mark span parent in def_collector. #127241

cjgillot · 2024-07-02T14:54:29Z

The current device of marking spans with a parent def-id during lowering has been frustrating me for quite some time, as it's very easy to forget marking some spans.

This PR moves such marking to the def_collector, which is responsible for creating def-ids on partially expanded AST. This is much more robust as long as visitors are exhaustive.

r? ghost

cjgillot · 2024-07-02T14:54:39Z

@bors try @rust-timer queue

bors · 2024-07-02T14:55:50Z

⌛ Trying commit 95dc7f7 with merge ec84e1c...

Mark span parent in def_collector. The current device of marking spans with a parent def-id during lowering has been frustrating me for quite some time, as it's very easy to forget marking some spans. This PR moves such marking to the def_collector, which is responsible for creating def-ids on partially expanded AST. This is much more robust as long as visitors are exhaustive. r? ghost

cjgillot · 2024-07-02T15:56:58Z

@bors try @rust-timer queue

bors · 2024-07-02T15:58:09Z

⌛ Trying commit 585fe45 with merge 04122fb...

Mark span parent in def_collector. The current device of marking spans with a parent def-id during lowering has been frustrating me for quite some time, as it's very easy to forget marking some spans. This PR moves such marking to the def_collector, which is responsible for creating def-ids on partially expanded AST. This is much more robust as long as visitors are exhaustive. r? ghost

bors · 2024-07-02T17:46:47Z

☀️ Try build successful - checks-actions
Build commit: 04122fb (04122fbfda6e59dcf9c54b89353e6d1281c9ebc8)

rust-timer · 2024-07-02T20:15:38Z

Finished benchmarking commit (04122fb): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.2%, 2.5%]	163
Regressions ❌ (secondary)	1.3%	[0.3%, 6.2%]	77
Improvements ✅ (primary)	-2.6%	[-4.8%, -0.3%]	12
Improvements ✅ (secondary)	-0.5%	[-0.7%, -0.4%]	2
All ❌✅ (primary)	0.7%	[-4.8%, 2.5%]	175

Max RSS (memory usage)

Results (primary 0.3%, secondary -0.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.1%	[1.4%, 2.6%]	6
Regressions ❌ (secondary)	4.2%	[4.0%, 4.6%]	3
Improvements ✅ (primary)	-2.4%	[-4.4%, -1.0%]	4
Improvements ✅ (secondary)	-3.6%	[-4.3%, -3.2%]	5
All ❌✅ (primary)	0.3%	[-4.4%, 2.6%]	10

Cycles

Results (primary 0.6%, secondary 3.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.0%	[0.7%, 3.4%]	28
Regressions ❌ (secondary)	3.2%	[2.1%, 4.3%]	11
Improvements ✅ (primary)	-4.9%	[-6.0%, -1.4%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[-6.0%, 3.4%]	35

Binary size

Results (primary -0.2%, secondary -0.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.2%]	7
Regressions ❌ (secondary)	0.1%	[0.1%, 0.2%]	6
Improvements ✅ (primary)	-0.2%	[-1.7%, -0.0%]	41
Improvements ✅ (secondary)	-0.3%	[-0.9%, -0.0%]	13
All ❌✅ (primary)	-0.2%	[-1.7%, 0.2%]	48

Bootstrap: 695.108s -> 697.834s (0.39%)
Artifact size: 327.55 MiB -> 327.54 MiB (-0.00%)

cjgillot · 2024-07-02T22:09:35Z

compiler/rustc_ast_lowering/src/path.rs

-            ),
+            span: p.segments[..proj_start]
+                .last()
+                .map_or(path_span_lo, |segment| path_span_lo.to(segment.span())),


It would appear that half the regression in incr-unchecked comes from Span::to in this line. The other half comes from using a MutVisitor in DefCollector, which had to be expected.

The other half comes from using a MutVisitor in DefCollector

I tried to address this in #127371, not sure if it will help.

cjgillot · 2024-07-03T06:42:35Z

@petrochenkov do I need to add something to this PR to handle metavar spans, or is this pass enough?

cjgillot · 2024-07-03T07:21:43Z

@bors try @rust-timer queue

petrochenkov · 2024-07-03T10:05:05Z

compiler/rustc_ast/src/mut_visit.rs

-        GenericBound::Trait(ty, _modifier) => vis.visit_poly_trait_ref(ty),
+        GenericBound::Trait(ty, modifier) => {
+            vis.visit_poly_trait_ref(ty);
+            visit_trait_bound_modifier(modifier, vis);


Could you visit things in the standard order (e.g. nodes in source code order, spans last, etc)?

petrochenkov · 2024-07-03T10:08:31Z

compiler/rustc_error_messages/src/lib.rs

@@ -513,6 +513,17 @@ impl MultiSpan {
    pub fn clone_ignoring_labels(&self) -> Self {
        Self { primary_spans: self.primary_spans.clone(), ..MultiSpan::new() }
    }
+
+    pub fn clone_ignoring_parents(&self) -> Self {


This commit needs some explanation.

petrochenkov · 2024-07-03T10:14:30Z

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -183,7 +183,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {

    pub(crate) fn build_reduced_graph(
        &mut self,
-        fragment: &AstFragment,
+        fragment: &mut AstFragment,


I admit I dislike the mutable def collector a lot.

Maybe make a separate small mut visitor for span marking instead of turning def collector mutable?
Def collector has only 12 calls to with_parent, shouldn't be hard to reproduce.

petrochenkov · 2024-07-03T10:15:37Z

compiler/rustc_ast_lowering/src/expr.rs

+                ExprKind::Index(el, er, brackets_span) => hir::ExprKind::Index(
+                    self.lower_expr(el),
+                    self.lower_expr(er),
+                    self.lower_span(*brackets_span),


Not sure why new lower_spans are added here if they are removed in a next commit.

petrochenkov · 2024-07-03T10:17:10Z

compiler/rustc_resolve/src/def_collector.rs

@@ -128,6 +128,10 @@ impl<'a, 'b, 'tcx> DefCollector<'a, 'b, 'tcx> {
 }

 impl<'a, 'b, 'tcx> mut_visit::MutVisitor for DefCollector<'a, 'b, 'tcx> {
+    fn visit_span(&mut self, span: &mut Span) {
+        *span = span.with_parent(Some(self.parent_def));


As I understand all the old spans here should have no parent, maybe add an assert?
(Can be turned into a debug assert before merging.)

petrochenkov · 2024-07-03T10:30:42Z

As I understand this change doesn't obsolete the invocation span parent setting logic in fn collect_invocations, because def collector cannot visit macro (or attribute) calls because they are already replaced with placeholders.

petrochenkov · 2024-07-03T10:36:54Z

Def collector also doesn't enable MutVisitor::VISIT_TOKENS.
This is probably good because visiting lazy token streams in AST nodes is probably not what we want.
But it also means that the stuff in macro definition bodies is not parented (it appears as values in the metavar_spans table).

It would generally make sense to parent token spans in macro definitions, but that's sort of an offtopic for this PR.

It would also make sense to parent token spans in macro arguments as well (which we don't do due to #127241 (comment), in addition to VISIT_TOKENS),
These spans appear as keys in the metavar_spans table.

petrochenkov · 2024-07-03T10:49:07Z

@petrochenkov do I need to add something to this PR to handle metavar spans, or is this pass enough?

Due to the comments above, I think both key and value spans in the metavar_spans table are still unparented.
The values will stay unparented because macro definitions are not visited by any span parenting logic.
The key spans can get parents later when they are integrated into AST (but not in the table, that's why #119412 sets key parents to None before table lookups).

So probably nothing to do right now.
Longer term I think we need to visit tokens in both macro definitions and in macro calls.
Moreover parents set in macro calls seem more resistant to code changes than those set later in AST building, so we should keep them and not override later.
(I'm actually interested why parent updates like Some(parent) -> Some(other_parent) happen now, maybe we need to get rid of them somehow or keep the old parent.)

petrochenkov · 2024-07-03T10:55:01Z

Once this work is generally ready it's probably better to split it into multiple parts (in any order)

Misc optimizations like f7a53c3
Misc changes to specific spans like kw_span or lower_expr_await
The main infrastructural change (moving parenting from AST lowering to collect_invocations)

rust-timer · 2024-07-03T12:20:51Z

Finished benchmarking commit (83cb4f9): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.2%, 2.1%]	97
Regressions ❌ (secondary)	0.9%	[0.3%, 1.9%]	42
Improvements ✅ (primary)	-1.9%	[-5.1%, -0.4%]	19
Improvements ✅ (secondary)	-0.6%	[-1.1%, -0.2%]	2
All ❌✅ (primary)	0.2%	[-5.1%, 2.1%]	116

Max RSS (memory usage)

Results (primary -1.8%, secondary -3.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.8%	[-2.3%, -0.9%]	4
Improvements ✅ (secondary)	-3.3%	[-4.2%, -2.4%]	2
All ❌✅ (primary)	-1.8%	[-2.3%, -0.9%]	4

Cycles

Results (primary 0.1%, secondary -1.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.1%	[1.2%, 3.5%]	17
Regressions ❌ (secondary)	2.4%	[2.1%, 2.7%]	2
Improvements ✅ (primary)	-4.7%	[-6.1%, -1.5%]	7
Improvements ✅ (secondary)	-3.5%	[-4.0%, -3.2%]	3
All ❌✅ (primary)	0.1%	[-6.1%, 3.5%]	24

Binary size

Results (primary -0.1%, secondary -0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	4
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	25
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.0%]	8
All ❌✅ (primary)	-0.1%	[-0.1%, -0.0%]	25

Bootstrap: 697.439s -> 698.541s (0.16%)
Artifact size: 327.71 MiB -> 327.66 MiB (-0.02%)

petrochenkov · 2024-07-19T09:42:49Z

I wonder if it would be possible to assign span parents even earlier, during parsing.

That way we'd automatically "visit" everything including tokens in macros, and also avoided mutating AST (if immutable arena-allocated AST is a goal).

We'd keep a current parent id in the parser state and update it when recusing into things like items.
The parent id doesn't even need to be a DefId or match the DefId hierarchy exactly, it just needs to be close enough to its inner spans to resist small changes and we can map these ids to DefIds later (e.g. in def collector).

Upd: if we do this and remove nonterminals (AST pieces inside tokens), then we'd be able to remove MutVisitor::visit_span as well (it will only be needed for token streams, but not AST).

cjgillot · 2024-08-24T16:39:01Z

I wonder if it would be possible to assign span parents even earlier, during parsing.

I don't see how: that would need to assign a parent even before the parent def-id is created.

Do not call source_span when not tracking dependencies. Split from rust-lang#127241

…enkov Do not call source_span when not tracking dependencies. Split from rust-lang#127241

Do not call source_span when not tracking dependencies. Split from rust-lang/rust#127241