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

Track perf regression "Branch Clause from Predicate #104846" #105060

Open
2 tasks
spastorino opened this issue Nov 29, 2022 · 6 comments
Open
2 tasks

Track perf regression "Branch Clause from Predicate #104846" #105060

spastorino opened this issue Nov 29, 2022 · 6 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.

Comments

@spastorino
Copy link
Member

spastorino commented Nov 29, 2022

A perf regression was introduced by #104846, #104846 (comment)

Right now we are moving (moderately large) Clauses around. What we often do in the compiler is to wrap such types in a newtype and an Interned (see ty::Ty, ty::Predicate and ty::Const) for examples. So what we could do is do the same thing with Clause. It does mean every time we match on it we need to add and call something like the kind method on Ty and Predicate to get at the inner Clause. There's also some naming issues, as you can't have both the interned type and the inner type be called ty::Clause. One solution could be to publicly reexport all Clause variants from the clause module, so we can write clause::Trait instead of Clause::Trait, and then rename the current Clause enum to ClauseKind. After that the Clause name is free again for use by the wrapper type.

each of these steps could be a separate PR, but should at least be a separate commit for easy review and rebases.

The plan again in a more structured manner (where each step is a commit):

  • (E-easy) reexport Clause's variants from the clause module and change all uses of Clause::Foo to clause::Foo and import the clause module where necessary.
  • (E-medium) rename Clause to ClauseKind and add the wrapper type called Clause, add Clause to the direct_interners! macro invocation, use mk_clause at all sites that create Clauses, add a kind method to the new Clause type, call kind at each site that matches on ClauseKind variants.
@spastorino
Copy link
Member Author

r? @spastorino

cc @oli-obk

@sladyn98

This comment was marked as resolved.

@oli-obk

This comment was marked as resolved.

@sladyn98

This comment was marked as outdated.

@oli-obk oli-obk added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Feb 9, 2023
@lcnr
Copy link
Contributor

lcnr commented Apr 13, 2023

reexport Clause's variants from the clause module and change all uses of Clause::Foo to clause::Foo and import the clause module where necessary.

I disagree with this step. We don't do that for constants or predicates and I believe that doing it for regions and types mostly happens due to historical reasons

@oli-obk
Copy link
Contributor

oli-obk commented Apr 13, 2023

I disagree with this step. We don't do that for constants or predicates and I believe that doing it for regions and types mostly happens due to historical reasons

I mostly dislike using ClauseKind everywhere. Since the interned clause type won't show up a lot, we could also just call it InternedClause and keep Clause as the name for the enum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants