delegation: fix cycles during delayed lowering

[aerooneqq]

View all comments

This PR forces lowering of delayed owners after hir_crate_items, as some diagnostics use hir_crate_items which results in query cycle which is then hangs calling def_path_str again and again. Fixes #154169. Part of #118212.

r? @petrochenkov

[petrochenkov]

What specific uses of def_path_str cause the infinite recursion?
Perhaps using something like with_reduced_queries will allow to break the recursion locally, and avoid global changes that this PR does.

[petrochenkov]

Relevant - #154387, #154389.

[jyn514]

This should really have a reviewer with a lot of query system experience.

r? @Zoxc

[rustbot]

Failed to set assignee to zoxc: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

[aerooneqq]

So the reason of this cycle was that this function wants to iterate over all free items, however they were not available during delayed lowering as it was executed before hir_crate_items(()), now delayed lowering is executed after hir_crate_items(()), and we use available-without-lowering information to fill hir_crate_items with information about delayed owners.

rust/compiler/rustc_middle/src/ty/print/pretty.rs

Line 3393 in e3691a5

for id in tcx.hir_free_items() {

Next, there is one more problem in this function, which is maybe not that actual now, but it will be during supporting inherent impls, as during function resolution we will do coherence check, and in cases like:

struct X<T> { t: T }

// No generics provided
impl X {
  pub fn foo() {}
}

reuse X::foo;

we would still come to for_each_def function, but this time this line will be a problem, as we would call delayed lowering during resolution of delegation, so I stored identifier which is needed by this loop in delayed owners, as it once again information that can be accessed without lowering.

rust/compiler/rustc_middle/src/ty/print/pretty.rs

Line 3398 in e3691a5

let item = tcx.hir_item(id);

@rustbot ready

[petrochenkov]

This use of hir_crate looks like it may be a source of perf regressions in incremental mode, I'll run benchmarks.

[petrochenkov]

So calls like tcx.hir_module_items(specific_module) now need to walk the whole crate instead of just specific_module.
Ideally, delayed_ids should perhaps be per-module to fit best into the current setup.
(Not sure if this needs to be addressed right now, perhaps we'll see after running benchmarks.)

[aerooneqq]

@rustbot ready
For perf.

[petrochenkov]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 40807f4 (40807f47e0044fa8a906be6caefbfcebd878e621, parent: e3691a5bd58e92999bcbffedf640ecd61e02c29e)

[rust-timer]

Finished benchmarking commit (40807f4): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	6.9%	[0.3%, 43.4%]	63
Regressions ❌ (secondary)	0.6%	[0.0%, 1.1%]	16
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	2
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	6.7%	[-0.2%, 43.4%]	65

Max RSS (memory usage)

Results (primary 6.6%, secondary 2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	6.6%	[1.2%, 24.4%]	64
Regressions ❌ (secondary)	5.3%	[1.4%, 19.8%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.2%	[-6.1%, -3.5%]	5
All ❌✅ (primary)	6.6%	[1.2%, 24.4%]	64

Cycles

Results (primary 13.2%, secondary 3.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	13.2%	[2.0%, 61.2%]	51
Regressions ❌ (secondary)	3.1%	[2.7%, 3.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	13.2%	[2.0%, 61.2%]	51

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 483.294s -> 485.596s (0.48%)
Artifact size: 395.05 MiB -> 395.15 MiB (0.03%)

[jyn514]

regressions are mostly in evaluate_obligation and typeck. probably they call either ItemCollector::new or hir_opt_ident a lot?

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[aerooneqq]

@rustbot ready
This should fix incremental compilation issues, however it may bring other perf regressions as now we call it every time we want to get Ident. Let's try run perf, If it will be OK then we can leave this thus adjusting all logic connected to getting identifier.

Honestly speaking I am not sure that this is a correct way of solving cycle problems during delegation function resolution, after some experiments with inherent impls I found several other scenarios where cycles are possible, for example one is connected to const evaluation which eventually comes to lints_that_dont_need_to_run which creates query cycle.
Maybe we need infrastructural solution, for example fill synthetic owners for delegations before delayed lowering, then execute delayed lowering in a special query system mode, in this mode we will use synthetic owners in all queries connected to delegations, and after this mode we will replace them with lowered delegations and invalidate all queries that used synthetic delegations. But I still need to experiment more with that.

[jyn514]

It's not clear to me why delegation needs a special "unprocessed HIR ID" at all. Can't you use type-dependent name resolution like the other things that can't be resolved without typeck info?