Make create_def a query

[cjgillot]

View all comments

Creating a new definition using create_def currently behaves as a red node for incremental compilation: the caller query must be recomputed at the next run. This is suboptimal.

If a query's dependencies have not changed, we'd like the query engine to re-create the definitions and continue replaying the dependency graph and load cached data. This idea falls short with a subtle global state dependency: when creating definitions, we must ensure that DefPathHash are globally unique, and carry a disambiguator global state around. Because of this, a call to create_def may change its result DefPathHash due to global state, and force the caller query to be recomputed. If that happens, the caller query would need to be recomputed, but must not re-create the definitions that the query engine created for it.

This PR attempts to solve this issue by using a create_def_raw query. This query is eval_always, so it can safely read global state. The query engine sees its return value as the DefPathHash, so will detect if it unexpectedly changed. We benefit from the caching queries do.

However, we want multiple successive calls to create_def from the same queries to result in a new definition each time. For instance when creating anonymous definitions that are only identified by their parent and a disambiguator. To allow this, we add 2 extra parameters to create_def_raw that are unique to each call by the caller query: the query's DepNode and a counter of calls from within that query.

Consider this example. Some query A calls create_def twice, we generate calls to:

1. create_def(CRATE_DEF_ID, Use) = create_def_raw(CRATE_DEF_ID, Use, A, 0) This returns ::{use#0}.
2. create_def(CRATE_DEF_ID, Use) = create_def_raw(CRATE_DEF_ID, Use, A, 1) The query has different arguments, so returns a brand new ::{use::1}.
3. create_def(CRATE_DEF_ID, Impl) = create_def_raw(CRATE_DEF_ID, Impl, A, 2) This returns ::{impl#0}.

When replaying, if nothing changed, the query engine will call create_def_raw thrice, and A will be green.

If another query created a definition with (CRATE_DEF_ID, Impl), ::{impl#0} is taken. The 3rd call to create_def needs to use another disambiguator, that call to create_def_raw returns ::{impl#1} and be correctly red. A needs to be re-computed. The definitions for uses have already been created when walking the dep-graph. The re-computation calls use the results in create_def_raw cache, aka ::{use#0}, ::{use#1} and ::{impl#1}, and we do not have duplicate definitions.

Reopening #142711
The original PR was halted pending design work, so this one is opened as draft.

r? @oli-obk

[rustbot]

oli-obk is not on the review rotation at the moment.
They may take a while to respond.

[cjgillot]

@bors try @rust-timer queue

[rust-bors]

💔 Test for ce9351f failed: CI. Failed job:

• try - dist-x86_64-linux-quick (web logs, enhanced plaintext logs)

[cjgillot]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 5a8f10b (5a8f10b842a607a7c2f064494064579c16a3a56b, parent: fb76025f2fe6a1c8ce03fe0931f941c8077a2c34)

[rust-timer]

Finished benchmarking commit (5a8f10b): comparison URL.

Overall result: ❌ regressions - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@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)	0.8%	[0.1%, 1.8%]	144
Regressions ❌ (secondary)	1.1%	[0.2%, 3.8%]	96
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.1%, 1.8%]	144

Max RSS (memory usage)

Results (primary 1.9%, secondary 2.3%)

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

	mean	range	count
Regressions ❌ (primary)	1.9%	[0.7%, 3.6%]	79
Regressions ❌ (secondary)	2.5%	[0.8%, 7.1%]	32
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-2.1%, -1.5%]	2
All ❌✅ (primary)	1.9%	[0.7%, 3.6%]	79

Cycles

Results (primary 2.8%, secondary 3.7%)

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

	mean	range	count
Regressions ❌ (primary)	2.8%	[2.0%, 3.6%]	15
Regressions ❌ (secondary)	4.0%	[2.2%, 6.5%]	20
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	2.8%	[2.0%, 3.6%]	15

Binary size

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

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-0.0%	[-0.1%, -0.0%]	29
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	20
All ❌✅ (primary)	-0.0%	[-0.1%, -0.0%]	29

Bootstrap: 487.131s -> 493.731s (1.35%)
Artifact size: 394.07 MiB -> 394.00 MiB (-0.02%)

[oli-obk]

I mean your PR is the "proper" way to do all that, and I wonder if #115613 (comment) is somewhat better because it's incomplete and will have similar perf to this PR if done properly.

But even if not, maybe we should just take the hit here and analyze why the queries are so expensive compared to a version specifically tuned to DefId creation.

Shorter term tho: maybe just don't call the query from untracked queries like the resolver and instead run the logic directly?

[cjgillot]

@bors try @rust-timer queue

[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.

[oli-obk]

Ideally we would be able to keep the non-global disambiguators, but since we obviously can't pass it into the query... we could just unconditionally bump the disambiguator under the assumption that either we're invoking the query, so we are bumping the disambiguator, or we're replaying it with all-known-values.

So basically instead of passing DefPathData, passing the full DisambiugatedDefPathData to the query

View changes since this review

[oli-obk]

shouldn't we still use them to avoid disambiguator state from being unpredictable with parallel-rustc? It shouldn't be a problem right now, as you just moved from non-global disambiguators back to global ones, but it seems like this could just regress again

[oli-obk]

do we need this anymore at all now?

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[cjgillot]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 3fdcc43 (3fdcc43b300ac73bf7391dd7670e400f2636df1a, parent: 37d85e592f9ae5f20f7d9a9f99785246fa7298da)

[rust-timer]

Finished benchmarking commit (3fdcc43): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.9%, secondary 5.7%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.7%	[5.1%, 6.3%]	2
Improvements ✅ (primary)	-2.9%	[-4.0%, -1.8%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.9%	[-4.0%, -1.8%]	3

Cycles

Results (secondary -2.3%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.3%, 2.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.5%	[-4.4%, -2.3%]	8
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 486.76s -> 488.672s (0.39%)
Artifact size: 393.43 MiB -> 393.49 MiB (0.01%)

[oli-obk]

Yay

r=me with commit history cleaned up