Make create_def a side effect instead of marking the entire query as always red

[oli-obk]

Before this PR:

• query A creates def id D
• query A is marked as depending on the always-red node, meaning it will always get re-run
• in the next run of rustc: query A is not loaded from the incremental cache, but rerun

After this PR:

• query A creates def id D
• query system registers this a side effect (just like we collect diagnostics to re-emit them without running a query)
• in the next run of rustc: query A is loaded from the incremental cache and its side effect is run (thus re-creating def id D without running query A)

r? @cjgillot

TODO:

• need to make feeding queries a side effect, too. At least ones that aren't written to disk.
• need to re-feed the def_span query
• many more tests

[cjgillot]

Another tricky case:

1. ensure() query A -> executes side-effect from the cache;
2. fetch query A's result -> calls the provider for A -> gets to create_def -> ?

We wouldn't want to create 2 definitions where we only ask for one.

[oli-obk]

Huh... why does that happen? Shouldn't we already be getting weird diagnostics in that case?

[cjgillot]

This happens if we don't have the result of A in the on-disk cache, but still need it later.
IIUC, diagnostic deduplication catches it, so there is no observable effect.

[cjgillot]

More precisely: the first call is ensure(), so we don't attempt to compute the result, but still mark the dep-node as green and re-execute side effects. The second call is get(), so we need the result, we don't find it in the on-disk cache, and compute it the only way we know, by calling the provider function.

[cjgillot]

The logic is the fallback case in try_load_from_disk_and_cache_in_memory.

[oli-obk]

Thanks. I really need to dig into ensure and all its behaviours.

[oli-obk]

More precisely: the first call is ensure(), so we don't attempt to compute the result, but still mark the dep-node as green and re-execute side effects. The second call is get(), so we need the result, we don't find it in the on-disk cache, and compute it the only way we know, by calling the provider function.

I did some testing and a code dive, and I don't think that's what's happening.

ensure does not execute side effects. It checks if something is in the cache, and if not, executes that query immediately. The cache lookup itself doesn't perform any actions but set up the dep graph dependency in case of a cache hit.

[cjgillot]

ensure() calls get_query_incr with QueryMode::Ensure, which calls ensure_must_run, which calls try_mark_green, which calls try_mark_previous_green, which calls emit_side_effects.

[oli-obk]

yay, with this hint I was able to produce an example that actually exhibits an issue

index out of bounds: the len is 9 but the index is 9

oh wait... I even have this issue without doing any other changes to rustc. So it's not even ensure related yet

[bors]

☔ The latest upstream changes (presumably #115920) made this pull request unmergeable. Please resolve the merge conflicts.

[cjgillot]

The difficulty is to know when to skip creating the DefId and reuse the one created by side-effect replay.

What about adding a new variant Replay to TaskDepsRef?
That variant would hold the list of definitions created by this query in the previous invocation. The nth call to create_def in the query would return the nth DefId in that list.

[oli-obk]

Thanks! I was thinking about doing

The nth call to create_def in the query would return the nth DefId in that list.

but didn't know how. I'll investigate the TaskDepsRef solution you hinted at.

[bors]

☔ The latest upstream changes (presumably #120486) made this pull request unmergeable. Please resolve the merge conflicts.

[oli-obk]

@cjgillot I implemented replaying, and that fixes the issues I was able to coax out of incremental tests, could you have a look? I'll keep working on it and adding more tests, but I think I could benefit from a review

@bors try @rust-timer queue

[oli-obk]

This affects symbol names in incremental compilation. Not sure if that is a problem

[bors]

☀️ Try build successful - checks-actions
Build commit: ffbd260 (ffbd260e50ffcb33307f4478fc21e4af2f10bcd0)

[rustbot]

This PR modifies run-make tests.

cc @jieyouxu

[oli-obk]

@bors try @rust-timer queue

[bors]

⌛ Trying commit c65f83e with merge b5eeded...

[rust-timer]

Finished benchmarking commit (ffbd260): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	1.1%	[0.1%, 2.7%]	31
Regressions ❌ (secondary)	387.4%	[0.3%, 10004.3%]	42
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-2.2%, -1.3%]	6
All ❌✅ (primary)	1.1%	[0.1%, 2.7%]	31

Max RSS (memory usage)

Results (primary 2.3%, secondary 6.0%)

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.3%	[0.4%, 6.5%]	35
Regressions ❌ (secondary)	10.0%	[1.6%, 25.9%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.9%	[-11.9%, -2.4%]	4
All ❌✅ (primary)	2.3%	[0.4%, 6.5%]	35

Cycles

Results (primary 1.3%, secondary 531.5%)

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)	1.9%	[1.2%, 2.5%]	9
Regressions ❌ (secondary)	677.7%	[2.1%, 8631.6%]	22
Improvements ✅ (primary)	-0.6%	[-1.1%, -0.4%]	3
Improvements ✅ (secondary)	-4.8%	[-5.8%, -4.0%]	6
All ❌✅ (primary)	1.3%	[-1.1%, 2.5%]	12

Binary size

Results (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.0%, 0.1%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 776.42s -> 775.666s (-0.10%)
Artifact size: 365.20 MiB -> 365.19 MiB (-0.00%)

[oli-obk]

Lmao oh right I need to take out c6c92c3

[bors]

☀️ Try build successful - checks-actions
Build commit: b5eeded (b5eededb23553e87d0deb603c7158c4cf0418876)

[oli-obk]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 6d5d29c with merge 8117600...

[bors]

☀️ Try build successful - checks-actions
Build commit: 8117600 (8117600a7b32915295f2627f60ead048ae3b3edf)

[rust-timer]

Finished benchmarking commit (8117600): comparison URL.

Overall result: ❌✅ regressions and improvements - no 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.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.0%)

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.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.6%	[-8.9%, -0.5%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.0%	[-8.9%, 0.7%]	5

Cycles

Results (primary 0.9%, secondary 2.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.9%	[0.5%, 2.6%]	5
Regressions ❌ (secondary)	2.2%	[1.6%, 2.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[0.5%, 2.6%]	5

Binary size

Results (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.0%, 0.1%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 776.211s -> 776.632s (0.05%)
Artifact size: 365.21 MiB -> 365.21 MiB (0.00%)

[oli-obk]

Yay, not even a regression anymore. Now I just need to figure out if there are still query feeding issues and how to resolve them

run A -> feed B
force A but do not replay -> where's the value of B?