Remove `GenKillAnalysis` #131481

nnethercote · 2024-10-10T02:38:23Z

There are two kinds of dataflow analysis in the compiler: Analysis, which is the basic kind, and GenKillAnalysis, which is a more specialized kind for gen/kill analyses that is intended as an optimization. However, it turns out that GenKillAnalysis is actually a pessimization! It's faster (and much simpler) to do all the gen/kill analyses via Analysis. This lets us remove GenKillAnalysis, and GenKillSet, and a few other things, and also merge AnalysisDomain into Analysis. The PR removes 500 lines of code and improves performance.

r? @tmiasko

nnethercote · 2024-10-10T02:38:39Z

@bors try @rust-timer queue

bors · 2024-10-10T02:39:49Z

⌛ Trying commit 5639840 with merge 690cb9f...

Remove `GenKillAnalysis` r? `@ghost`

bors · 2024-10-10T04:23:22Z

☀️ Try build successful - checks-actions
Build commit: 690cb9f (690cb9f750436d665c936d6f1b3ddb83c9f7c196)

rust-timer · 2024-10-10T06:45:31Z

Finished benchmarking commit (690cb9f): 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.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-0.3%	[-0.7%, -0.2%]	53
Improvements ✅ (secondary)	-0.7%	[-1.7%, -0.1%]	23
All ❌✅ (primary)	-0.3%	[-0.7%, -0.2%]	53

Max RSS (memory usage)

Results (primary -3.0%, secondary 1.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)	1.9%	[1.9%, 1.9%]	2
Improvements ✅ (primary)	-3.0%	[-3.0%, -3.0%]	1
Improvements ✅ (secondary)	-0.7%	[-0.7%, -0.7%]	1
All ❌✅ (primary)	-3.0%	[-3.0%, -3.0%]	1

Cycles

Results (primary -1.0%, secondary -0.7%)

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)	5.5%	[3.9%, 8.4%]	3
Improvements ✅ (primary)	-1.0%	[-1.2%, -0.7%]	13
Improvements ✅ (secondary)	-2.4%	[-3.0%, -1.8%]	11
All ❌✅ (primary)	-1.0%	[-1.2%, -0.7%]	13

Binary size

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

Bootstrap: 775.903s -> 772.537s (-0.43%)
Artifact size: 329.71 MiB -> 329.49 MiB (-0.07%)

nnethercote · 2024-10-10T09:15:23Z

I was hoping for neutral performance effects, but the perf run is much better than that: icount results are great, cycles results are good, bootstrap time drops by 3.4s, and artifact size drops by 228 KiB.

rustbot · 2024-10-10T09:17:36Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

nnethercote · 2024-10-10T09:22:28Z

cc @cjgillot

cjgillot · 2024-10-12T23:23:03Z

IIRC, the GenKill version has a different asymptotic complexity: it avoids looping through all statements when looping for fixed-point. Could this change introduce a degenerate worst case? Or is it too unlikely to warrant the maintenance cost?

tmiasko · 2024-10-13T12:40:59Z

compiler/rustc_mir_dataflow/src/framework/mod.rs

+//! To use this framework, implement the [`Analysis`] trait. There used to be a `GenKillAnalysis`
+//! alternative trait intended as an optimzation, but it ended up not being any faster than
+//! `Analysis`. `impls` module contains several examples of dataflow analyses.


GenKillAnalysis will be unfamiliar to future readers. Can you describe that the main idea was to pre-compute transfer function for each block or omit the historical comment altogether.

tmiasko · 2024-10-13T13:21:52Z

I am in favor of removing the GenKillAnalysis, unless someone champions the idea and adds a motivating compile time benchmark.

LGTM.

r? cjgillot

nnethercote · 2024-10-14T03:18:13Z

IIRC, the GenKill version has a different asymptotic complexity: it avoids looping through all statements when looping for fixed-point. Could this change introduce a degenerate worst case? Or is it too unlikely to warrant the maintenance cost?

I'm no expert on the analyses, but I do have pretty high confidence in the coverage of our test suite, such that any degenerate cases are likely to be very rare. Also, it's interesting that the largest wins were in the secondary benchmarks such as await-call-tree and deeply-nested-multi, two artificial stress tests that caused performance problems in the past.

It's hardly worth it, and it needs to be removed so that `GenKillAnalysis` can be removed.

This is an alternative to `Engine::new_generic` for gen/kill analyses. It's supposed to be an optimization, but it has negligible effect. The commit merges `Engine::new_generic` into `Engine::new`. This allows the removal of various other things: `GenKillSet`, `gen_kill_statement_effects_in_block`, `is_cfg_cyclic`.

`GenKillAnalysis` has very similar methods to `Analysis`, but the first two have a notable difference: the second argument is `&mut impl GenKill<Self::Idx>` instead of `&mut Self::Domain`. But thanks to the previous commit, this difference is no longer necessary.

Thanks to the previous couple of commits, many uses of the `GenKill` trait can be replaced with a concrete type.

It's now functionally identical to `Analysis`.

With `GenKillAnalysis` gone, there is no need for them to be separate.

…ct}`. To avoid some low-value boilerplate code.

cjgillot · 2024-10-14T10:43:36Z

I don't have the same confidence in the test suite, but I don't have any argument besides "may".
Let's go, and see if we get a perf report.
@bors r+

bors · 2024-10-14T10:43:39Z

📌 Commit 33abf6a has been approved by cjgillot

It is now in the queue for this repository.

bors · 2024-10-14T15:12:58Z

⌛ Testing commit 33abf6a with merge 8f78cbd...

Remove `GenKillAnalysis` There are two kinds of dataflow analysis in the compiler: `Analysis`, which is the basic kind, and `GenKillAnalysis`, which is a more specialized kind for gen/kill analyses that is intended as an optimization. However, it turns out that `GenKillAnalysis` is actually a pessimization! It's faster (and much simpler) to do all the gen/kill analyses via `Analysis`. This lets us remove `GenKillAnalysis`, and `GenKillSet`, and a few other things, and also merge `AnalysisDomain` into `Analysis`. The PR removes 500 lines of code and improves performance. r? `@tmiasko`

bors · 2024-10-14T15:15:25Z

💔 Test failed - checks-actions

matthiaskrgr · 2024-10-14T15:16:38Z

@bors retry

rust-log-analyzer · 2024-10-14T15:31:35Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#3 [auth] library/centos:pull token for registry-1.docker.io
#3 DONE 0.0s

#4 [internal] load metadata for docker.io/library/centos:7
#4 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/centos/manifests/sha256:be65f488b7764ad3638f236b7b515b3678369a5124c47b8d32916d6487418ea4: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
 > [internal] load metadata for docker.io/library/centos:7:
------
Dockerfile:5
--------------------
--------------------
   3 |     # actually use newer APIs in rustc or std without a fallback. It's more
   4 |     # important that we match glibc for ELF symbol versioning.
   5 | >>> FROM centos:7
   6 |     
   7 |     WORKDIR /build
--------------------
ERROR: failed to solve: centos:7: failed to resolve source metadata for docker.io/library/centos:7: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/centos/manifests/sha256:be65f488b7764ad3638f236b7b515b3678369a5124c47b8d32916d6487418ea4: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit (did you mean centos?)
#0 building with "exciting_robinson" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 3.22kB done
#1 transferring dockerfile: 3.22kB done
#1 DONE 0.0s

#2 [internal] load metadata for docker.io/library/centos:7
#2 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/centos/manifests/sha256:be65f488b7764ad3638f236b7b515b3678369a5124c47b8d32916d6487418ea4: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
 > [internal] load metadata for docker.io/library/centos:7:
------
Dockerfile:5
--------------------
--------------------
   3 |     # actually use newer APIs in rustc or std without a fallback. It's more
   4 |     # important that we match glibc for ELF symbol versioning.
   5 | >>> FROM centos:7
   6 |     
   7 |     WORKDIR /build
--------------------
ERROR: failed to solve: centos:7: failed to resolve source metadata for docker.io/library/centos:7: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/centos/manifests/sha256:be65f488b7764ad3638f236b7b515b3678369a5124c47b8d32916d6487418ea4: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit (did you mean centos?)
#0 building with "exciting_robinson" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 3.22kB done
#1 transferring dockerfile: 3.22kB done
#1 DONE 0.0s

#2 [internal] load metadata for docker.io/library/centos:7
#2 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/centos/manifests/sha256:be65f488b7764ad3638f236b7b515b3678369a5124c47b8d32916d6487418ea4: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
 > [internal] load metadata for docker.io/library/centos:7:
------
Dockerfile:5
--------------------
--------------------
   3 |     # actually use newer APIs in rustc or std without a fallback. It's more
   4 |     # important that we match glibc for ELF symbol versioning.
   5 | >>> FROM centos:7
   6 |     
   7 |     WORKDIR /build
--------------------
ERROR: failed to solve: centos:7: failed to resolve source metadata for docker.io/library/centos:7: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/centos/manifests/sha256:be65f488b7764ad3638f236b7b515b3678369a5124c47b8d32916d6487418ea4: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit (did you mean centos?)
#0 building with "exciting_robinson" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 3.22kB done
#1 transferring dockerfile: 3.22kB done
#1 DONE 0.0s

#2 [internal] load metadata for docker.io/library/centos:7
#2 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/centos/manifests/sha256:be65f488b7764ad3638f236b7b515b3678369a5124c47b8d32916d6487418ea4: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
 > [internal] load metadata for docker.io/library/centos:7:
------
Dockerfile:5
--------------------
--------------------
   3 |     # actually use newer APIs in rustc or std without a fallback. It's more
   4 |     # important that we match glibc for ELF symbol versioning.
   5 | >>> FROM centos:7
   6 |     
   7 |     WORKDIR /build
--------------------
ERROR: failed to solve: centos:7: failed to resolve source metadata for docker.io/library/centos:7: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/centos/manifests/sha256:be65f488b7764ad3638f236b7b515b3678369a5124c47b8d32916d6487418ea4: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit (did you mean centos?)
#0 building with "exciting_robinson" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 3.22kB done
#1 transferring dockerfile: 3.22kB done
#1 DONE 0.0s

#2 [internal] load metadata for docker.io/library/centos:7
#2 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/centos/manifests/sha256:be65f488b7764ad3638f236b7b515b3678369a5124c47b8d32916d6487418ea4: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
 > [internal] load metadata for docker.io/library/centos:7:
------
Dockerfile:5
--------------------
--------------------
   3 |     # actually use newer APIs in rustc or std without a fallback. It's more
   4 |     # important that we match glibc for ELF symbol versioning.
   5 | >>> FROM centos:7
   6 |     
   7 |     WORKDIR /build
--------------------
ERROR: failed to solve: centos:7: failed to resolve source metadata for docker.io/library/centos:7: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/centos/manifests/sha256:be65f488b7764ad3638f236b7b515b3678369a5124c47b8d32916d6487418ea4: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit (did you mean centos?)
##[error]Process completed with exit code 1.
Post job cleanup.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 10, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 10, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2024

Auto merge of rust-lang#131481 - nnethercote:rm-GenKillSet, r=<try>

690cb9f

Remove `GenKillAnalysis` r? `@ghost`

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 10, 2024

rustbot assigned tmiasko Oct 10, 2024

nnethercote force-pushed the rm-GenKillSet branch from 5639840 to 28b7c57 Compare October 10, 2024 09:13

nnethercote marked this pull request as ready for review October 10, 2024 09:17

tmiasko reviewed Oct 13, 2024

View reviewed changes

rustbot assigned cjgillot and unassigned tmiasko Oct 13, 2024

nnethercote added 6 commits October 14, 2024 16:35

Remove ResultsCursor::contains.

874b03e

It's hardly worth it, and it needs to be removed so that `GenKillAnalysis` can be removed.

Minimize use of GenKill.

525f655

Thanks to the previous couple of commits, many uses of the `GenKill` trait can be replaced with a concrete type.

Remove GenKillAnalysis.

4dc1b4d

It's now functionally identical to `Analysis`.

Merge AnalysisDomain into Analysis.

ba13775

With `GenKillAnalysis` gone, there is no need for them to be separate.

Add defaults for `Analysis::apply_{call_return_effect,terminator_effe…

33abf6a

…ct}`. To avoid some low-value boilerplate code.

nnethercote force-pushed the rm-GenKillSet branch from 28b7c57 to 33abf6a Compare October 14, 2024 05:35

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2024

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 14, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `GenKillAnalysis` #131481

Remove `GenKillAnalysis` #131481

nnethercote commented Oct 10, 2024 •

edited

Loading

nnethercote commented Oct 10, 2024

This comment has been minimized.

bors commented Oct 10, 2024

This comment has been minimized.

bors commented Oct 10, 2024

This comment has been minimized.

rust-timer commented Oct 10, 2024

nnethercote commented Oct 10, 2024 •

edited

Loading

rustbot commented Oct 10, 2024

nnethercote commented Oct 10, 2024

cjgillot commented Oct 12, 2024

tmiasko Oct 13, 2024

tmiasko commented Oct 13, 2024

nnethercote commented Oct 14, 2024

cjgillot commented Oct 14, 2024

bors commented Oct 14, 2024

bors commented Oct 14, 2024

bors commented Oct 14, 2024

matthiaskrgr commented Oct 14, 2024

rust-log-analyzer commented Oct 14, 2024

Remove GenKillAnalysis #131481

Are you sure you want to change the base?

Remove GenKillAnalysis #131481

Conversation

nnethercote commented Oct 10, 2024 • edited Loading

nnethercote commented Oct 10, 2024

This comment has been minimized.

bors commented Oct 10, 2024

This comment has been minimized.

bors commented Oct 10, 2024

This comment has been minimized.

rust-timer commented Oct 10, 2024

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

nnethercote commented Oct 10, 2024 • edited Loading

rustbot commented Oct 10, 2024

nnethercote commented Oct 10, 2024

cjgillot commented Oct 12, 2024

tmiasko Oct 13, 2024

Choose a reason for hiding this comment

tmiasko commented Oct 13, 2024

nnethercote commented Oct 14, 2024

cjgillot commented Oct 14, 2024

bors commented Oct 14, 2024

bors commented Oct 14, 2024

bors commented Oct 14, 2024

matthiaskrgr commented Oct 14, 2024

rust-log-analyzer commented Oct 14, 2024

Remove `GenKillAnalysis` #131481

Remove `GenKillAnalysis` #131481

nnethercote commented Oct 10, 2024 •

edited

Loading

nnethercote commented Oct 10, 2024 •

edited

Loading