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: rm partial / bounds checks in Array.qsort #3933

Closed
wants to merge 1 commit into from

Conversation

digama0
Copy link
Collaborator

@digama0 digama0 commented Apr 17, 2024

With omega in core, it is now more feasible to perform the necessary proofs to eliminate the partial from Array.qsort, as well as the many uses of Array.get! and Array.swap!. (This is also a perfect example of the kind of code that would benefit from the proposed Vector API.)

I wrote this code over a year ago, but the stance regarding proofs in lean core has shifted so I think this is now more likely to be accepted. This is in any case blocking some proofs in Std which are built on top of uses of Array.qsort.

@digama0
Copy link
Collaborator Author

digama0 commented Apr 17, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 228efff.
The entire run failed.
Found no significant differences.

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

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

Mathlib CI status (docs):

  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 88ee503f02e047c8c3fd1eb3ac8310b123380038 --onto d3e004932c1f5ac3946850692940512d381c7634. (2024-04-17 12:10:44)
  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 036b5381f0f1a193f1734ca9ffb608f09c7b7f0b --onto d3e004932c1f5ac3946850692940512d381c7634. (2024-04-17 20:12:37)
  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 036b5381f0f1a193f1734ca9ffb608f09c7b7f0b --onto 62cdb51ed5b9d8487877d5a4adbcd4659d81fc6a. (2024-04-24 06:24:17)

@digama0 digama0 marked this pull request as draft April 17, 2024 12:43
@digama0
Copy link
Collaborator Author

digama0 commented Apr 17, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 28c305a.
There were significant changes against commit 036b538:

  Benchmark   Metric          Change
  ============================================
- stdlib      instructions      2.0% (371.8 σ)
- stdlib      task-clock        1.3%  (11.5 σ)
- stdlib      type checking     9.0%  (11.0 σ)
- stdlib      wall-clock        8.0%  (86.5 σ)

@digama0 digama0 marked this pull request as ready for review April 18, 2024 05:12
@hargoniX
Copy link
Contributor

Do you know why this gets the stdlib benchmark slower by a clearly significant margin? This seems rather counter intuitive to me. Are the proofs in Array.qsort taking so long?

if h : lo < hi.1 then
let ⟨as, mid, (_ : lo ≤ mid), _⟩ :=
qpartition as lt ⟨lo, Nat.lt_trans h hi.2⟩ hi (Nat.le_of_lt h)
let as := sort as lo ⟨mid - 1, by omega⟩
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily relevant for this PR, but one simple improvement to quicksort (as explained in the Sedgewick paper) is to recurse on the smallest subproblem first. This ensures a stack depth of O(log n) because the other call will be in tail position.

@digama0
Copy link
Collaborator Author

digama0 commented Apr 18, 2024

The proof is actually suspiciously slow (the definition of qpartition is about 2 seconds). I think it might actually be the compiler too; this code contains three if statements at the beginning and an @[inline] and this seems to hit a relatively bad case for the compiler (there were other variations of this code that behaved even worse). The generated code itself seems to be fine though.

This code is slightly suboptimal, in that n is passed as an argument to qpartition even though it is redundant, and likewise lo and hi are passed unnecessarily to qpartition.loop. My original version of this code used some tricks to eliminate this, at the cost of making the proof (and code) significantly more obtuse; I decided that this was not a good tradeoff for maintenance reasons and submitted the simpler version here. Ideally, this would be solved by marking these parameters as "ghost" / not computationally relevant.

@digama0
Copy link
Collaborator Author

digama0 commented Apr 18, 2024

!bench

@digama0
Copy link
Collaborator Author

digama0 commented Apr 18, 2024

(The version that was previously benched had a sharing bug. Unsure if that is related to the stdlib slowdown.)

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 824a7fe.
There were significant changes against commit 036b538:

  Benchmark   Metric          Change
  ===========================================
- stdlib      type checking     4.6% (17.2 σ)
- stdlib      wall-clock        3.8% (23.5 σ)

@kim-em kim-em added the WIP This is work in progress, and only a PR for the sake of CI or sharing. No review yet. label Apr 24, 2024
@kim-em kim-em self-requested a review as a code owner May 29, 2024 16:23
@Kha
Copy link
Member

Kha commented Aug 29, 2024

Marking draft pending further benchmarking (build time and run time)

@kim-em
Copy link
Collaborator

kim-em commented Dec 11, 2024

Oh, sorry, I missed this, and did it independently. (Additionally, using Vector to clean up a bit.)

I also have most of proof that the result is sorted, on a branch.

@kim-em kim-em closed this Dec 11, 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 WIP This is work in progress, and only a PR for the sake of CI or sharing. No review yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants