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

Use DeepRejectCtxt to quickly reject ParamEnv candidates #128776

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

Bryanskiy
Copy link
Contributor

The description is on the zulip thread

r? @lcnr

@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Aug 7, 2024
{
return Err(NoSolution);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

please also add this for NormalizesTo goals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pub fn types_may_unify(self, lhs: I::Ty, rhs: I::Ty) -> bool {
// Non rigid types may unify with each other.
if !lhs.is_known_rigid() && !rhs.is_known_rigid() {
return true;
}
Copy link
Contributor

@lcnr lcnr Aug 7, 2024

Choose a reason for hiding this comment

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

this means we never reject Param(T) == Param(U)

when treating params as rigid, compare their index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

and also in project::assemble_candidates_from_predicates to also use a fast-path when normalizing in the old solver

match ty.kind() {
ty::Param(p) => match (self.treat_lhs_params, self.treat_rhs_params) {
(TreatParams::AsRigid, TreatParams::AsRigid) => param == p,
_ => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

please exhaustively match here

(ty::Placeholder(lhs), ty::Placeholder(rhs)) => lhs == rhs,
(ty::Placeholder(_), _) | (_, ty::Placeholder(_)) => false,

_ => false,
Copy link
Contributor

Choose a reason for hiding this comment

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

this function has to be an overapproximation, so incorrectly returning false would result in subtle bugs. Please exhaustively match here: (ty::Whatever | ty::Whatever | ...., _) | (_, ty::whatever | .....)

Comment on lines 235 to 237
(TreatParams::InstantiateWithInfer, TreatParams::AsRigid)
| (TreatParams::AsRigid, TreatParams::InstantiateWithInfer)
| (TreatParams::InstantiateWithInfer, TreatParams::InstantiateWithInfer) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(TreatParams::InstantiateWithInfer, TreatParams::AsRigid)
| (TreatParams::AsRigid, TreatParams::InstantiateWithInfer)
| (TreatParams::InstantiateWithInfer, TreatParams::InstantiateWithInfer) => {
(TreatParams::InstantiateWithInfer, _)
| (_, TreatParams::InstantiateWithInfer) => {

small style preference

},
ty::Pat(obl_ty, _) => {
}
(ty::Param(_), ty::Placeholder(_)) | (ty::Placeholder(_), ty::Param(_)) => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

not really relevant as it doesnt ever happen on stable, but ty::Param and placeholder can't unify when treating params as rigid, so you can remove this branch and just use the self.treat_lhs_params == TreatParams::InstantiateWithInfer branch

@lcnr
Copy link
Contributor

lcnr commented Aug 9, 2024

@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 Aug 9, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 9, 2024
@bors
Copy link
Contributor

bors commented Aug 9, 2024

⌛ Trying commit 77fc853 with merge 7a0b60e...

@bors
Copy link
Contributor

bors commented Aug 9, 2024

☀️ Try build successful - checks-actions
Build commit: 7a0b60e (7a0b60e0de7dfef4ed18ad00b864a144909632d3)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7a0b60e): 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)
6.3% [0.2%, 12.7%] 18
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.8% [-4.2%, -0.3%] 15
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.6% [-4.2%, 12.7%] 33

Max RSS (memory usage)

Results (primary -3.7%, secondary 2.9%)

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

Cycles

Results (primary 2.3%)

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)
4.5% [3.3%, 7.4%] 13
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.8% [-2.5%, -1.4%] 7
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.3% [-2.5%, 7.4%] 20

Binary size

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

Bootstrap: 759.636s -> 760.059s (0.06%)
Artifact size: 337.10 MiB -> 337.13 MiB (0.01%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 9, 2024
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

2 ideas for perf

  • move all (Rigid, _) | (_, Rigid) => false into a single branch at the end

and if that doesn't fix the regressions make the deep reject ctxt instead be generic over TreatParams 🤔

@Bryanskiy
Copy link
Contributor Author

A small statistical observation:

bitmaps-3.1.0 benchmark (12.70%):

  • successful rejects in assemble_candidates_from_caller_bounds: 10/58 (~17%)
  • successful rejects in project: 1/1032 (~0.1%)

typenum-1.17.0 benchmark (9.59%):

  • successful rejects in assemble_candidates_from_caller_bounds: 7286/12526 (~58%)
  • successful rejects in project: 115/518 (~22%)

diesel-1.4.8 benchmark (-4.17%):

  • successful rejects in assemble_candidates_from_caller_bounds: 1405482/1558566 (~90%)
  • successful rejects in project: 159641/168826 (~95%)

std:

  • successful rejects in assemble_candidates_from_caller_bounds: 212125/285199 (~74%)
  • successful rejects in project: 5754/25544 (~22%)

We waste time more often in project and we can try to remove DeepRejectCtxt from there.

@lcnr
Copy link
Contributor

lcnr commented Aug 12, 2024

the impact on diesel seems quite desirable however. I think we can improve the performance of fast reject significantly so that even the ~20% hit rate from typenum is a perf improvement.

@lcnr
Copy link
Contributor

lcnr commented Aug 12, 2024

@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 Aug 12, 2024
@bors
Copy link
Contributor

bors commented Aug 12, 2024

⌛ Trying commit ea66e5b with merge bb463aa...

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 2, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
@bors
Copy link
Contributor

bors commented Sep 3, 2024

⌛ Testing commit c51953f with merge 06ab993...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] miri test:false 4.428
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Tue, Sep  3, 2024 12:17:37 PM
  network time: Tue, 03 Sep 2024 12:17:37 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@bors
Copy link
Contributor

bors commented Sep 3, 2024

💔 Test failed - checks-actions

@bors 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 Sep 3, 2024
@lqd
Copy link
Member

lqd commented Sep 3, 2024

@bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 3, 2024
@bors
Copy link
Contributor

bors commented Sep 3, 2024

⌛ Testing commit c51953f with merge c190066...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] miri test:false 4.501
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Tue, Sep  3, 2024 10:13:31 PM
  network time: Tue, 03 Sep 2024 22:13:31 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@bors
Copy link
Contributor

bors commented Sep 3, 2024

💔 Test failed - checks-actions

@bors 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 Sep 3, 2024
@lcnr
Copy link
Contributor

lcnr commented Sep 4, 2024

failed to remove file C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe
2024-09-03T22:13:31.6487555Z
2024-09-03T22:13:31.6487659Z Caused by:
2024-09-03T22:13:31.6487920Z Access is denied. (os error 5)

I think that's also a spurious error? cc @rust-lang/infra

@Kobzol
Copy link
Contributor

Kobzol commented Sep 4, 2024

Sadly, yes.

@bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2024
@bors
Copy link
Contributor

bors commented Sep 6, 2024

⌛ Testing commit c51953f with merge 26b5599...

@bors
Copy link
Contributor

bors commented Sep 6, 2024

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 26b5599 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 6, 2024
@bors bors merged commit 26b5599 into rust-lang:master Sep 6, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 6, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (26b5599): 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.8% [-4.7%, -0.3%] 17
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.8% [-4.7%, -0.3%] 17

Max RSS (memory usage)

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

Cycles

Results (primary -1.8%)

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.8% [-2.2%, -1.5%] 7
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.8% [-2.2%, -1.5%] 7

Binary size

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

Bootstrap: 754.723s -> 756.217s (0.20%)
Artifact size: 341.14 MiB -> 341.14 MiB (0.00%)

github-merge-queue bot pushed a commit to model-checking/kani that referenced this pull request Sep 8, 2024
Relevant upstream PR:

rust-lang/rust#128776: The `TreatParams` enum
variants were renamed.

Resolves #3503 

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants