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 with_reducible in special-purpose decreasing_trivial macros #3991

Merged
merged 6 commits into from
Apr 29, 2024

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Apr 25, 2024

Because of the last-added-tried-first rule for macros, all the special
purpose decreasing_trivial rules are tried for most recursive
definitions out there, and because they use apply and assumption
with default transparency may cause some definitoins to be unfolded over
and over again.

A quick test with one of the functions in the leansat project shows that
elaboration time goes down from 600ms to 375ms when using

decreasing_by all_goals decreasing_with with_reducible decreasing_trivial

instead of

decreasing_by all_goals decreasing_with decreasing_trivial

This change uses with_reducible in most of these macros.

This means that these tactics will no longer work when the
relations/definitions they look for is hidden behind a definition.
This affected in particular Array.sizeOf_get, which now has a companion sizeOf_getElem.

In addition, there were three tactics using apply to apply Nat-related lemmas
that we now expect omega to solve. We still need them when building Init modules
that don’t have access to omega, but they now live in decreasing_trivial_pre_omega,
meant to be only used internally.

nomeata added 4 commits April 25, 2024 12:01
Because of the last-added-tried-first rule for macros, all the special
purpose `decreasing_trivial` rules are tried for most recursive
definitions out there, and because they use `apply` and `assumption`
with default transparency may cause some definitoins to be unfolded over
and over again.

A quick test with one of the funcitons in the leansat project shows that
elaboration time goes down from 600ms to 375ms when using
```
decreasing_by all_goals decreasing_with with_reducible decreasing_trivial
```
instead of
```
decreasing_by all_goals decreasing_with decreasing_trivial
```

This change uses `with_reducible` in most of these macros.

This means that these tactics will no longer work when the
relations/definitions they look for is hidden behind a definition.
@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 Apr 25, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Apr 25, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Apr 25, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Apr 25, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Apr 25, 2024

Mathlib CI status (docs):

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc April 25, 2024 11:52 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Apr 25, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Apr 25, 2024
@nomeata
Copy link
Collaborator Author

nomeata commented Apr 25, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit c97e21f.
There were significant changes against commit 62cdb51:

  Benchmark   Metric             Change
  =================================================
+ stdlib      instructions        -1.9%  (-704.2 σ)
+ stdlib      tactic execution   -55.0% (-9162.0 σ)
- stdlib      type checking        2.9%    (12.0 σ)

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added builds-mathlib CI has verified that Mathlib builds against this PR and removed breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan labels Apr 25, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc April 25, 2024 13:13 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Apr 25, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Apr 25, 2024
@nomeata
Copy link
Collaborator Author

nomeata commented Apr 25, 2024

@nomeata nomeata marked this pull request as ready for review April 25, 2024 13:59
@nomeata nomeata added the will-merge-soon …unless someone speaks up label Apr 26, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc April 26, 2024 13:19 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Apr 26, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Apr 26, 2024
@nomeata nomeata added this pull request to the merge queue Apr 29, 2024
Merged via the queue into master with commit e2983e4 Apr 29, 2024
28 checks passed
@nomeata nomeata deleted the joachim/with_reducible_decreasing branch April 29, 2024 17:07
github-merge-queue bot pushed a commit that referenced this pull request Aug 14, 2024
Using `Nat.lt_trans` is too restrictive, and using `Nat.lt_of_lt_of_le`
should make this tactic prove more goals.

This fixes a regression probably introduced by #3991; at least in some
cases before that `apply sizeOf_get` would have solved the goal here.
And it’s true that this is now subsumed by `simp`, but because of the
order that `macro_rules` are tried, the too restrictive variant with
`Nat.lt_trans` would be tried before `simp`, without backtracking.

Fixes #5027
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 will-merge-soon …unless someone speaks up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants