-
Notifications
You must be signed in to change notification settings - Fork 7
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
Replace lia_tactics
and field_tactics
with mczify and Algebra Tactics
#4
Conversation
lia_tactics
and field_tactics
with mczify and Algebra Tacticslia_tactics
and field_tactics
with mczify and Algebra Tactics
GitHub Actions kill |
FTR, I managed to obtain a memory trace using the Memtrace utilities. |
I think I'm going to optimize the size of reified terms (by inserting let-ins for structure instances) for the moment. I can also share the memory trace I obtained if it is useful. |
In the last invocation of the field tactic in |
Hi, thanks for running memory profiler. You are right context readback is naive and could be cached. It would be nice to have an issue on coq elpi with all the dzta so that I can reproduce and test any optimization |
I also attempted to replace the use of |
@CohenCyril I guess I need your suggestion on the above point before further investigation. My understanding is that, in theory, we should use CoqEAL's computable (parameterized) Z instead. But, in the presence of other tools (e.g., mczify, and the heavy use of the opaque |
Done. (But I still have to release mczify 1.2.0 to fix CI.) |
This is really cool! If I read the CI correctly, this PR remains ~23% slower than the original code (16s vs 13s with coq 8.14 + mathcomp 1.12). I have seen that the two largest instances of polynomials are now normalized (both in |
@amahboubi I think that execution time in CI is not reliable for benchmarking purposes. I will do that on a more reliable machine and report. BTW, I recently observed that several performance issues related to |
Here is the benchmark result with Coq 8.14.0 and MathComp 1.13.0, comparing this PR (after) with the current master branch (before).
Overall this PR is faster because of |
And here is the same comparison except that it uses MathComp 1.12.0.
|
Note that fixing LPCIC/coq-elpi#320 seems to unblock possibilities for further performance improvements, e.g., locally introducing locked versions of field operators to optimize the second step of conversion (I have a patch to do this but it rather deoptimizes reification because of the issue of Coq-Elpi), that may make Algebra Tactics faster than the |
When trying to compile this branch on my machine (coq 8.14, mathcomp 1.13.0, algebra-tactics 0.1.0), I get an error in file |
@amahboubi You need Algebra Tactics 0.2.0 and Mczify 1.2.0 to compile this branch. |
OK thanks. |
The last commit fixes some performance issues. So here is the new result of performance comparison using Coq 8.14.0 and MathComp 1.13.0:
and that using Coq 8.14.0 and MathComp 1.12.0:
|
theories/a_props.v
Outdated
by vm_compute. | ||
have squared (rat2 := rat_of_Z 2) : | ||
(rho 2 * rat2 - alpha rat2) ^+ 2 <= delta rat2. | ||
by rewrite /delta rho2_eq /alpha [_ <= _]refines_eq; vm_compute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this rewrite [_ <= _]refines_eq
fragile performance-wise. In the case of MathComp 1.12.0, if I do not unfold alpha
before that, it becomes too slow. Is this because typeclass search performs wasteful computations?
Here is a new result of performance comparison using Coq 8.15.0, MathComp 1.14.0, LPCIC/coq-elpi#324, and the master branch of Algebra Tactics.
|
FYI, the Algebra Tactics paper has been accepted to ITP'22. I will try to finish this PR before submitting the final version (deadline: Apr. 27). |
Ci failure should solve itself automagically soon when the docker images will get opam 2.1 |
Just FYI @pi8027 I restarted the last GHA run, and the recompilation-slowdown issue you reported seems to be fixed: |
Thanks @erikmd. That's really helpful. |
Is this PR ready then? |
well, lets tag @amahboubi |
lia_tactics
and field_tactics
with mczify and Algebra Tacticslia_tactics
and field_tactics
with mczify and Algebra Tactics
I will do the benchmark again. Then this PR should be ready to merge. |
With Coq Platform 2022.04.0, Algebra Tactics a2edba0, and Apery 4f6f581...7335782, I obtained the following result:
|
@amahboubi This PR is also ready to merge. When you merge this PR, please "create a merge commit" rather than "squash and merge" if you are ok with that. I intentionally split this work into 6 commits, which might be useful for further performance investigation. |
This PR is based on #3, #9, and #11. The plan is to use apery as a benchmark of mczify and Algebra Tactics (in terms of LoC, performance, and some other specific details). Therefore, changes independent from those libraries should be done in #3 (or in another separate PR).
Major changes:
binomialz
now returnsint
instead ofrat
.rat_of_positive
has been replaced withrat_of_Z
. CoqEAL refinements inrho_computations.v
have also been adapted.Some observations:
rat_of_Z
has been redefined usingint_of_Z
and declared as a ring morphism (ring instances ofZ
is declared inssrZ.v
of mczify). Algebra Tactics can handlerat_of_Z
without any special treatment since they support ring morphisms. Also, declaringrat_of_Z
as a ring morphism drastically simplifieshanson.v
.Two invocations of theThey are now faster than before.field
tactic inops_for_b.v
are still too slow. Investigation needed.