Skip to content
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.

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

fast path for declared_generic_bounds_from_env #119084

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

aliemjay
Copy link
Member

~2% perf gain for diesel

@rustbot
Copy link
Collaborator

rustbot commented Dec 18, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@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 Dec 18, 2023
@aliemjay
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 18, 2023
@bors
Copy link
Contributor

bors commented Dec 18, 2023

⌛ Trying commit 20e222a with merge e0a7b59...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2023
fast path for declared_generic_bounds_from_env

~2% perf gain for diesel
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me after perf

Comment on lines 250 to 254
(ty::Alias(_, a1), GenericKind::Alias(a2)) if a1.def_id != a2.def_id => {
return None;
}
(ty::Param(p1), GenericKind::Param(p2)) if p1 != p2 => return None,
_ => {}
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
(ty::Alias(_, a1), GenericKind::Alias(a2)) if a1.def_id != a2.def_id => {
return None;
}
(ty::Param(p1), GenericKind::Param(p2)) if p1 != p2 => return None,
_ => {}
(ty::Alias(_, a1), GenericKind::Alias(a2)) if a1.def_id == a2.def_id =>{}
(ty::Param(p1), GenericKind::Param(p2)) if p1 == p2 => {}
_ => return None,

This should cover more cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is missing the case of GenericKind::Placeholder.

@bors
Copy link
Contributor

bors commented Dec 18, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e0a7b59): 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)
-1.4% [-2.2%, -0.2%] 8
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.4% [-2.2%, -0.2%] 8

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.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
1.3% [1.3%, 1.4%] 2
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.0% [-0.5%, 0.5%] 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.6% [0.4%, 0.7%] 4
Regressions ❌
(secondary)
2.1% [1.6%, 2.5%] 5
Improvements ✅
(primary)
-1.3% [-1.5%, -1.1%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.1% [-1.5%, 0.7%] 6

Binary size

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

Bootstrap: 673.256s -> 671.287s (-0.29%)
Artifact size: 312.47 MiB -> 312.47 MiB (0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 18, 2023
@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot 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 Dec 19, 2023
@aliemjay
Copy link
Member Author

aliemjay commented Dec 19, 2023

Added a comment about rigid alias types and implemented an alternative approach

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 19, 2023
@compiler-errors
Copy link
Member

r=me

@compiler-errors compiler-errors 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 Dec 19, 2023
@aliemjay

This comment was marked as duplicate.

@aliemjay
Copy link
Member Author

@bors r=compiler-errors rollup=never

@bors
Copy link
Contributor

bors commented Dec 19, 2023

📌 Commit 27e964d has been approved by compiler-errors

It is now in the queue for this repository.

@bors 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 Dec 19, 2023
@bors
Copy link
Contributor

bors commented Dec 19, 2023

⌛ Testing commit 27e964d with merge 57ad505...

@bors
Copy link
Contributor

bors commented Dec 19, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2023
@bors bors merged commit 57ad505 into rust-lang:master Dec 19, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 19, 2023
@nnethercote
Copy link
Contributor

@aliemjay: how did you find this improvement? Profiling, looking through the code, something else? I'm curious :)

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (57ad505): 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)
-1.9% [-2.9%, -0.2%] 8
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.9% [-2.9%, -0.2%] 8

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)
3.5% [3.5%, 3.5%] 1
Improvements ✅
(primary)
-0.6% [-0.6%, -0.5%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.6% [-0.6%, -0.5%] 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.7% [-1.8%, -1.5%] 4
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.7% [-1.8%, -1.5%] 4

Binary size

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

Bootstrap: 673.86s -> 671.988s (-0.28%)
Artifact size: 312.43 MiB -> 312.43 MiB (0.00%)

@aliemjay
Copy link
Member Author

@aliemjay: how did you find this improvement? Profiling, looking through the code, something else? I'm curious :)

by callgrind profiles, looking at code that I have some expertise in so that I can easily catch optimazation opportunities.

@aliemjay aliemjay deleted the perf-env-bounds branch March 30, 2024 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants