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

refactor: omega: avoid MVar machinery #5991

Merged
merged 3 commits into from
Nov 13, 2024
Merged

refactor: omega: avoid MVar machinery #5991

merged 3 commits into from
Nov 13, 2024

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Nov 7, 2024

This PR simplifies the implementation of omega.

When constructing the proof, omega is using MVars only for the purpose of doing case analysis on Or. We can simplify the implementation a fair bit if we just produce the proof directly using Or.elim.

While it didn’t yield the performance benefits I was hoping for, this still seems a worthwhile simplification, now that we already have it.

@TwoFX mentioned that in his work on weight-balanced trees he has some
`omega` proofs that take very long (16s), had many disjunctions, and
already traced it down to `instantiateMVars`. So I got curious and had a
look how omega is using the `MVar` machinery, and it is only using it to
do case analysis on `Or`.

It wasn't hard to rewrite this code to to construct the proof object
directly, without using `MVars`, using `Or.elim`, and indeed it brings
down the processing time of that particular proof from 16s to just below
one second. And we even get a net reduction of the line count!
@nomeata nomeata requested a review from kim-em as a code owner November 7, 2024 11:29
@nomeata
Copy link
Collaborator Author

nomeata commented Nov 7, 2024

!bench

@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 Nov 7, 2024
@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit be2bd5c.
There were significant changes against commit c779f3a:

  Benchmark           Metric                 Change
  =============================================================
+ big_omega.lean      branch-misses           -5.2%   (-21.2 σ)
+ big_omega.lean      branches                -9.1%  (-829.7 σ)
+ big_omega.lean      instructions           -10.4% (-1068.9 σ)
+ big_omega.lean MT   branches               -11.4%  (-528.6 σ)
+ big_omega.lean MT   instructions           -10.9%  (-666.8 σ)
- nat_repr            task-clock               8.0%    (11.6 σ)
- nat_repr            wall-clock               7.9%    (12.3 σ)
- stdlib              instantiate metavars    12.1%    (17.7 σ)
- stdlib              share common exprs       1.7%    (10.9 σ)

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

nomeata commented Nov 7, 2024

Mixed benchmark results, it seems. In particular instantiate metavars to show up more is strange.

@leanprover-community-bot leanprover-community-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Nov 7, 2024
@nomeata
Copy link
Collaborator Author

nomeata commented Nov 7, 2024

Especially the nat_repr benchmark showing up is strange, I did not expect any omega in

~/build/lean/lean4 $ cat ./tests/bench/nat_repr.lean
def main : List String → IO Unit
| [n] => do
  let mut s := 0
  for i in [0:n.toNat!] do
    for j in [:i] do
      s := s + j.repr.length
  IO.println s
| _ => throw $ IO.userError "give upper bound"

@nomeata
Copy link
Collaborator Author

nomeata commented Nov 7, 2024

And the mathlib perf changes are essentially empty:
http://speed.lean-fro.org/mathlib4/compare/9aa8a0ce-0c12-4365-b1be-6012a7c2698c/to/ea453449-19e9-424e-860e-2ead229239a0

Is omega used that little in mathlib?

I’m inclined to merge anyways, given the big impact on @TwoFX’s work. Any objections?

@nomeata nomeata marked this pull request as draft November 7, 2024 16:47
@nomeata
Copy link
Collaborator Author

nomeata commented Nov 7, 2024

I made a mistake while testing, and there is no performance gain after all. Was too good to be true in the first place.

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc November 7, 2024 17:10 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 7, 2024
@nomeata
Copy link
Collaborator Author

nomeata commented Nov 12, 2024

!bench

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc November 12, 2024 10:06 Inactive
@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit c3d4f27.
There were significant changes against commit 456e6d2:

  Benchmark           Metric         Change
  =====================================================
+ big_omega.lean      branches        -9.0% (-1467.6 σ)
+ big_omega.lean      instructions   -10.4% (-1882.4 σ)
+ big_omega.lean      task-clock      -8.0%   (-10.2 σ)
+ big_omega.lean      wall-clock      -8.0%   (-10.2 σ)
+ big_omega.lean MT   branches       -11.3%  (-302.2 σ)
+ big_omega.lean MT   instructions   -10.8%  (-221.0 σ)
+ import Lean         task-clock      -7.6%   (-25.6 σ)
+ import Lean         wall-clock      -7.5%   (-25.5 σ)
+ nat_repr            task-clock      -3.8%   (-10.1 σ)
+ nat_repr            wall-clock      -3.8%   (-10.6 σ)
+ stdlib              maxrss          -1.4%   (-60.2 σ)

@nomeata
Copy link
Collaborator Author

nomeata commented Nov 12, 2024

This benchmark report looks better, although it makes me wonder if something is amiss with our benchmarks here, because I just pulled in the latest master, and didn’t change anything else. Oh well.

@nomeata nomeata marked this pull request as ready for review November 12, 2024 10:20
@nomeata nomeata added the awaiting-review Waiting for someone to review the PR label Nov 12, 2024
@nomeata
Copy link
Collaborator Author

nomeata commented Nov 12, 2024

@kim-em, do you want to have a brief look if you are ok with this change?

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 12, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 12, 2024
@leanprover-community-bot
Copy link
Collaborator

Mathlib CI status (docs):

@kim-em kim-em added this pull request to the merge queue Nov 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 13, 2024
@nomeata nomeata added this pull request to the merge queue Nov 13, 2024
Merged via the queue into master with commit f18d9e0 Nov 13, 2024
21 checks passed
@kim-em kim-em added the changelog-language Language features, tactics, and metaprograms label Jan 4, 2025
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-language Language features, tactics, and metaprograms 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.

4 participants