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

perf: use NatPow Int instead of HPow Int Nat Int #4903

Merged
merged 2 commits into from
Aug 3, 2024
Merged

Conversation

leodemoura
Copy link
Member

This modification improves the performance of the example in issue #4861. It no longer times out but is still expensive.

Here is the analysis of the performance issue: Given (x : Int), to elaborate x ^ 1, a few default instances have to be tried.

First, the homogeneous instance is tried and fails since Int does not implement Pow Int. Then, the NatPow instance is tried, and it also fails. The same process is performed for each term of the form p ^ 1. There are seveal of them at #4861. After all of these fail, the lower priority default instance for numerals is tried, and x ^ 1 becomes x ^ (1 : Nat). Then, HPow Int Nat Int can be applied, and the elaboration succeeds. However, this process has to be repeated for every single term of the form p ^ 1. The elaborator tries all homogeneous HPow and NatPow instances for all p ^ 1 terms before trying the lower priority default instance OfNat.

This commit ensures Int has a NatPow instance instead of HPow Int Nat Int. This change shortcuts the process, but it still first tries the homogeneous HPow instance, fails, and then tries NatPow. The elaboration can be made much more efficient by writing p ^ (1 : Nat).

This modification improves the performance of the example in issue #4861.
It no longer times out but is still expensive.

Here is the analysis of the performance issue: Given `(x : Int)`, to elaborate `x ^ 1`, a few default instances have to be tried.

First, the homogeneous instance is tried and fails since `Int` does
not implement `Pow Int`. Then, the `NatPow` instance is tried, and it
also fails. The same process is performed for each term of the form `p
^ 1`. There are seveal of them at #4861. After all of these fail,
the lower priority default instance for numerals is tried, and
`x ^ 1` becomes `x ^ (1 : Nat)`. Then, `HPow Int Nat Int` can be
applied, and the elaboration succeeds. However, this process has
to be repeated for every single term of the form `p ^ 1`.
The elaborator tries all homogeneous `HPow` and `NatPow` instances
for all `p ^ 1` terms before trying the lower priority default instance `OfNat`.

This commit ensures `Int` has a `NatPow` instance instead of `HPow Int
Nat Int`. This change shortcuts the process, but it still first
tries the homogeneous `HPow` instance, fails, and then tries
`NatPow`. The elaboration can be made much more efficient by writing `p ^ (1 : Nat)`.
@leodemoura leodemoura requested a review from kim-em as a code owner August 2, 2024 19:58
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Aug 2, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Aug 2, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 2, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 2, 2024 20:46 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Aug 2, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 2, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Aug 2, 2024

Mathlib CI status (docs):

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Aug 2, 2024
@leodemoura leodemoura added this pull request to the merge queue Aug 3, 2024
Merged via the queue into master with commit 647a5e9 Aug 3, 2024
16 checks passed
@kmill
Copy link
Collaborator

kmill commented Aug 4, 2024

This seems to have been an oversight in #2778, and it should have become a NatPow Int instance back then.

Mathlib adds a default Pow instance for monoids, which is used instead of this HPow Int Nat Int instance. I suppose that hid the performance issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds-mathlib CI has verified that Mathlib builds against this PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants