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

Make RPITITs simple cases work when using lower_impl_trait_in_trait_to_assoc_ty #108700

Merged
merged 13 commits into from
Mar 12, 2023

Conversation

spastorino
Copy link
Member

@spastorino spastorino commented Mar 3, 2023

r? @compiler-errors

It's probably best reviewed commit by commit.

@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 3, 2023
@spastorino spastorino force-pushed the new-rpitit-impl-side-2 branch 3 times, most recently from 6a02389 to 4f6a819 Compare March 3, 2023 18:57
@spastorino
Copy link
Member Author

#108672 landed, I've rebased this one on top of master and it's now ready.

compiler/rustc_hir_analysis/src/check/compare_impl_item.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/dead.rs Show resolved Hide resolved
tcx.mk_opaque(def_id, substs)
};
opaque_type_bounds(tcx, bounds_def_id, bounds, item_ty, *span)
}
Copy link
Member

Choose a reason for hiding this comment

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

This code feels a little tangled but I don't know how to disentangle it... 🤔

@@ -117,6 +117,13 @@ fn adt_sized_constraint(tcx: TyCtxt<'_>, def_id: DefId) -> &[Ty<'_>] {

/// See `ParamEnv` struct definition for details.
fn param_env(tcx: TyCtxt<'_>, def_id: DefId) -> ty::ParamEnv<'_> {
let def_id =
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not being fed? Cycles?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, all of the queries that I'm not feeding are because of cycles.
To be honest, I'm starting to think that feeding everything is ungreat :). Maybe just feeding opt_rpitit_info and inside each query deciding is better because in order to understand what a query does you just go to the query. With the feeding strategy, if you want to understand how generics_of works, there's code in the query and there's code in assoc.rs which feeds that query and makes it do something completely different.

compiler/rustc_ty_utils/src/assoc.rs Show resolved Hide resolved
compiler/rustc_ty_utils/src/assoc.rs Show resolved Hide resolved
compiler/rustc_ty_utils/src/assoc.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/check/compare_impl_item.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/astconv/mod.rs Outdated Show resolved Hide resolved
@spastorino
Copy link
Member Author

@compiler-errors all comments addressed, I guess some things could be tweaked further, just let me know.

compiler/rustc_hir_analysis/src/collect/type_of.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/collect/item_bounds.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/check/compare_impl_item.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/check/compare_impl_item.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/astconv/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_ty_utils/src/ty.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Mar 5, 2023

☔ The latest upstream changes (presumably #108351) made this pull request unmergeable. Please resolve the merge conflicts.

@spastorino spastorino force-pushed the new-rpitit-impl-side-2 branch 2 times, most recently from 55ee8e5 to a6ccedf Compare March 6, 2023 01:04
@spastorino
Copy link
Member Author

@compiler-errors is ready for another pass.

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

@spastorino
Copy link
Member Author

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Mar 6, 2023

📌 Commit 5daa01e 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2023
@spastorino
Copy link
Member Author

@bors rollup=always

only affects -Z flag

@compiler-errors
Copy link
Member

compiler-errors commented Mar 6, 2023

@bors rollup-

This makes a bunch of calls to opt_rpitit_info that are not gated behind the -Z flag (every single call to item bounds, param env, check_type_bounds, astconv'ing every opaque), let's keep this as rollup=maybe.

@matthiaskrgr
Copy link
Member

should be
@bors rollup=never
then imo, just in case, since not everyone's reading through every pr and the queue is quite big already 🙃

@bors
Copy link
Contributor

bors commented Mar 12, 2023

⌛ Testing commit 5daa01e with merge 9455a55...

@bors
Copy link
Contributor

bors commented Mar 12, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 12, 2023
@bors bors merged commit 9455a55 into rust-lang:master Mar 12, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 12, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9455a55): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.6% [0.2%, 1.2%] 93
Regressions ❌
(secondary)
1.1% [0.2%, 2.9%] 45
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.5% [-1.5%, -1.5%] 1
All ❌✅ (primary) 0.6% [0.2%, 1.2%] 93

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.9% [0.7%, 3.3%] 11
Regressions ❌
(secondary)
3.0% [1.6%, 5.7%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.4% [-3.4%, -3.4%] 1
All ❌✅ (primary) 1.9% [0.7%, 3.3%] 11

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)
2.1% [1.0%, 3.4%] 20
Regressions ❌
(secondary)
3.8% [2.4%, 6.0%] 15
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.1% [1.0%, 3.4%] 20

@rustbot rustbot added the perf-regression Performance regression. label Mar 12, 2023
@nnethercote
Copy link
Contributor

Lots of regressions here, and no prior CI perf run. @spastorino, was this expected?

@compiler-errors
Copy link
Member

Only thing I can think of is cache_on_disk_if for the opt_rpitit_info query, which probably doesn't need to be set -- seeing if that fixes the regressions in #109048.

@nnethercote
Copy link
Contributor

https://perf.rust-lang.org/detailed-query.html?commit=9455a5591b1435dfc9a88b2922d0dfc155d9614f&base_commit=542ed2bf72b232b245ece058fc11aebb1ca507d7&benchmark=bitmaps-3.1.0-check&scenario=incr-unchanged says there are 5920 additional incr_comp_encode_dep_graph queries, which makes sense because the regressions are all in incremental builds. You could run the "Local profile (diff)" Cachegrind command to double check.

@spastorino
Copy link
Member Author

Just seeing this comments, thanks for opening perf trying to remove cache_on_disk_if and yeah, I guess this doesn't need anything like that given also how simple this query is.

@spastorino
Copy link
Member Author

I'm not with the computer today and don't know exactly what drives execution of incr_comp_encode_dep_graph, I would need to investigate. But I do wonder if we shouldn't also mark this query as no_hash and eval_always given the nature and simplicity of this query.

@Mark-Simulacrum
Copy link
Member

With #109057 landed, I think we can mark this as triaged.

@Mark-Simulacrum Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Mar 14, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 14, 2023
…r-errors

Remove some direct calls to local_def_id_to_hir_id on diagnostics

Was playing with `tests/ui/impl-trait/in-trait/default-body-with-rpit.rs` and was able to remove some ICEs. Still getting ...

```
error[E0277]: `impl Future<Output = Foo::{opaque#0}>` is not a future
  --> tests/ui/impl-trait/in-trait/default-body-with-rpit.rs:10:28
   |
10 |     async fn baz(&self) -> impl Debug {
   |                            ^^^^^^^^^^ `impl Future<Output = Foo::{opaque#0}>` is not a future
   |
   = help: the trait `Future` is not implemented for `impl Future<Output = Foo::{opaque#0}>`
   = note: impl Future<Output = Foo::{opaque#0}> must be a future or must implement `IntoFuture` to be awaited
note: required by a bound in `Foo::{opaque#1}`
  --> tests/ui/impl-trait/in-trait/default-body-with-rpit.rs:10:28
   |
10 |     async fn baz(&self) -> impl Debug {
   |                            ^^^^^^^^^^ required by this bound in `Foo::{opaque#1}`

error[E0277]: the size for values of type `impl Future<Output = Foo::{opaque#0}>` cannot be known at compilation time
  --> tests/ui/impl-trait/in-trait/default-body-with-rpit.rs:10:28
   |
10 |     async fn baz(&self) -> impl Debug {
   |                            ^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `Sized` is not implemented for `impl Future<Output = Foo::{opaque#0}>`
note: required by a bound in `Foo::{opaque#1}`
  --> tests/ui/impl-trait/in-trait/default-body-with-rpit.rs:10:28
   |
10 |     async fn baz(&self) -> impl Debug {
   |                            ^^^^^^^^^^ required by this bound in `Foo::{opaque#1}`

error: internal compiler error: compiler/rustc_hir_typeck/src/closure.rs:724:18: async fn generator return type not an inference variable: Foo::{opaque#1}<'_>
  --> tests/ui/impl-trait/in-trait/default-body-with-rpit.rs:10:39
   |
10 |       async fn baz(&self) -> impl Debug {
   |  _______________________________________^
11 | |         ""
12 | |     }
   | |_____^
```

But I guess this is a little bit of progress anyway.

This one goes on top of rust-lang#108700 and rust-lang#108945
r? `@compiler-errors`
@nnethercote
Copy link
Contributor

nnethercote commented Mar 14, 2023

Yes, #109057 this looks to have fixed about 90-100% of the regression. Thank you, @compiler-errors.

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. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.

8 participants