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

feat: Nat.decidableBallLT handles large numbers #6651

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

llllvvuu
Copy link
Contributor

@llllvvuu llllvvuu commented Jan 15, 2025

This PR enables the following:

example : ∀ a < 28000, a ^ 27 := by decide

where currently, the following already causes "maximum recursion depth has been reached":

example : ∀ a < 200, a ^ 27 := by decide

This PR also prevents a stack overflow in:

example : ∀ a < 10 ^ 7, a ^ 27 := by native_decide

@llllvvuu llllvvuu requested a review from kim-em as a code owner January 15, 2025 08:01
@llllvvuu llllvvuu force-pushed the feat/natdecidableballlt branch from 727214a to d53c45e Compare January 15, 2025 08:09
@github-actions github-actions bot added changelog-library Library awaiting-review Waiting for someone to review the PR labels Jan 15, 2025
@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 Jan 15, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jan 15, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 15, 2025
@leanprover-community-bot leanprover-community-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Jan 15, 2025
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Jan 15, 2025

Mathlib CI status (docs):

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jan 16, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 16, 2025
@llllvvuu llllvvuu force-pushed the feat/natdecidableballlt branch from d39c120 to 1607174 Compare January 16, 2025 02:18
@kim-em
Copy link
Collaborator

kim-em commented Jan 16, 2025

How far does it get after these changes?

@kim-em
Copy link
Collaborator

kim-em commented Jan 16, 2025

!bench

@llllvvuu
Copy link
Contributor Author

How far does it get after these changes?

This seems to be close to the limit:

set_option maxRecDepth 512 in
set_option maxHeartbeats 130000 in
example : ∀ a < 28000, a ^ 2 ≠ 7 := by decide

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 1607174.
There were significant changes against commit e9bd980:

  Benchmark                   Metric                 Change
  ==================================================================
- big_omega.lean              branch-misses            4.0% (29.1 σ)
- big_omega.lean MT           branch-misses            6.3% (27.7 σ)
- bv_decide_inequality.lean   branch-misses            1.2% (16.8 σ)
- bv_decide_realworld         branch-misses            1.4% (11.4 σ)
- simp_arith1                 branch-misses            2.7% (28.8 σ)
- stdlib                      fix level params         1.5% (30.7 σ)
- stdlib                      instantiate metavars     6.4% (25.6 σ)

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jan 16, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 16, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jan 16, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 16, 2025
@kim-em
Copy link
Collaborator

kim-em commented Jan 16, 2025

The benchmark results are not encouraging. Would you mind opening a Mathlib PR (using the lean-pr-testing-6651 branch, and targeting the nightly-testing-2025-01-15 branch --- or whatever was the base of lean-pr-testing-6651, you may need to check), and then running !bench on that?

@llllvvuu
Copy link
Contributor Author

The benchmark results are not encouraging. Would you mind opening a Mathlib PR (using the lean-pr-testing-6651 branch, and targeting the nightly-testing-2025-01-15 branch --- or whatever was the base of lean-pr-testing-6651, you may need to check), and then running !bench on that?

leanprover-community/mathlib4#20799 (comment)

@kim-em
Copy link
Collaborator

kim-em commented Jan 17, 2025

Wow, is that for real?! I'm confused how a 10% wall-clock improvement is possible while not touching the instruction count.

@nomeata
Copy link
Collaborator

nomeata commented Jan 17, 2025

Looks like a fluke to me, I usually don't trust wall-time measurements.

@llllvvuu
Copy link
Contributor Author

I think it's more that the base commit was 10% slower than usual for some reason (some kind of noise). I'm not sure how to re-run the base commit.

@llllvvuu
Copy link
Contributor Author

llllvvuu commented Jan 17, 2025

IIUC none of the benchmarks flagged in #6651 (comment) use this instance

FWIW performance-wise there is a slight heartbeats overhead (something like 20) for values of n that previously ran, in exchange for supporting more values of n

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review Waiting for someone to review the PR builds-mathlib CI has verified that Mathlib builds against this PR changelog-library Library 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.

5 participants