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.

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

Various refactorings of the TAIT infrastructure #87587

Merged
merged 13 commits into from
Aug 11, 2021
Merged

Various refactorings of the TAIT infrastructure #87587

merged 13 commits into from
Aug 11, 2021

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jul 29, 2021 •
edited
Loading

Before this PR we used to store the opaque type knowledge outside the InferCtxt, so it got recomputed on every opaque type instantiation.

I also removed a feature gate check that makes no sense in the planned lazy TAIT resolution scheme

Each commit passes all tests, so this PR is best reviewed commit by commit.

r? @spastorino

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2021
spastorino
spastorino reviewed Aug 3, 2021
View reviewed changes
src/test/ui/type-alias-impl-trait/issue-60371.stderr
|
= note: see issue #63063 <https://github.com/rust-lang/rust/issues/63063> for more information
= help: add `#![feature(type_alias_impl_trait)]` to the crate attributes to enable

Copy link
Member

Choose a reason for hiding this comment

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

Is it ok to remove this error report? will we need to add it back again with the new scheme? do we need to track this somewhere?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something that will be allowed under the new scheme

oli-obk added 11 commits August 6, 2021 10:12
@oli-obk
Move opaque type cache into InferCtxt
d998059
@oli-obk
Remove a field that is computed later anyway
14021fe
@oli-obk
Move some code around in preparation of splitting a function
3418280
@oli-obk
Split fold_opaque_ty
a30b548
@oli-obk
Remove a useless feature gateing
c091b5c 
With the planned lazy TAIT system, this will not really make sense anymore anyway.
@oli-obk
Remove Option only used as its Some variant
816b9fc
@oli-obk
Immediately register new opaque types in the global list.
20371b9 
Previously each opaque type instantiation would create new inference vars, even for the same opaque type/substs combination. Now there is a global map in InferCtxt that gets filled whenever we encounter an opaque type.
@oli-obk
Store the DefId of the currently typechecked item in InferCtxt
b2c1919 
This allows opaque type inference to check for defining uses without having to pass down that def id via function arguments to every method that could possibly cause an opaque type to be compared with a concrete type
@oli-obk
Point to the value instead of the TAIT declaration for obligation fai…
092e9cc 
…lures
@oli-obk
Use existing type alias instead of manually writing it
1b9ad13
@oli-obk
Document with_opaque_type_inference's use cases.
238d974
@spastorino
Copy link
Member

@bors r+ rollup=always

@bors
Copy link
Contributor

bors commented Aug 6, 2021

📌 Commit 7af445d has been approved by spastorino

@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 Aug 6, 2021
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 6, 2021

@bors r- forgot to do a perf run first

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 6, 2021
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 6, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

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

bors commented Aug 6, 2021

⌛ Trying commit 7af445d with merge 2b20ba7aedf3287b7b8171fcfda96eec1f5d0427...

@bors
Copy link
Contributor

bors commented Aug 6, 2021

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

@bors
Copy link
Contributor

bors commented Aug 10, 2021

💥 Test timed out

@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 Aug 10, 2021
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 10, 2021

@bors r=spastorino rollup=never

@bors
Copy link
Contributor

bors commented Aug 10, 2021

📌 Commit 93c4aa8 has been approved by spastorino

@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 Aug 10, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Aug 10, 2021

⌛ Testing commit 93c4aa8 with merge ac109101591bef66e317fada816b2d8b548385e3...

@bors
Copy link
Contributor

bors commented Aug 10, 2021

💔 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 Aug 10, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@ehuss
Copy link
Contributor

ehuss commented Aug 10, 2021

@bors retry

macos runner exited without logs after 1h 47m 35s.

@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 Aug 10, 2021
@bors
Copy link
Contributor

bors commented Aug 11, 2021

⌛ Testing commit 93c4aa8 with merge d488de8...

@bors
Copy link
Contributor

bors commented Aug 11, 2021

☀️ Test successful - checks-actions
Approved by: spastorino
Pushing d488de8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 11, 2021
@bors bors merged commit d488de8 into rust-lang:master Aug 11, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 11, 2021
@oli-obk oli-obk deleted the lazy_tait branch August 11, 2021 11:38
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 11, 2021

The perf regression here is expected and I am keeping an eye on it. I have ideas how to resolve it once my tait refactoring is done

@rylev
Copy link
Member

rylev commented Aug 17, 2021

@oli-obk would it be worth it to track the perf regression in an issue? Then we can mark this as being addressed by adding the perf-regression-triaged labeled.

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 17, 2021

We could add it as a bullet point to the steps of #63063

@Mark-Simulacrum
Copy link
Member

I'm going to actually go ahead and mark this regression as triaged. I did some investigation into where the increased instruction counts are coming from, and it looks like it's pretty much entirely down to more obligation processing. Even if we believe there's a possible workaround (e.g., the resolution @oli-obk suggests), I don't think it's useful to keep this regression marked as untriaged in the interim.

The regression fixes here are likely to come about as more general work -- this PR did not introduce some "particularly slow" code or anything like that.

@rustbot label +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 19, 2021
@semarie semarie mentioned this pull request Oct 25, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spastorino spastorino spastorino left review comments

@klensy klensy klensy left review comments

Assignees

@spastorino spastorino

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.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.