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

Stop doing expensive work in opt_suggest_box_span eagerly #123006

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

compiler-errors
Copy link
Member

This PR defers the can_coerce and predicate_must_hold_modulo_regions calls in opt_suggest_box_span, and instead defers it until error reporting. This requires pulling the logic out of note_obligation_cause_code and into coercion, where we have access to the trait solver.

This also allows us to remove TypeVariableOriginKind::OpaqueTypeInference.

@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Mar 24, 2024
@compiler-errors
Copy link
Member Author

r? compiler

Copy link
Member

@fee1-dead fee1-dead 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 a small nit

Comment on lines +590 to +591
// Does the expectation of the match define an RPIT?
// (e.g. we're in the tail of a function body)
Copy link
Member

@fee1-dead fee1-dead Mar 27, 2024

Choose a reason for hiding this comment

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

would probably make sense to document that the DefId return type is of the AliasTy for the expectation. A little bit unclear by the name. Maybe it should be renamed to something like alias_ty_id_from_rpit_expectation, which wouldn't need documentation on the return type I suppose.

@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 Mar 27, 2024
@compiler-errors
Copy link
Member Author

@bors r=fee1-dead

@bors
Copy link
Contributor

bors commented Mar 27, 2024

📌 Commit 864e1fb has been approved by fee1-dead

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 Mar 27, 2024
@bors
Copy link
Contributor

bors commented Mar 27, 2024

⌛ Testing commit 864e1fb with merge d5db7fb...

@bors
Copy link
Contributor

bors commented Mar 27, 2024

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing d5db7fb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 27, 2024
@bors bors merged commit d5db7fb into rust-lang:master Mar 27, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 27, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d5db7fb): comparison URL.

Overall result: ❌ regressions - 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.2% [0.2%, 0.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
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)
0.9% [0.4%, 1.3%] 5
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.6% [-0.8%, -0.5%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.5% [-0.8%, 1.3%] 7

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.8%] 7
Regressions ❌
(secondary)
2.6% [2.3%, 2.8%] 4
Improvements ✅
(primary)
-3.0% [-3.0%, -3.0%] 1
Improvements ✅
(secondary)
-2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) 0.2% [-3.0%, 0.8%] 8

Binary size

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

Bootstrap: 670.226s -> 670.379s (0.02%)
Artifact size: 315.70 MiB -> 315.75 MiB (0.01%)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 6, 2024
…, r=lcnr

Remove `TypeVariableOriginKind` and `ConstVariableOriginKind`

It's annoying to have to import `TypeVariableOriginKind` just to fill it with `MiscVariable` for almost every use. Every other usage other than `TypeParameterDefinition` wasn't even used -- I can see how it may have been useful once for debugging, but I do quite a lot of typeck debugging and I've never really needed it.

So let's just remove it, and keep around the only useful thing which is the `DefId` of the param for `var_for_def`.

This is based on rust-lang#123006, which removed the special use of `TypeVariableOriginKind::OpaqueInference`, which I'm pretty sure I was the one that added.

r? lcnr or re-roll to types
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2024
…r=lcnr

Remove `TypeVariableOriginKind` and `ConstVariableOriginKind`

It's annoying to have to import `TypeVariableOriginKind` just to fill it with `MiscVariable` for almost every use. Every other usage other than `TypeParameterDefinition` wasn't even used -- I can see how it may have been useful once for debugging, but I do quite a lot of typeck debugging and I've never really needed it.

So let's just remove it, and keep around the only useful thing which is the `DefId` of the param for `var_for_def`.

This is based on rust-lang#123006, which removed the special use of `TypeVariableOriginKind::OpaqueInference`, which I'm pretty sure I was the one that added.

r? lcnr or re-roll to types
fmease added a commit to fmease/rust that referenced this pull request Apr 15, 2024
…, r=lcnr

Remove `TypeVariableOriginKind` and `ConstVariableOriginKind`

It's annoying to have to import `TypeVariableOriginKind` just to fill it with `MiscVariable` for almost every use. Every other usage other than `TypeParameterDefinition` wasn't even used -- I can see how it may have been useful once for debugging, but I do quite a lot of typeck debugging and I've never really needed it.

So let's just remove it, and keep around the only useful thing which is the `DefId` of the param for `var_for_def`.

This is based on rust-lang#123006, which removed the special use of `TypeVariableOriginKind::OpaqueInference`, which I'm pretty sure I was the one that added.

r? lcnr or re-roll to types
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 16, 2024
Rollup merge of rust-lang#123016 - compiler-errors:no-type-var-origin, r=lcnr

Remove `TypeVariableOriginKind` and `ConstVariableOriginKind`

It's annoying to have to import `TypeVariableOriginKind` just to fill it with `MiscVariable` for almost every use. Every other usage other than `TypeParameterDefinition` wasn't even used -- I can see how it may have been useful once for debugging, but I do quite a lot of typeck debugging and I've never really needed it.

So let's just remove it, and keep around the only useful thing which is the `DefId` of the param for `var_for_def`.

This is based on rust-lang#123006, which removed the special use of `TypeVariableOriginKind::OpaqueInference`, which I'm pretty sure I was the one that added.

r? lcnr or re-roll to types
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.

6 participants