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

fix: reduceNat? match terms with free or meta variables #3139

Merged
merged 2 commits into from Jan 5, 2024

Conversation

joehendrix
Copy link
Contributor

This removes checks in Lean.Meta.reduceNat? that caused it to fail on terms it could handle because they contain meta variables in arguments. This lead to those operations being reduced using their equational definitions and slow performance on large patterns:

set_option profiler true
set_option profiler.threshold 1

def testMod (x:Nat) :=
  match x with
  | 128 % 1024 => true
  | _ => false
-- elaboration took 3.02ms

def testMul (x:Nat) :=
  match x with
  | 128 * 1 => true
  | _ => false
-- type checking took 11.1ms
-- compilation of testMul.match_1 took 313ms
-- compilation of testMul took 65.7ms
-- elaboration took 58.9ms

Performance is slower on testMul than testMod because whnf ends up evaluateing 128 * 1 using Peano arithmetic while 128 % 1024 is able to avoid that treatment since 128 < 1024.

@joehendrix joehendrix added the WIP This is work in progress, and only a PR for the sake of CI or sharing. No review yet. label Jan 4, 2024
@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 4, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

  • ❗ Mathlib CI will not be attempted unless you rebase your PR onto the 'nightly' branch. (2024-01-04 19:22:41)

@joehendrix joehendrix changed the title fix: reduceNat? match terms with free or meta variables. fix: reduceNat? match terms with free or meta variables Jan 4, 2024
@leodemoura
Copy link
Member

@joehendrix Thanks for catching this issue. Could you please add new tests using set_option maxHeartbeats <small-value> that fail without the fix?

@joehendrix
Copy link
Contributor Author

Yes, I validated on my machine, and reordered the commits too so hopefully the CI will report the first commit fails.

@joehendrix joehendrix removed the WIP This is work in progress, and only a PR for the sake of CI or sharing. No review yet. label Jan 4, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc January 4, 2024 19:41 Inactive
@joehendrix
Copy link
Contributor Author

joehendrix commented Jan 4, 2024

Oh, I'm not sure if it ran the tests after all, but the commit shows a failure. If there's something else that needs to be done with this, let me know.

@joehendrix
Copy link
Contributor Author

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit d0d3678.
There were significant changes against commit 7d90b05:

  Benchmark   Metric       Change
  =========================================
+ stdlib      wall-clock    -1.1% (-34.9 σ)

@leodemoura leodemoura added this pull request to the merge queue Jan 5, 2024
Merged via the queue into master with commit 9034937 Jan 5, 2024
8 checks passed
@semorrison
Copy link
Collaborator

Uh oh, this has caused a bunch of timeouts in Mathlib. I wish that we had tested Mathlib first before merging! (Per the bot's instructions above, this requires basing the Lean PR off nightly, or even better nightly-with-mathlib.)

joehendrix added a commit that referenced this pull request Jan 7, 2024
This removes checks in `Lean.Meta.reduceNat?` that caused it to fail on
terms it could handle because they contain meta variables in arguments.
This lead to those operations being reduced using their equational
definitions and slow performance on large patterns:

```
set_option profiler true
set_option profiler.threshold 1

def testMod (x:Nat) :=
  match x with
  | 128 % 1024 => true
  | _ => false
-- elaboration took 3.02ms

def testMul (x:Nat) :=
  match x with
  | 128 * 1 => true
  | _ => false
-- type checking took 11.1ms
-- compilation of testMul.match_1 took 313ms
-- compilation of testMul took 65.7ms
-- elaboration took 58.9ms
```

Performance is slower on `testMul` than `testMod` because `whnf` ends up
evaluateing `128 * 1` using Peano arithmetic while `128 % 1024` is able
to avoid that treatment since `128 < 1024`.
mathlib-bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Jan 8, 2024
This proof has been causing timeouts for both leanprover/lean4#3124 and leanprover/lean4#3139 (and its in-progress fixes), so I'm just fixing it pre-emptively here.

(Fix due to @joehendrix.)



Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Kha pushed a commit that referenced this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

5 participants