(EXPERIMENT) Replace zero-deps nodes with a singleton

[Zalathar]

View all comments

Experimental follow-up to #154122.

This might have subtle and undesired consequences for disk caching, so I'll have to do more thinking and testing before I propose it as a real PR.

[Zalathar]

@bors try @rust-timer queue

[Zalathar]

For example, it might be the case that we only want to perform this simplification when the query+key is not eligible for disk caching, as disk caching might rely on having a distinct prev_index.

[rust-bors]

💔 Test for fbe5cb6 failed: CI. Failed job:

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

[Zalathar]

Yeah, looks like there's more subtlety here that I'll have to dig into.

[Zalathar]

Let's try a narrower version that only applies to queries that don't have cache_on_disk or eval_always.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 2f12db4 (2f12db4591c11e2b854add0bf24a1e527019b0c8, parent: 44e662074f187af2723a26182f06ffff17d833e6)

[rust-timer]

Finished benchmarking commit (2f12db4): comparison URL.

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

Max RSS (memory usage)

Results (primary -1.1%, secondary 2.1%)

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.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-1.1%	[-2.7%, -0.4%]	10
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.1%	[-2.7%, -0.4%]	10

Cycles

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

Binary size

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

Bootstrap: 485.042s -> 483.702s (-0.28%)
Artifact size: 394.99 MiB -> 394.76 MiB (-0.06%)

[Zalathar]

Now that #154122 has landed, let's do a clean perf run to measure just these new changes.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: d41f91c (d41f91c6c7e6300b4181f9cc877ac20234bcdd70, parent: 562dee4820c458d823175268e41601d4c060588a)

[rust-timer]

Finished benchmarking commit (d41f91c): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)	0.2%	[0.2%, 0.2%]	2
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	6
Improvements ✅ (primary)	-0.3%	[-0.6%, -0.2%]	24
Improvements ✅ (secondary)	-0.5%	[-0.8%, -0.1%]	11
All ❌✅ (primary)	-0.3%	[-0.6%, 0.2%]	26

Max RSS (memory usage)

Results (primary -1.8%)

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

Cycles

Results (primary 2.4%, secondary -2.4%)

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

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

Binary size

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

Bootstrap: 485.484s -> 485.487s (0.00%)
Artifact size: 394.88 MiB -> 396.85 MiB (0.50%)

[Zalathar]

Looks like a nice reduction in dep graph size, but clearly there are some subtle side effects that I’ll want to investigate.

[Zalathar]

Let's try again, but this time exclude queries that have recoverable keys.

If a query doesn't cache on disk, and can't recover keys, then in theory there should be no reason for no-deps nodes to exist.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 16b8f32 (16b8f3218eb1cce0ff5f18e1586c5d33878529a4, parent: 562dee4820c458d823175268e41601d4c060588a)

[rust-timer]

Finished benchmarking commit (16b8f32): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	6
Improvements ✅ (primary)	-0.3%	[-0.6%, -0.2%]	21
Improvements ✅ (secondary)	-0.6%	[-0.8%, -0.1%]	11
All ❌✅ (primary)	-0.3%	[-0.6%, 0.2%]	22

Max RSS (memory usage)

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

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
Improvements ✅ (primary)	-1.8%	[-6.6%, -0.6%]	7
Improvements ✅ (secondary)	-1.6%	[-1.6%, -1.6%]	1
All ❌✅ (primary)	-1.8%	[-6.6%, -0.6%]	7

Cycles

Results (secondary 0.6%)

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

Binary size

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

Bootstrap: 485.484s -> 484.656s (-0.17%)
Artifact size: 394.88 MiB -> 396.90 MiB (0.51%)

[Zalathar]

Hmm, there’s still something weird going on in incr-full builds.

In theory this change should not substantially affect them, outside of the smaller dep graph, but benchmark detailed results show some unexpected changes in query execution/hit counts.

[Zalathar]

Closing this experiment for now; I might return to it later.