Represent diagnostic side effects as dep nodes #122156

Zoxc · 2024-03-07T22:10:16Z

This changes diagnostic to be tracked as a special dep node (SideEffect) instead of having a list of side effects associated with each dep node. SideEffect is always red and when forced, it emits the diagnostic and marks itself green. Each emitted diagnostic generates a new SideEffect with an unique dep node index.

Some implications of this:

Diagnostic may now be emitted more than once as they can be emitted once when the SideEffect gets marked green and again if the task it depends on needs to be re-executed due to another node being red. It relies on deduplicating of diagnostics to avoid that.
Anon tasks which emits diagnostics will no longer incorrectly be merged with other anon tasks.
Reusing a CGU will now emit diagnostics from the task generating it.

rustbot · 2024-03-07T22:10:24Z

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Zoxc · 2024-03-08T01:25:31Z

Performance impact:

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check:unchanged	0.3793s	0.3780s	-0.34%
🟣 hyper:check:unchanged	0.1193s	0.1180s	💚 -1.04%
🟣 regex:check:unchanged	0.2780s	0.2768s	-0.42%
🟣 syn:check:unchanged	0.5607s	0.5593s	-0.25%
🟣 syntex_syntax:check:unchanged	1.3421s	1.3397s	-0.18%
Total	2.6793s	2.6718s	-0.28%
Summary	1.0000s	0.9955s	-0.45%

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check:initial	1.8871s	1.8846s	-0.13%
🟣 hyper:check:initial	0.3099s	0.3094s	-0.16%
🟣 regex:check:initial	1.1095s	1.1079s	-0.15%
🟣 syn:check:initial	1.8133s	1.8098s	-0.19%
🟣 syntex_syntax:check:initial	6.6921s	6.6865s	-0.08%
Total	11.8119s	11.7981s	-0.12%
Summary	1.0000s	0.9986s	-0.14%

bors · 2024-03-13T05:32:22Z

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

Zoxc · 2024-03-15T16:57:34Z

cc @michaelwoerister

michaelwoerister · 2024-03-18T09:09:06Z

Thanks for the ping, @Zoxc! I'll try to take a closer look some time this week.

michaelwoerister · 2024-03-21T13:18:23Z

The changes¹ look good to me. I like how this remove state from the context 🙂

One question: does this increase the number of dep-nodes we create? I can't quite tell if this will unconditionally emit a SideEffect dep-node, even if no diagnostic was emitted.

I've only looked at the last commit, since the rest seems to come from https://github.com/rust-lang/rust/pull/122070) ↩

michaelwoerister · 2024-03-21T13:22:31Z

does this increase the number of dep-nodes we create? I can't quite tell if this will unconditionally emit a SideEffect dep-node, even if no diagnostic was emitted.

Looks like it will be one dep-node per emitted diagnostic, so that's fine.

cjgillot

Thanks @Zoxc! I like a lot how this design simplifies this whole side-effect tracking business.

Could you run perf on the rebased PR?

Could we do without having to thread the SerializedDepNodeIndex everywhere?
An quick idea I have would be some SideEffectIndex: a sequential index returned by store_side_effects, and put into the side-effect DepNode's fingerprint part. We'd still have the information we need, encoded sequentially in the on-disk cache instead of a hash-map.

compiler/rustc_query_system/src/dep_graph/graph.rs

compiler/rustc_query_system/src/query/mod.rs

Zoxc · 2025-03-11T22:10:33Z

This can get a perf run now.

Kobzol · 2025-03-11T23:16:14Z

@bors try @rust-timer queue

Represent diagnostic side effects as dep nodes This changes diagnostic to be tracked as a special dep node (`SideEffect`) instead of having a list of side effects associated with each dep node. `SideEffect` is always red and when forced, it emits the diagnostic and marks itself green. Each emitted diagnostic generates a new `SideEffect` with an unique dep node index. Some implications of this: - Diagnostic may now be emitted more than once as they can be emitted once when the `SideEffect` gets marked green and again if the task it depends on needs to be re-executed due to another node being red. It relies on deduplicating of diagnostics to avoid that. - Anon tasks which emits diagnostics will no longer *incorrectly* be merged with other anon tasks. - Reusing a CGU will now emit diagnostics from the task generating it. Based on rust-lang#122070.

bors · 2025-03-11T23:17:26Z

⌛ Trying commit c6011a7 with merge b616195...

bors · 2025-03-12T01:20:37Z

☀️ Try build successful - checks-actions
Build commit: b616195 (b61619566995f8b3b176227de681f28b7add8834)

rust-timer · 2025-03-12T02:56:13Z

Finished benchmarking commit (b616195): comparison URL.

Overall result: ✅ 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
Improvements ✅ (primary)	-0.7%	[-1.3%, -0.3%]	114
Improvements ✅ (secondary)	-0.9%	[-1.5%, -0.2%]	29
All ❌✅ (primary)	-0.7%	[-1.3%, -0.3%]	114

Max RSS (memory usage)

Results (primary 0.8%, secondary -4.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)	3.6%	[2.7%, 4.9%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.3%	[-4.0%, -2.6%]	2
Improvements ✅ (secondary)	-4.2%	[-6.2%, -2.2%]	2
All ❌✅ (primary)	0.8%	[-4.0%, 4.9%]	5

Cycles

Results (primary -1.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)	1.2%	[0.5%, 1.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.9%	[-3.9%, -2.4%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.3%	[-3.9%, 1.9%]	5

Binary size

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

Bootstrap: 780.09s -> 779.623s (-0.06%)
Artifact size: 365.26 MiB -> 365.23 MiB (-0.01%)

Zoxc · 2025-03-13T18:35:53Z

An quick idea I have would be some SideEffectIndex: a sequential index returned by store_side_effects, and put into the side-effect DepNode's fingerprint part. We'd still have the information we need, encoded sequentially in the on-disk cache instead of a hash-map.

I think something like that could work, less sure if it's an improvement though.

Zoxc · 2025-03-14T17:57:22Z

@rustbot ready

oli-obk · 2025-03-18T13:19:39Z

compiler/rustc_query_system/src/dep_graph/graph.rs

+        if let Some(ref data) = self.data {
+            D::read_deps(|task_deps| match task_deps {
+                TaskDepsRef::EvalAlways | TaskDepsRef::Ignore => return,
+                TaskDepsRef::Forbid | TaskDepsRef::Allow(..) => {


Shouldn't Forbid be unreachable here? If we are decoding a query will we ever emit diagnostics?

The forbid case will just panic in read_index, so it's just a sanity check.

I guess 😃

oli-obk · 2025-03-18T13:44:11Z

@bors r+

bors · 2025-03-18T13:44:14Z

📌 Commit b43a297 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2025-03-18T13:44:15Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

bors · 2025-03-19T15:51:57Z

⌛ Testing commit b43a297 with merge 1aeb99d...

bors · 2025-03-19T19:00:48Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 1aeb99d to master...

github-actions · 2025-03-19T19:02:52Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing a7fc463 (parent) -> 1aeb99d (this PR)

Test differences

No test diffs found

rust-timer · 2025-03-19T20:55:16Z

Finished benchmarking commit (1aeb99d): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Improvements ✅ (primary)	-0.7%	[-1.4%, -0.2%]	117
Improvements ✅ (secondary)	-0.9%	[-1.6%, -0.0%]	37
All ❌✅ (primary)	-0.7%	[-1.4%, -0.2%]	117

Max RSS (memory usage)

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

Cycles

Results (primary -1.6%, secondary -2.9%)

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.6%	[-2.3%, -0.9%]	22
Improvements ✅ (secondary)	-2.9%	[-4.5%, -1.6%]	10
All ❌✅ (primary)	-1.6%	[-2.3%, -0.9%]	22

Binary size

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

Bootstrap: 773.717s -> 774.586s (0.11%)
Artifact size: 365.52 MiB -> 365.51 MiB (-0.00%)

rustbot assigned fmease Mar 7, 2024

rustbot added A-query-system S-waiting-on-review T-compiler labels Mar 7, 2024

cjgillot self-assigned this Mar 14, 2024

fmease removed their assignment Mar 14, 2024

cjgillot reviewed Mar 26, 2024

View reviewed changes

Dylan-DPC added S-waiting-on-author and removed S-waiting-on-review labels May 1, 2024

Zoxc force-pushed the side-effect-dep-node branch from f0413f6 to c6011a7 Compare March 11, 2025 20:44

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf label Mar 11, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf label Mar 12, 2025

Zoxc added 2 commits March 14, 2025 16:01

Represent diagnostic side effects as dep nodes

3ca5220

Rename QuerySideEffects to QuerySideEffect

453b51a

Add comments

9a847b1

Zoxc force-pushed the side-effect-dep-node branch from c6011a7 to 9a847b1 Compare March 14, 2025 17:55

Zoxc marked this pull request as ready for review March 14, 2025 17:56

rustbot added S-waiting-on-review and removed S-waiting-on-author labels Mar 14, 2025

Fix record_diagnostic

b43a297

oli-obk assigned oli-obk and unassigned cjgillot Mar 18, 2025

oli-obk reviewed Mar 18, 2025

View reviewed changes

bors added S-waiting-on-bors and removed S-waiting-on-review labels Mar 18, 2025

bors added the merged-by-bors label Mar 19, 2025

bors merged commit 1aeb99d into rust-lang:master Mar 19, 2025
7 checks passed

rustbot added this to the 1.87.0 milestone Mar 19, 2025

Zoxc deleted the side-effect-dep-node branch March 19, 2025 19:03

bors mentioned this pull request Mar 19, 2025

Avoiding calling queries when collecting active queries #138672

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Represent diagnostic side effects as dep nodes #122156

Represent diagnostic side effects as dep nodes #122156

Zoxc commented Mar 7, 2024 •

edited

Loading

rustbot commented Mar 7, 2024

Zoxc commented Mar 8, 2024

bors commented Mar 13, 2024

Zoxc commented Mar 15, 2024

michaelwoerister commented Mar 18, 2024

michaelwoerister commented Mar 21, 2024

michaelwoerister commented Mar 21, 2024

cjgillot left a comment

Zoxc commented Mar 11, 2025

Kobzol commented Mar 11, 2025

This comment has been minimized.

bors commented Mar 11, 2025

bors commented Mar 12, 2025

This comment has been minimized.

rust-timer commented Mar 12, 2025

Zoxc commented Mar 13, 2025

Zoxc commented Mar 14, 2025

oli-obk Mar 18, 2025

Zoxc Mar 18, 2025

oli-obk Mar 18, 2025

oli-obk commented Mar 18, 2025

bors commented Mar 18, 2025

bors commented Mar 18, 2025

bors commented Mar 19, 2025

bors commented Mar 19, 2025

github-actions bot commented Mar 19, 2025

rust-timer commented Mar 19, 2025

Represent diagnostic side effects as dep nodes #122156

Represent diagnostic side effects as dep nodes #122156

Conversation

Zoxc commented Mar 7, 2024 • edited Loading

rustbot commented Mar 7, 2024

Zoxc commented Mar 8, 2024

bors commented Mar 13, 2024

Zoxc commented Mar 15, 2024

michaelwoerister commented Mar 18, 2024

michaelwoerister commented Mar 21, 2024

Footnotes

michaelwoerister commented Mar 21, 2024

cjgillot left a comment

Choose a reason for hiding this comment

Zoxc commented Mar 11, 2025

Kobzol commented Mar 11, 2025

This comment has been minimized.

bors commented Mar 11, 2025

bors commented Mar 12, 2025

This comment has been minimized.

rust-timer commented Mar 12, 2025

Overall result: ✅ improvements - no action needed

Zoxc commented Mar 13, 2025

Zoxc commented Mar 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Mar 18, 2025

bors commented Mar 18, 2025

bors commented Mar 18, 2025

bors commented Mar 19, 2025

bors commented Mar 19, 2025

github-actions bot commented Mar 19, 2025

Test differences

rust-timer commented Mar 19, 2025

Overall result: ✅ improvements - no action needed

Zoxc commented Mar 7, 2024 •

edited

Loading