`rustc_borrowck` cleanups, part 2 #132623

nnethercote · 2024-11-05T03:05:09Z

The code under do_mir_borrowck is pretty messy, especially the various types like MirBorrowckCtxt, BorrowckInferCtxt, MirTypeckResults, MirTypeckRegionConstraints, CreateResult, TypeChecker, TypeVerifier, LivenessContext, LivenessResults. This PR does some tidying up, though there's still plenty of mess left afterwards.

A sequel to #132250.

r? @compiler-errors

compiler-errors · 2024-11-11T22:39:49Z

I don't have the time to review this right now (edit: sorry for the tone on the original message!)

r? compiler

compiler-errors · 2024-11-11T22:51:02Z

compiler/rustc_borrowck/src/type_check/mod.rs

@@ -2783,12 +2772,12 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
    ) -> ty::InstantiatedPredicates<'tcx> {
        if let Some(closure_requirements) = &tcx.mir_borrowck(def_id).closure_requirements {
            constraint_conversion::ConstraintConversion::new(
-                self.infcx,
+                self.infcx, // njn: change to BorrowckInferCtxt?


you should probably convert this to a fixme or address it

Nadrieril · 2024-11-15T17:31:59Z

@bors try @rust-timer queue

…2, r=<try> `rustc_borrowck` cleanups, part 2 The code under `do_mir_borrowck` is pretty messy, especially the various types like `MirBorrowckCtxt`, `BorrowckInferCtxt`, `MirTypeckResults`, `MirTypeckRegionConstraints`, `CreateResult`, `TypeChecker`, `TypeVerifier`, `LivenessContext`, `LivenessResults`. This PR does some tidying up, though there's still plenty of mess left afterwards. A sequel to rust-lang#132250. r? `@compiler-errors`

bors · 2024-11-15T17:33:16Z

⌛ Trying commit 976ba9e with merge 717a575...

Nadrieril · 2024-11-15T17:42:42Z

All good for me, I checked the logic and the changes are pretty unambiguously improvements. Much appreciation for the small and well-explained commits.

I second Err's remark about the FIXME comment; r=me once that is fixed if perf is good.

bors · 2024-11-15T17:46:15Z

💔 Test failed - checks-actions

bors · 2024-11-17T12:56:52Z

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

nnethercote · 2024-11-18T05:00:05Z

@bors try @rust-timer queue

bors · 2024-11-18T05:01:16Z

⌛ Trying commit 99e2f2d with merge f40e933...

…2, r=<try> `rustc_borrowck` cleanups, part 2 The code under `do_mir_borrowck` is pretty messy, especially the various types like `MirBorrowckCtxt`, `BorrowckInferCtxt`, `MirTypeckResults`, `MirTypeckRegionConstraints`, `CreateResult`, `TypeChecker`, `TypeVerifier`, `LivenessContext`, `LivenessResults`. This PR does some tidying up, though there's still plenty of mess left afterwards. A sequel to rust-lang#132250. r? `@compiler-errors`

bors · 2024-11-18T06:46:28Z

☀️ Try build successful - checks-actions
Build commit: f40e933 (f40e933faa4eb17682404f55de2724bf7e0ef960)

rust-timer · 2024-11-18T08:08:13Z

Finished benchmarking commit (f40e933): 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)	0.2%	[0.2%, 0.2%]	2
Regressions ❌ (secondary)	1.0%	[0.9%, 1.1%]	6
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	0.1%	[-0.1%, 0.2%]	3

Max RSS (memory usage)

Results (primary 1.5%, secondary 5.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.8%	[2.5%, 3.1%]	2
Regressions ❌ (secondary)	5.1%	[3.6%, 6.5%]	2
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.5%	[-1.0%, 3.1%]	3

Cycles

Results (primary -2.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
Improvements ✅ (primary)	-2.1%	[-2.7%, -1.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.1%	[-2.7%, -1.5%]	2

Binary size

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

Bootstrap: 788.58s -> 790.819s (0.28%)
Artifact size: 335.03 MiB -> 335.04 MiB (0.00%)

nnethercote · 2024-11-18T09:41:51Z

I think the perf results are just noise.

@rustbot label: +perf-regression-triaged

bors · 2024-11-18T23:28:18Z

📌 Commit 7b0619d has been approved by Nadrieril

It is now in the queue for this repository.

bors · 2024-11-19T00:22:48Z

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

`TypeChecker` already has it in a field.

This avoids the need to arena allocate it. `ConstraintConversion` needs some simple lifetime adjustments to allow this.

No reason not to be, and it's simpler that way.

There is an `Rc<UniversalRegions>` within `UniversalRegionRelations`, and yet the two types get passed around in tandem a lot. This commit makes `UniversalRegionRelations` own `UniversalRegions`, removing the `Rc` (which wasn't truly needed) and the tandem-passing. This requires adding a `universal_relations` method to `UniversalRegionRelations`, and renaming a couple of existing methods producing iterators to avoid a name clash.

It can be computed from `tcx` on demand, instead of computing it eagerly and passing it around.

It has a single call site.

It's not necessary.

Instead of destructuring it in advance and passing all the components individually. It's less code that way.

Because they get passed around together a lot.

It's simpler that way, and we don't need the explicit `drop`.

nnethercote · 2024-11-19T01:34:31Z

I rebased.

@bors r=Nadrieril

bors · 2024-11-19T01:34:34Z

📌 Commit 75108b6 has been approved by Nadrieril

It is now in the queue for this repository.

bors · 2024-11-19T08:32:31Z

⌛ Testing commit 75108b6 with merge 7d40450...

bors · 2024-11-19T11:40:08Z

☀️ Test successful - checks-actions
Approved by: Nadrieril
Pushing 7d40450 to master...

rust-timer · 2024-11-19T12:57:43Z

Finished benchmarking commit (7d40450): comparison URL.

Overall result: ❌ regressions - 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.9%	[0.4%, 1.1%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.6%)

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.5%	[1.2%, 1.8%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-3.2%, -1.5%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-3.2%, 1.8%]	5

Cycles

Results (secondary 1.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)	1.7%	[1.7%, 1.7%]	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: 792.888s -> 795.261s (0.30%)
Artifact size: 335.29 MiB -> 335.30 MiB (0.00%)

nnethercote · 2024-11-20T02:07:39Z

I did some local measurements that strongly indicated that the final commit was responsible for the wg-grammar regression. So I filed #133220 to revert that commit, but the perf run there showed it had no effect. Weird. I don't think it's worth spending more time on a mysterious regression that affects just a single secondary benchmark.

rustbot assigned compiler-errors Nov 5, 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 Nov 5, 2024

aDotInTheVoid added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Nov 5, 2024

rustbot assigned Nadrieril and unassigned compiler-errors Nov 11, 2024

compiler-errors reviewed Nov 11, 2024

View reviewed changes

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 Nov 15, 2024

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2024

nnethercote force-pushed the rustc_borrowck-cleanups-2 branch from 976ba9e to 99e2f2d Compare November 18, 2024 04:59

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 Nov 18, 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 18, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 19, 2024

nnethercote added 10 commits November 19, 2024 11:33

Don't pass universal_regions unnecessarily.

4c8a23a

`TypeChecker` already has it in a field.

Make TypeChecker::known_type_outlives_obligations owned.

227ecc8

This avoids the need to arena allocate it. `ConstraintConversion` needs some simple lifetime adjustments to allow this.

Make TypeChecker::region_bound_pairs owned.

ed11fbe

No reason not to be, and it's simpler that way.

Compute upvars lazily.

2709dc8

It can be computed from `tcx` on demand, instead of computing it eagerly and passing it around.

Inline and remove TypeVerifier::new.

fe3c49f

It has a single call site.

Don't refcount PlaceholderIndices.

801b150

It's not necessary.

Pass constraints to RegionInferenceContext::new.

c9283f8

Instead of destructuring it in advance and passing all the components individually. It's less code that way.

Put param_env into infcx.

af50fe4

Because they get passed around together a lot.

Pass flow_inits by value.

75108b6

It's simpler that way, and we don't need the explicit `drop`.

nnethercote force-pushed the rustc_borrowck-cleanups-2 branch from 7b0619d to 75108b6 Compare November 19, 2024 01:34

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 19, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 19, 2024

bors merged commit 7d40450 into rust-lang:master Nov 19, 2024
7 checks passed

rustbot added this to the 1.84.0 milestone Nov 19, 2024

rustbot removed the perf-regression Performance regression. label Nov 19, 2024

nnethercote deleted the rustc_borrowck-cleanups-2 branch November 19, 2024 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`rustc_borrowck` cleanups, part 2 #132623

`rustc_borrowck` cleanups, part 2 #132623

nnethercote commented Nov 5, 2024

compiler-errors commented Nov 11, 2024 •

edited

Loading

compiler-errors Nov 11, 2024

Nadrieril commented Nov 15, 2024

This comment has been minimized.

bors commented Nov 15, 2024

Nadrieril commented Nov 15, 2024

This comment has been minimized.

bors commented Nov 15, 2024

bors commented Nov 17, 2024

nnethercote commented Nov 18, 2024

This comment has been minimized.

bors commented Nov 18, 2024

bors commented Nov 18, 2024

This comment has been minimized.

rust-timer commented Nov 18, 2024

nnethercote commented Nov 18, 2024

bors commented Nov 18, 2024

bors commented Nov 19, 2024

nnethercote commented Nov 19, 2024

bors commented Nov 19, 2024

bors commented Nov 19, 2024

bors commented Nov 19, 2024

rust-timer commented Nov 19, 2024

nnethercote commented Nov 20, 2024

rustc_borrowck cleanups, part 2 #132623

rustc_borrowck cleanups, part 2 #132623

Conversation

nnethercote commented Nov 5, 2024

compiler-errors commented Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Nadrieril commented Nov 15, 2024

This comment has been minimized.

bors commented Nov 15, 2024

Nadrieril commented Nov 15, 2024

This comment has been minimized.

bors commented Nov 15, 2024

bors commented Nov 17, 2024

nnethercote commented Nov 18, 2024

This comment has been minimized.

bors commented Nov 18, 2024

bors commented Nov 18, 2024

This comment has been minimized.

rust-timer commented Nov 18, 2024

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

nnethercote commented Nov 18, 2024

bors commented Nov 18, 2024

bors commented Nov 19, 2024

nnethercote commented Nov 19, 2024

bors commented Nov 19, 2024

bors commented Nov 19, 2024

bors commented Nov 19, 2024

rust-timer commented Nov 19, 2024

Overall result: ❌ regressions - no action needed

nnethercote commented Nov 20, 2024

`rustc_borrowck` cleanups, part 2 #132623

`rustc_borrowck` cleanups, part 2 #132623

compiler-errors commented Nov 11, 2024 •

edited

Loading