Skip to content

Fast-path some relations via strict equality #104598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fast-path some relations via strict equality #104598

wants to merge 2 commits into from

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Nov 19, 2022 •
edited
Loading

Some relations can take advantage of strict equality (that is, the == operator), which is far cheaper to check in some cases like substs and existential predicate lists.

This also fixes #104583.

r? @ghost

@rustbot 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 19, 2022
@compiler-errors
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 19, 2022
@bors
Copy link
Collaborator

bors commented Nov 19, 2022

⌛ Trying commit 3a5bf11 with merge bf7cfa7141d188772c184387e27cc0505b3dcc5c...

@bors
Copy link
Collaborator

bors commented Nov 19, 2022

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bf7cfa7141d188772c184387e27cc0505b3dcc5c): 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-review -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.3% [-0.4%, -0.3%] 5
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.3% [-0.4%, -0.3%] 5

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)		 1.2% [1.2%, 1.2%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -4.0% [-5.0%, -3.0%] 2
All ❌✅ (primary) - - 0

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 19, 2022
matthiaskrgr
matthiaskrgr reviewed Nov 19, 2022
View reviewed changes
compiler/rustc_hir_analysis/src/check/dropck.rs Outdated
Comment on lines 250 to 254

fn fast_equate_combine(&self) -> bool {
true
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these be marked #[inline]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps that would help... 🤔

@compiler-errors
Copy link
Member Author

I'll try one more perf run, but unclear that this is worth the complexity...

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 19, 2022
@bors
Copy link
Collaborator

bors commented Nov 19, 2022

⌛ Trying commit d1ed286 with merge aff588effced4b8915a1406ff5b4271169c1181a...

@bors
Copy link
Collaborator

bors commented Nov 19, 2022

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (aff588effced4b8915a1406ff5b4271169c1181a): 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-review -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
Improvements ✅
(secondary)		 -1.3% [-1.3%, -1.3%] 2
All ❌✅ (primary) - - 0

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)		 1.1% [1.1%, 1.1%] 1
Regressions ❌
(secondary)		 3.2% [3.2%, 3.2%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.1% [1.1%, 1.1%] 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)		 5.8% [5.4%, 6.2%] 2
Improvements ✅
(primary)		 -1.5% [-2.1%, -0.7%] 17
Improvements ✅
(secondary)		 -3.0% [-4.1%, -2.2%] 18
All ❌✅ (primary) -1.5% [-2.1%, -0.7%] 17

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 19, 2022
faptc
faptc reviewed Nov 20, 2022
View reviewed changes
compiler/rustc_middle/src/ty/relate.rs
@@ -29,7 +29,7 @@ pub trait TypeRelation<'tcx>: Sized {
fn tag(&self) -> &'static str;

/// Returns whether or not structural equality can be used to fast-path this relation
fn fast_equate_combine(&self) -> bool;
fn fast_equate(&self) -> bool;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotate inline this declaration instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[inline] means nothing on trait methods without a body.

@jackh726
Copy link
Member

Is this confirmed to fix #104583? I don't think the perf is enough of a win to justify to added complexity otherwise. If we want to land this, I think every one of the trues need to have a comment explaining why the can be true.

Also, can just make a default method with false, and only add an impl method for true.

@compiler-errors
Copy link
Member Author

Is this confirmed to fix #104583?

Yes, though I can probably reformulate this to be simpler, given that we don't have a perf win in general. Let me close this and go back to the drawing board.

@compiler-errors compiler-errors mentioned this pull request Dec 6, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 13, 2022
@bors
Auto merge of rust-lang#105350 - compiler-errors:faster-binder-relate…
109cccb 
…, r=oli-obk

Fast-path some binder relations

A simpler approach than rust-lang#104598

Fixes rust-lang#104583

r? types
@compiler-errors compiler-errors deleted the fast-path-relate branch August 11, 2023 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiaskrgr matthiaskrgr matthiaskrgr left review comments

@faptc faptc faptc left review comments

Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exponential compile time with nested &dyn Fn type
7 participants
@compiler-errors @rust-timer @bors @jackh726 @matthiaskrgr @faptc @rustbot