use Vec for region constraints instead of BTreeMap #118824

aliemjay · 2023-12-11T13:07:48Z

~1% perf gain

Diagnostic regressions need more investigation.

rustbot · 2023-12-11T13:10:43Z

Failed to set assignee to ghost: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

aliemjay · 2023-12-11T13:14:35Z

@bors try @rust-timer queue

bors · 2023-12-11T13:15:45Z

⌛ Trying commit fb14cf7 with merge 2fc4795...

use Vec for region constraints instead of BTreeMap ~1% perf gain Diagnostic regressions need more investigation. r? `@ghost`

Nilstrieb · 2023-12-11T13:41:18Z

compiler/rustc_infer/src/infer/region_constraints/mod.rs

@@ -273,7 +272,7 @@ pub(crate) enum UndoLog<'tcx> {
    AddVar(RegionVid),

    /// We added the given `constraint`.
-    AddConstraint(Constraint<'tcx>),
+    AddConstraint(usize),


Could be an IndexVec :)

or simply remove the index from AddConstraint? 🤷‍♂️ It is useless and I only added it for consistency with AddVerify.

It is useless and I only added it for consistency with AddVerify.

This is wrong. The index was actually needed in in leak_check code, ~~but now I have removed the UndoLog altogether so it doesn't matter anymore~~ (*edit: I reverted this commit).

Adding a newtype index seems incompatible with sorting and deduplicating the IdexVec IMO.

bors · 2023-12-11T14:41:45Z

☀️ Try build successful - checks-actions
Build commit: 2fc4795 (2fc4795b87dcc533a38f9e9507ed9c7e5a782446)

rust-timer · 2023-12-11T17:14:38Z

Finished benchmarking commit (2fc4795): 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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-1.4%, -0.2%]	44
Improvements ✅ (secondary)	-0.7%	[-1.8%, -0.1%]	30
All ❌✅ (primary)	-0.6%	[-1.4%, -0.2%]	44

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)	6.9%	[6.9%, 6.9%]	1
Regressions ❌ (secondary)	4.9%	[4.2%, 5.5%]	2
Improvements ✅ (primary)	-1.2%	[-1.2%, -1.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.8%	[-1.2%, 6.9%]	2

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)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.3%, -1.0%]	2
Improvements ✅ (secondary)	-2.2%	[-2.6%, -1.9%]	8
All ❌✅ (primary)	-1.1%	[-1.3%, -1.0%]	2

Binary size

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

Bootstrap: 668.867s -> 668.905s (0.01%)
Artifact size: 314.18 MiB -> 314.18 MiB (0.00%)

compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs

michaelwoerister · 2023-12-14T10:33:49Z

This looks similar to SortedMap. Using that might simplify the code.

aliemjay · 2023-12-15T11:14:27Z

another experiment: remove undo log
@bors try @rust-timer queue

bors · 2023-12-15T11:15:37Z

⌛ Trying commit f388f23 with merge e499e7a...

use Vec for region constraints instead of BTreeMap ~1% perf gain Diagnostic regressions need more investigation. r? `@ghost`

aliemjay · 2023-12-15T12:20:59Z

This looks similar to SortedMap. Using that might simplify the code.

@michaelwoerister A downside is that it incurs a perf penalty for every insert operation when in practice most inserted region constraints end up being removed either by rolling back to a previous snapshot or by ignoring region resolution entirely.

michaelwoerister · 2023-12-15T12:41:26Z

Ah OK, that makes sense!

bors · 2023-12-15T12:42:17Z

☀️ Try build successful - checks-actions
Build commit: e499e7a (e499e7ae9f0bbf4e31d9098449d41b915437833e)

rust-timer · 2023-12-15T14:02:36Z

Finished benchmarking commit (e499e7a): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.4%	[0.2%, 0.8%]	8
Regressions ❌ (secondary)	0.8%	[0.1%, 1.6%]	12
Improvements ✅ (primary)	-0.7%	[-1.6%, -0.2%]	36
Improvements ✅ (secondary)	-0.8%	[-2.1%, -0.2%]	27
All ❌✅ (primary)	-0.5%	[-1.6%, 0.8%]	44

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)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.0%, 2.7%]	2
Improvements ✅ (primary)	-1.6%	[-2.7%, -0.6%]	2
Improvements ✅ (secondary)	-2.2%	[-3.1%, -0.4%]	6
All ❌✅ (primary)	-1.6%	[-2.7%, -0.6%]	2

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.4%	[1.4%, 1.4%]	1
Improvements ✅ (primary)	-1.3%	[-2.2%, -0.6%]	12
Improvements ✅ (secondary)	-2.2%	[-3.1%, -0.6%]	12
All ❌✅ (primary)	-1.3%	[-2.2%, -0.6%]	12

Binary size

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

Bootstrap: 672.135s -> 670.525s (-0.24%)
Artifact size: 312.47 MiB -> 312.34 MiB (-0.04%)

aliemjay · 2023-12-17T07:37:32Z

I reverted the UndoLog experiment, so this is no longer a perf regression as per the original perf run.
@rustbot label: -perf-regression

The diagnostic regression is fixed by the last commit.

r? compiler
@rustbot ready

compiler-errors · 2023-12-21T15:46:35Z

tests/ui/higher-ranked/higher-ranked-lifetime-error.stderr

@@ -4,8 +4,8 @@ error[E0308]: mismatched types
 LL |     assert_all::<_, &String>(id);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
   |
-   = note: expected reference `&String`
-              found reference `&String`
+   = note: expected trait `for<'a> <for<'a> fn(&'a String) -> &'a String {id} as FnMut<(&'a String,)>>`


Why did this change 🤔

Ah, it's the second commit.

compiler-errors · 2023-12-21T15:48:53Z

@bors r+

bors · 2023-12-21T15:48:56Z

📌 Commit 8c215e7 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-12-21T23:13:07Z

⌛ Testing commit 8c215e7 with merge 8b30243...

…r-errors use Vec for region constraints instead of BTreeMap ~1% perf gain Diagnostic regressions need more investigation. r? `@ghost`

rust-log-analyzer · 2023-12-22T00:02:21Z

The job aarch64-gnu failed! Check out the build log: (web) (plain)

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

[RUSTC-TIMING] serde test:false 8.189
[RUSTC-TIMING] rustdoc_json_types test:false 3.137
[RUSTC-TIMING] basic_toml test:false 1.412
   Compiling askama_derive v0.12.1
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
##[group]Clock drift check
  local time: Fri Dec 22 00:02:06 UTC 2023
  local time: Fri Dec 22 00:02:06 UTC 2023
Session terminated, killing shell... ...killed.
##[error]The operation was canceled.
Cleaning up orphan processes

bors · 2023-12-22T00:02:51Z

💔 Test failed - checks-actions

compiler-errors · 2023-12-22T18:30:10Z

Unrelated

@bors retry

bors · 2023-12-22T20:28:52Z

⌛ Testing commit 8c215e7 with merge d6d7a93...

bors · 2023-12-22T22:26:28Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing d6d7a93 to master...

rust-timer · 2023-12-22T23:47:55Z

Finished benchmarking commit (d6d7a93): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-1.3%, -0.2%]	39
Improvements ✅ (secondary)	-0.7%	[-1.7%, -0.2%]	29
All ❌✅ (primary)	-0.5%	[-1.3%, -0.2%]	39

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)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.5%, 0.6%]	2
Improvements ✅ (primary)	-1.4%	[-1.4%, -1.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.4%	[-1.4%, -1.4%]	1

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)	-	-	0
Improvements ✅ (primary)	-1.4%	[-1.5%, -1.2%]	2
Improvements ✅ (secondary)	-2.7%	[-2.8%, -2.6%]	2
All ❌✅ (primary)	-1.4%	[-1.5%, -1.2%]	2

Binary size

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

Bootstrap: 674.597s -> 672.389s (-0.33%)
Artifact size: 312.79 MiB -> 312.74 MiB (-0.02%)

Stop sorting via `DefId`s in region resolution hopefully maintains the perf improvement from rust-lang#118824 works towards rust-lang#90317

…ichaelwoerister Stop sorting via `DefId`s in region resolution hopefully maintains the perf improvement from rust-lang#118824 works towards rust-lang#90317

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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 11, 2023

aliemjay changed the title ~~use Vec for region constraints instead for BTreeMap~~ use Vec for region constraints instead of BTreeMap Dec 11, 2023

rust-cloud-vms bot force-pushed the perf-region-cons branch from 59acb33 to fb14cf7 Compare December 11, 2023 13:14

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 Dec 11, 2023

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2023

Auto merge of rust-lang#118824 - aliemjay:perf-region-cons, r=<try>

2fc4795

use Vec for region constraints instead of BTreeMap ~1% perf gain Diagnostic regressions need more investigation. r? `@ghost`

Nilstrieb reviewed Dec 11, 2023

View reviewed changes

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 Dec 11, 2023

the8472 reviewed Dec 11, 2023

View reviewed changes

compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs Show resolved Hide resolved

rust-cloud-vms bot force-pushed the perf-region-cons branch from fb14cf7 to f388f23 Compare December 15, 2023 11:13

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 Dec 15, 2023

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 15, 2023

Auto merge of rust-lang#118824 - aliemjay:perf-region-cons, r=<try>

e499e7a

use Vec for region constraints instead of BTreeMap ~1% perf gain Diagnostic regressions need more investigation. r? `@ghost`

This comment has been minimized.

Sign in to view

rustbot added the perf-regression Performance regression. label Dec 15, 2023

fix diagnostic regresssion

8c215e7

rust-cloud-vms bot force-pushed the perf-region-cons branch from f388f23 to 8c215e7 Compare December 17, 2023 07:31

rustbot removed the perf-regression Performance regression. label Dec 17, 2023

rustbot assigned compiler-errors Dec 17, 2023

aliemjay marked this pull request as ready for review December 17, 2023 07:37

compiler-errors reviewed Dec 21, 2023

View reviewed changes

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 Dec 21, 2023

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 Dec 22, 2023

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 Dec 22, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 22, 2023

bors merged commit d6d7a93 into rust-lang:master Dec 22, 2023
12 checks passed

rustbot added this to the 1.77.0 milestone Dec 22, 2023

bors mentioned this pull request Dec 22, 2023

Cleanup error handlers: round 4 #119171

Merged

oli-obk mentioned this pull request Mar 21, 2024

Stop sorting via DefIds in region resolution #122824

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use Vec for region constraints instead of BTreeMap #118824

use Vec for region constraints instead of BTreeMap #118824

aliemjay commented Dec 11, 2023 •

edited

rustbot commented Dec 11, 2023

aliemjay commented Dec 11, 2023

This comment has been minimized.

bors commented Dec 11, 2023

Nilstrieb Dec 11, 2023

aliemjay Dec 11, 2023

aliemjay Dec 15, 2023 •

edited

aliemjay Dec 17, 2023

bors commented Dec 11, 2023

This comment has been minimized.

rust-timer commented Dec 11, 2023

michaelwoerister commented Dec 14, 2023

aliemjay commented Dec 15, 2023

This comment has been minimized.

bors commented Dec 15, 2023

aliemjay commented Dec 15, 2023 •

edited

michaelwoerister commented Dec 15, 2023

bors commented Dec 15, 2023

This comment has been minimized.

rust-timer commented Dec 15, 2023

aliemjay commented Dec 17, 2023

compiler-errors Dec 21, 2023

compiler-errors Dec 21, 2023

compiler-errors commented Dec 21, 2023

bors commented Dec 21, 2023

bors commented Dec 21, 2023

rust-log-analyzer commented Dec 22, 2023

bors commented Dec 22, 2023

compiler-errors commented Dec 22, 2023

bors commented Dec 22, 2023

bors commented Dec 22, 2023

rust-timer commented Dec 22, 2023

use Vec for region constraints instead of BTreeMap #118824

use Vec for region constraints instead of BTreeMap #118824

Conversation

aliemjay commented Dec 11, 2023 • edited

rustbot commented Dec 11, 2023

aliemjay commented Dec 11, 2023

This comment has been minimized.

bors commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliemjay Dec 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 11, 2023

This comment has been minimized.

rust-timer commented Dec 11, 2023

Overall result: ✅ improvements - no action needed

michaelwoerister commented Dec 14, 2023

aliemjay commented Dec 15, 2023

This comment has been minimized.

bors commented Dec 15, 2023

aliemjay commented Dec 15, 2023 • edited

michaelwoerister commented Dec 15, 2023

bors commented Dec 15, 2023

This comment has been minimized.

rust-timer commented Dec 15, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

aliemjay commented Dec 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Dec 21, 2023

bors commented Dec 21, 2023

bors commented Dec 21, 2023

rust-log-analyzer commented Dec 22, 2023

bors commented Dec 22, 2023

compiler-errors commented Dec 22, 2023

bors commented Dec 22, 2023

bors commented Dec 22, 2023

rust-timer commented Dec 22, 2023

Overall result: ✅ improvements - no action needed

aliemjay commented Dec 11, 2023 •

edited

aliemjay Dec 15, 2023 •

edited

aliemjay commented Dec 15, 2023 •

edited