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

#17984 can cause significant slowdown when the context contains many assumptions of the form 0 <= ... < ... #18770

Closed
MackieLoeffel opened this issue Mar 11, 2024 · 6 comments · Fixed by #18818
Labels
kind: bug An error, flaw, fault or unintended behaviour. needs: triage The validity of this issue needs to be checked, or the issue itself updated.
Milestone

Comments

@MackieLoeffel
Copy link

Description of the problem

The change in #17984 can cause significant slowdown when the context contains many assumptions of the form 0 <= ... < .... See the Coq file below for an example. This can maybe be resolved by moving the destruct the the beginning of Z.euclidean_division_equations_cleanup.

Small Coq file to reproduce the bug

From Coq Require Import ZArith List Zify.
Import ListNotations.

Open Scope Z_scope.

Ltac Zify.zify_post_hook ::= Z.to_euclidean_division_equations.

Goal (fold_left (fun T _ => (forall x, 0 <= x < 100 -> T)) (repeat tt 100) False).
  simpl. intros.
  Time Succeed zify. (* Finished transaction in 5.049 secs (5.049u,0.s) (successful) *)

(* Removing | [ H : 0 <= ?x < _ |- _ ] => destruct H *)
Ltac Z.euclidean_division_equations_cleanup ::=
  repeat match goal with
         | [ H : ?x = ?x -> ?Q |- _ ] => specialize (H eq_refl)
         | [ H : ?x <> ?x -> _ |- _ ] => clear H
         | [ H : ?x < ?x -> _ |- _ ] => clear H
         | [ H : ?T -> ?Q, H' : ?T |- _ ] => specialize (H H')
         | [ H : ?T -> _, H' : ~?T |- _ ] => clear H
         | [ H : ~?T -> _, H' : ?T |- _ ] => clear H
         | [ H : ?A -> ?x = ?x -> ?Q |- _ ] => specialize (fun a => H a eq_refl)
         | [ H : ?A -> ?x <> ?x -> _ |- _ ] => clear H
         | [ H : ?A -> ?x < ?x -> _ |- _ ] => clear H
         | [ H : ?A -> ?B -> ?Q, H' : ?B |- _ ] => specialize (fun a => H a H')
         | [ H : ?A -> ?B -> _, H' : ~?B |- _ ] => clear H
         | [ H : ?A -> ~?B -> _, H' : ?B |- _ ] => clear H
         | [ H : 0 < ?x -> _, H' : ?x < 0 |- _ ] => clear H
         | [ H : ?x < 0 -> _, H' : 0 < ?x |- _ ] => clear H
         | [ H : ?A -> 0 < ?x -> _, H' : ?x < 0 |- _ ] => clear H
         | [ H : ?A -> ?x < 0 -> _, H' : 0 < ?x |- _ ] => clear H
         | [ H : 0 <= ?x -> _, H' : ?x < 0 |- _ ] => clear H
         | [ H : ?x <= 0 -> _, H' : 0 < ?x |- _ ] => clear H
         | [ H : ?A -> 0 <= ?x -> _, H' : ?x < 0 |- _ ] => clear H
         | [ H : ?A -> ?x <= 0 -> _, H' : 0 < ?x |- _ ] => clear H
         | [ H : 0 < ?x -> _, H' : ?x <= 0 |- _ ] => clear H
         | [ H : ?x < 0 -> _, H' : 0 <= ?x |- _ ] => clear H
         | [ H : ?A -> 0 < ?x -> _, H' : ?x <= 0 |- _ ] => clear H
         | [ H : ?A -> ?x < 0 -> _, H' : 0 <= ?x |- _ ] => clear H
         | [ H : 0 <= ?x -> ?Q, H' : ?x <= 0 |- _ ] => specialize (fun pf => H (@Z.eq_le_incl 0 x (eq_sym pf)))
         | [ H : ?A -> 0 <= ?x -> ?Q, H' : ?x <= 0 |- _ ] => specialize (fun a pf => H a (@Z.eq_le_incl 0 x (eq_sym pf)))
         | [ H : ?x <= 0 -> ?Q, H' : 0 <= ?x |- _ ] => specialize (fun pf => H (@Z.eq_le_incl 0 x pf))
         | [ H : ?A -> ?x <= 0 -> ?Q, H' : 0 <= ?x |- _ ] => specialize (fun a pf => H a (@Z.eq_le_incl x 0 pf))
         | [ H : ?x < ?y -> _, H' : ?x = ?y |- _ ] => clear H
         | [ H : ?x < ?y -> _, H' : ?y = ?x |- _ ] => clear H
         | [ H : ?A -> ?x < ?y -> _, H' : ?x = ?y |- _ ] => clear H
         | [ H : ?A -> ?x < ?y -> _, H' : ?y = ?x |- _ ] => clear H
         | [ H : ?x = ?y -> _, H' : ?x < ?y |- _ ] => clear H
         | [ H : ?x = ?y -> _, H' : ?y < ?x |- _ ] => clear H
         | [ H : ?A -> ?x = ?y -> _, H' : ?x < ?y |- _ ] => clear H
         | [ H : ?A -> ?x = ?y -> _, H' : ?y < ?x |- _ ] => clear H
         (* | [ H : 0 <= ?x < _ |- _ ] => destruct H *)
         end.
  Time Succeed zify. (* Finished transaction in 0.036 secs (0.036u,0.s) (successful) *)

(* Moving | [ H : 0 <= ?x < _ |- _ ] => destruct H to the beginning. *)
Ltac Z.euclidean_division_equations_cleanup ::=
  repeat match goal with
         | [ H : 0 <= ?x < _ |- _ ] => destruct H
         | [ H : ?x = ?x -> ?Q |- _ ] => specialize (H eq_refl)
         | [ H : ?x <> ?x -> _ |- _ ] => clear H
         | [ H : ?x < ?x -> _ |- _ ] => clear H
         | [ H : ?T -> ?Q, H' : ?T |- _ ] => specialize (H H')
         | [ H : ?T -> _, H' : ~?T |- _ ] => clear H
         | [ H : ~?T -> _, H' : ?T |- _ ] => clear H
         | [ H : ?A -> ?x = ?x -> ?Q |- _ ] => specialize (fun a => H a eq_refl)
         | [ H : ?A -> ?x <> ?x -> _ |- _ ] => clear H
         | [ H : ?A -> ?x < ?x -> _ |- _ ] => clear H
         | [ H : ?A -> ?B -> ?Q, H' : ?B |- _ ] => specialize (fun a => H a H')
         | [ H : ?A -> ?B -> _, H' : ~?B |- _ ] => clear H
         | [ H : ?A -> ~?B -> _, H' : ?B |- _ ] => clear H
         | [ H : 0 < ?x -> _, H' : ?x < 0 |- _ ] => clear H
         | [ H : ?x < 0 -> _, H' : 0 < ?x |- _ ] => clear H
         | [ H : ?A -> 0 < ?x -> _, H' : ?x < 0 |- _ ] => clear H
         | [ H : ?A -> ?x < 0 -> _, H' : 0 < ?x |- _ ] => clear H
         | [ H : 0 <= ?x -> _, H' : ?x < 0 |- _ ] => clear H
         | [ H : ?x <= 0 -> _, H' : 0 < ?x |- _ ] => clear H
         | [ H : ?A -> 0 <= ?x -> _, H' : ?x < 0 |- _ ] => clear H
         | [ H : ?A -> ?x <= 0 -> _, H' : 0 < ?x |- _ ] => clear H
         | [ H : 0 < ?x -> _, H' : ?x <= 0 |- _ ] => clear H
         | [ H : ?x < 0 -> _, H' : 0 <= ?x |- _ ] => clear H
         | [ H : ?A -> 0 < ?x -> _, H' : ?x <= 0 |- _ ] => clear H
         | [ H : ?A -> ?x < 0 -> _, H' : 0 <= ?x |- _ ] => clear H
         | [ H : 0 <= ?x -> ?Q, H' : ?x <= 0 |- _ ] => specialize (fun pf => H (@Z.eq_le_incl 0 x (eq_sym pf)))
         | [ H : ?A -> 0 <= ?x -> ?Q, H' : ?x <= 0 |- _ ] => specialize (fun a pf => H a (@Z.eq_le_incl 0 x (eq_sym pf)))
         | [ H : ?x <= 0 -> ?Q, H' : 0 <= ?x |- _ ] => specialize (fun pf => H (@Z.eq_le_incl 0 x pf))
         | [ H : ?A -> ?x <= 0 -> ?Q, H' : 0 <= ?x |- _ ] => specialize (fun a pf => H a (@Z.eq_le_incl x 0 pf))
         | [ H : ?x < ?y -> _, H' : ?x = ?y |- _ ] => clear H
         | [ H : ?x < ?y -> _, H' : ?y = ?x |- _ ] => clear H
         | [ H : ?A -> ?x < ?y -> _, H' : ?x = ?y |- _ ] => clear H
         | [ H : ?A -> ?x < ?y -> _, H' : ?y = ?x |- _ ] => clear H
         | [ H : ?x = ?y -> _, H' : ?x < ?y |- _ ] => clear H
         | [ H : ?x = ?y -> _, H' : ?y < ?x |- _ ] => clear H
         | [ H : ?A -> ?x = ?y -> _, H' : ?x < ?y |- _ ] => clear H
         | [ H : ?A -> ?x = ?y -> _, H' : ?y < ?x |- _ ] => clear H
         end.
  Time Succeed zify. (* Finished transaction in 0.191 secs (0.191u,0.s) (successful) *)

Version of Coq where this bug occurs

8.19.0

Last version of Coq where the bug did not occur

8.18

@MackieLoeffel MackieLoeffel added kind: bug An error, flaw, fault or unintended behaviour. needs: triage The validity of this issue needs to be checked, or the issue itself updated. labels Mar 11, 2024
@SkySkimmer SkySkimmer changed the title https://github.com/coq/coq/pull/17984 can cause significant slowdown when the context contains many assumptions of the form 0 <= ... < ... #17984 can cause significant slowdown when the context contains many assumptions of the form 0 <= ... < ... Mar 11, 2024
@JasonGross
Copy link
Member

This example might be worth adding to https://github.com/coq-community/coq-performance-tests. If moving destruct to the top fixes the issue, I'd be happy to merge such a PR. (Especially if you can add the example to https://github.com/coq-community/coq-performance-tests so it shows up on the bench)

@MackieLoeffel
Copy link
Author

I created a PR for adding the example to coq-performace-tests here: coq-community/coq-performance-tests#42

@JasonGross
Copy link
Member

Plausibly the clears should all be collected at the bottom? Or we should split up the match into repeat (repeat match <all the clears>; repeat match <all the specialize>; repeat match <destruct>)?

@MackieLoeffel
Copy link
Author

I would think that putting the destruct at the beginning would make sense as it seems like a cheap operation, but I don't have a good intuition for what the most efficient order is.

@Janno
Copy link
Contributor

Janno commented Mar 14, 2024

What is the status of using Ltac2 in the standard library/standard plugins like zify? This would be a prime example of code that I would replace with Ltac2 without thinking about it twice. Collect everything that needs clearing, everything that needs specializing, everything that needs destructing, perform the operations in that order (and batch the clearing operations if the API allows that), rinse, repeat.

@ppedrot
Copy link
Member

ppedrot commented Mar 14, 2024

What is the status of using Ltac2 in the standard library

I think that this question is very much related to the ongoing discussion about the split of the stdlib from the Coq core itself.

JasonGross added a commit to JasonGross/coq that referenced this issue Mar 18, 2024
Fixes coq#18770

We first destruct, then clear, then specialize.  The reasoning is:
- `destruct` before `clear` because the `clear` is not going to make much of
  a difference to `destruct`, but the `destruct` might expose more
  opportunities for `clear`ing
- `clear` between `destruct` and `specialize` so that we don't waste a
  bunch of time modifying the goal (via `specialize`) on hypotheses that
  we would anyway want to `clear`
@coqbot-app coqbot-app bot closed this as completed in ddf6cde Mar 21, 2024
@coqbot-app coqbot-app bot modified the milestone: 8.20+rc1 Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug An error, flaw, fault or unintended behaviour. needs: triage The validity of this issue needs to be checked, or the issue itself updated.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants