Extend SCC construction to enable extra functionality #125069

amandasystems · 2024-05-13T12:06:03Z

Do YOU feel like your SCC construction doesn't do enough? Then I have a patch for you! SCCs can now do everything! Well, almost.

This patch has been extracted from #123720. It specifically enhances
Sccs to allow tracking arbitrary commutative properties (think min/max mappings on nodes vs arbitrary closures) of strongly connected components, including

reachable values (max/min)
SCC-internal values (max/min)

This helps with among other things universe computation. We can now identify
SCC universes as a reasonably straightforward "find max/min" operation during SCC construction. This is also included in this patch.

It's also more or less zero-cost; don't use the new features, don't pay for them.

This commit also vastly extends the documentation of the SCCs module, which I had a very hard time following. It may or may not have gotten easier to read for someone else.

I believe this logic can also be used in leak check, but haven't checked. Ha. ha. Ha.

This patch has been extracted from rust-lang#123720. It specifically enhances `Sccs` to allow tracking arbitrary commutative properties of SCCs, including - reachable values (max/min) - SCC-internal values (max/min) This helps with among other things universe computation: we can now identify SCC universes as a straightforward "find max/min" operation during SCC construction. It's also more or less zero-cost; don't use the new features, don't pay for them. This commit also vastly extends the documentation of the SCCs module, which I had a very hard time following.

rustbot · 2024-05-13T12:06:12Z

r? @pnkfelix

rustbot has assigned @pnkfelix.
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

amandasystems · 2024-05-13T12:21:30Z

r? Lqd

lqd · 2024-05-13T13:08:22Z

@bors try @rust-timer queue

As discussed on zulip, until I have more time let's also temporarily redirect to niko for review or reassignment, and discuss at this week's meeting: r? nikomatsakis

Extend SCC construction to enable extra functionality Do YOU feel like your SCC construction doesn't do enough? Then I have a patch for you! SCCs can now do *everything*! Well, almost. This patch has been extracted from rust-lang#123720. It specifically enhances `Sccs` to allow tracking arbitrary commutative properties (think min/max mappings on nodes vs arbitrary closures) of strongly connected components, including - reachable values (max/min) - SCC-internal values (max/min) This helps with among other things universe computation. We can now identify SCC universes as a reasonably straightforward "find max/min" operation during SCC construction. This is also included in this patch. It's also more or less zero-cost; don't use the new features, don't pay for them. This commit also vastly extends the documentation of the SCCs module, which I had a very hard time following. It may or may not have gotten easier to read for someone else. I believe this logic can also be used in leak check, but haven't checked. Ha. ha. Ha.

bors · 2024-05-13T13:09:33Z

⌛ Trying commit 609a202 with merge 6c7f0a6...

bors · 2024-05-13T14:47:07Z

☀️ Try build successful - checks-actions
Build commit: 6c7f0a6 (6c7f0a6914780a8bd5240c26e668f7b0625b66dc)

rust-timer · 2024-05-13T16:03:36Z

Finished benchmarking commit (6c7f0a6): comparison URL.

Overall result: ❌ regressions - 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.

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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.2%, 1.6%]	45
Regressions ❌ (secondary)	1.4%	[0.1%, 4.0%]	32
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.2%, 1.6%]	45

Max RSS (memory usage)

Results

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.8%	[2.4%, 3.2%]	2
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	2
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[-2.6%, 3.2%]	3

Cycles

Results

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

Binary size

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

Bootstrap: 675.291s -> 676.941s (0.24%)
Artifact size: 315.98 MiB -> 316.29 MiB (0.10%)

Kobzol · 2024-05-17T13:19:27Z

@bors try @rust-timer queue

bors · 2024-05-17T13:25:36Z

⌛ Trying commit c510e73 with merge 1805d8a...

Extend SCC construction to enable extra functionality Do YOU feel like your SCC construction doesn't do enough? Then I have a patch for you! SCCs can now do *everything*! Well, almost. This patch has been extracted from rust-lang#123720. It specifically enhances `Sccs` to allow tracking arbitrary commutative properties (think min/max mappings on nodes vs arbitrary closures) of strongly connected components, including - reachable values (max/min) - SCC-internal values (max/min) This helps with among other things universe computation. We can now identify SCC universes as a reasonably straightforward "find max/min" operation during SCC construction. This is also included in this patch. It's also more or less zero-cost; don't use the new features, don't pay for them. This commit also vastly extends the documentation of the SCCs module, which I had a very hard time following. It may or may not have gotten easier to read for someone else. I believe this logic can also be used in leak check, but haven't checked. Ha. ha. Ha.

Note that this changes no executing code. The change is 100% in documentation.

amandasystems · 2024-05-17T15:00:46Z

My guess is 0.3% primary regressions and 0.84% in secondary, let's see how wrong I am!

Also, you can't edit on GitHub so there is no way for me to remove this if I'm enormously off. Too bad!

Kobzol · 2024-05-20T13:03:03Z

@bors try @rust-timer queue

Extend SCC construction to enable extra functionality Do YOU feel like your SCC construction doesn't do enough? Then I have a patch for you! SCCs can now do *everything*! Well, almost. This patch has been extracted from rust-lang#123720. It specifically enhances `Sccs` to allow tracking arbitrary commutative properties (think min/max mappings on nodes vs arbitrary closures) of strongly connected components, including - reachable values (max/min) - SCC-internal values (max/min) This helps with among other things universe computation. We can now identify SCC universes as a reasonably straightforward "find max/min" operation during SCC construction. This is also included in this patch. It's also more or less zero-cost; don't use the new features, don't pay for them. This commit also vastly extends the documentation of the SCCs module, which I had a very hard time following. It may or may not have gotten easier to read for someone else. I believe this logic can also be used in leak check, but haven't checked. Ha. ha. Ha.

bors · 2024-05-20T13:04:16Z

⌛ Trying commit b453158 with merge f02f9b5...

jackh726 · 2024-05-20T13:27:57Z

@bors try @rust-timer queue

Extend SCC construction to enable extra functionality Do YOU feel like your SCC construction doesn't do enough? Then I have a patch for you! SCCs can now do *everything*! Well, almost. This patch has been extracted from rust-lang#123720. It specifically enhances `Sccs` to allow tracking arbitrary commutative properties (think min/max mappings on nodes vs arbitrary closures) of strongly connected components, including - reachable values (max/min) - SCC-internal values (max/min) This helps with among other things universe computation. We can now identify SCC universes as a reasonably straightforward "find max/min" operation during SCC construction. This is also included in this patch. It's also more or less zero-cost; don't use the new features, don't pay for them. This commit also vastly extends the documentation of the SCCs module, which I had a very hard time following. It may or may not have gotten easier to read for someone else. I believe this logic can also be used in leak check, but haven't checked. Ha. ha. Ha.

bors · 2024-05-20T13:30:05Z

⌛ Trying commit b453158 with merge f3129a5...

Kobzol · 2024-05-20T13:35:29Z

Hmm, looks like we started two try runs in a row, but the previous one was not cancelled. That looks like a CI config bug (?).

jackh726 · 2024-05-20T14:24:28Z

Oops, I missed that you started a try/perf run before I did...

bors · 2024-05-20T15:07:18Z

☀️ Try build successful - checks-actions
Build commit: f3129a5 (f3129a502c9cb5f245ac39b47c8a169b68fba662)

rust-timer · 2024-05-20T16:57:05Z

Finished benchmarking commit (f3129a5): comparison URL.

Overall result: ❌ regressions - 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.

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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.2%, 0.7%]	26
Regressions ❌ (secondary)	1.3%	[0.1%, 3.9%]	21
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.7%]	26

Max RSS (memory usage)

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

Cycles

Results (primary 2.4%, secondary 1.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)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	2.2%	[2.1%, 2.5%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.9%, -2.0%]	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: 670.321s -> 670.92s (0.09%)
Artifact size: 316.18 MiB -> 316.07 MiB (-0.03%)

compiler/rustc_borrowck/src/region_infer/mod.rs

compiler/rustc_data_structures/src/graph/scc/mod.rs

compiler/rustc_borrowck/src/region_infer/mod.rs

compiler/rustc_data_structures/src/graph/scc/mod.rs

compiler/rustc_borrowck/src/region_infer/mod.rs

Apparently this interferes with inlining and murders performance on `wg-grammar`.

@lqd

This commit addresses @lqd's code review and resurrects a lost comment and removes some dead code.

In terms of code organisation, this is a lot cleaner and allows tighter access modifiers.

lqd · 2024-05-22T10:19:28Z

@bors try @rust-timer queue

Extend SCC construction to enable extra functionality Do YOU feel like your SCC construction doesn't do enough? Then I have a patch for you! SCCs can now do *everything*! Well, almost. This patch has been extracted from rust-lang#123720. It specifically enhances `Sccs` to allow tracking arbitrary commutative properties (think min/max mappings on nodes vs arbitrary closures) of strongly connected components, including - reachable values (max/min) - SCC-internal values (max/min) This helps with among other things universe computation. We can now identify SCC universes as a reasonably straightforward "find max/min" operation during SCC construction. This is also included in this patch. It's also more or less zero-cost; don't use the new features, don't pay for them. This commit also vastly extends the documentation of the SCCs module, which I had a very hard time following. It may or may not have gotten easier to read for someone else. I believe this logic can also be used in leak check, but haven't checked. Ha. ha. Ha.

bors · 2024-05-22T10:20:42Z

⌛ Trying commit 58ccdd0 with merge 4580462...

bors · 2024-05-22T12:01:07Z

☀️ Try build successful - checks-actions
Build commit: 4580462 (4580462219447558f8ed6b6c8e1baa94d6fc24f8)

rust-timer · 2024-05-22T13:20:17Z

Finished benchmarking commit (4580462): comparison URL.

Overall result: ❌ regressions - 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.

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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	2
Regressions ❌ (secondary)	0.6%	[0.2%, 1.1%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	2

Max RSS (memory usage)

Results (primary -2.3%, secondary -3.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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-3.0%	[-3.2%, -2.9%]	2
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

Cycles

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

Binary size

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

Bootstrap: 672.666s -> 672.967s (0.04%)
Artifact size: 315.47 MiB -> 316.17 MiB (0.22%)

amandasystems added 2 commits May 13, 2024 13:47

Documentation fixes

609a202

rustbot assigned pnkfelix May 13, 2024

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 May 13, 2024

rustbot assigned lqd and unassigned pnkfelix May 13, 2024

This comment has been minimized.

Sign in to view

rustbot assigned nikomatsakis and unassigned lqd May 13, 2024

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

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 13, 2024

amandasystems added 2 commits May 17, 2024 15:11

Simplify path compression logic

cc132b5

Formatting, weird because I just did that

c510e73

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 May 17, 2024

Docstring for for Annotation

46d3568

Note that this changes no executing code. The change is 100% in documentation.

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 May 20, 2024

This comment has been minimized.

Sign in to view

amandasystems mentioned this pull request May 20, 2024

[WIP] Rewrite handling of universe-leaking placeholder regions into outlives constraints #123720

Open

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 May 20, 2024

lqd reviewed May 20, 2024

View reviewed changes

amandasystems added 4 commits May 21, 2024 10:18

Roll back a few #[instrument]

565c8e9

Apparently this interferes with inlining and murders performance on `wg-grammar`.

Resurrect some comments, remove a pub

32146ec

This commit addresses @lqd's code review and resurrects a lost comment and removes some dead code.

Revise documentation after @lqd's comments

7a32009

Move RegionTracker to region_infer

58ccdd0

In terms of code organisation, this is a lot cleaner and allows tighter access modifiers.

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 May 22, 2024

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 May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend SCC construction to enable extra functionality #125069

Extend SCC construction to enable extra functionality #125069

amandasystems commented May 13, 2024

rustbot commented May 13, 2024

amandasystems commented May 13, 2024

lqd commented May 13, 2024

This comment has been minimized.

bors commented May 13, 2024

bors commented May 13, 2024

This comment has been minimized.

rust-timer commented May 13, 2024

Kobzol commented May 17, 2024

This comment has been minimized.

bors commented May 17, 2024

amandasystems commented May 17, 2024 •

edited

Kobzol commented May 20, 2024

This comment has been minimized.

bors commented May 20, 2024

jackh726 commented May 20, 2024

This comment has been minimized.

bors commented May 20, 2024

Kobzol commented May 20, 2024

jackh726 commented May 20, 2024

bors commented May 20, 2024

This comment has been minimized.

rust-timer commented May 20, 2024

lqd commented May 22, 2024

This comment has been minimized.

bors commented May 22, 2024

bors commented May 22, 2024

This comment has been minimized.

rust-timer commented May 22, 2024

Extend SCC construction to enable extra functionality #125069

Are you sure you want to change the base?

Extend SCC construction to enable extra functionality #125069

Conversation

amandasystems commented May 13, 2024

rustbot commented May 13, 2024

amandasystems commented May 13, 2024

lqd commented May 13, 2024

This comment has been minimized.

bors commented May 13, 2024

bors commented May 13, 2024

This comment has been minimized.

rust-timer commented May 13, 2024

Overall result: ❌ regressions - ACTION NEEDED

Kobzol commented May 17, 2024

This comment has been minimized.

bors commented May 17, 2024

amandasystems commented May 17, 2024 • edited

Kobzol commented May 20, 2024

This comment has been minimized.

bors commented May 20, 2024

jackh726 commented May 20, 2024

This comment has been minimized.

bors commented May 20, 2024

Kobzol commented May 20, 2024

jackh726 commented May 20, 2024

bors commented May 20, 2024

This comment has been minimized.

rust-timer commented May 20, 2024

Overall result: ❌ regressions - ACTION NEEDED

lqd commented May 22, 2024

This comment has been minimized.

bors commented May 22, 2024

bors commented May 22, 2024

This comment has been minimized.

rust-timer commented May 22, 2024

Overall result: ❌ regressions - ACTION NEEDED

amandasystems commented May 17, 2024 •

edited