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

[rust-bors]

☀️ Try build successful (CI)
Build commit: fee3d49 (fee3d49cfd10675e256ef8db13b8b34b52ae8b8a, parent: 9838411cb723b60dc62b1625751075c4d933b992)

[rust-timer]

Finished benchmarking commit (fee3d49): comparison URL.

Overall result: ❌✅ regressions and improvements - 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
Regressions ❌ (secondary)	0.2%	[0.1%, 0.2%]	3
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.3%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-0.4%, -0.3%]	2

Max RSS (memory usage)

Results (primary 1.3%, secondary 0.9%)

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

	mean	range	count
Regressions ❌ (primary)	1.3%	[0.8%, 2.4%]	21
Regressions ❌ (secondary)	2.3%	[0.6%, 6.6%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-4.9%, -1.9%]	2
All ❌✅ (primary)	1.3%	[0.8%, 2.4%]	21

Cycles

Results (secondary 6.5%)

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

Binary size

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

Bootstrap: 486.61s -> 488.378s (0.36%)
Artifact size: 394.05 MiB -> 394.01 MiB (-0.01%)