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

Turn class_of records into primitive records and get rid of the xclass idiom #591

Merged
merged 2 commits into from
Oct 7, 2020

Conversation

pi8027
Copy link
Member

@pi8027 pi8027 commented Sep 10, 2020

Motivation for this change

This PR redefines the class_of records as primitive records except for morphisms and finGroupType, and also removes the xclass idiom by following #462 (comment) by @ggonthier.

Fixes #544 and closes #546.

Things done/to do
  • added corresponding entries in CHANGELOG_UNRELEASED.md (do not edit former entries)
  • [ ] added corresponding documentation in the headers
Automatic note to reviewers

Read this Checklist and make sure there is a milestone.

@CohenCyril
Copy link
Member

I am curious about the new performances...

@pi8027
Copy link
Member Author

pi8027 commented Sep 10, 2020

Yes. It will be nice to have benchmarks, including this commented out proof:

(*Coq 8.3 processes this shorter proof correctly, but then crashes on Qed.
In Coq 8.4 Qed takes about 18s.
In Coq 8.7, everything seems to be all right *)
(*
Lemma Xirredp_FAdjoin' (F : fieldType) (p : {poly F}) :
irreducible_poly p ->
{L : fieldExtType F & Vector.dim L = (size p).-1 &
{z | root (map_poly (in_alg L) p) z & <<1; z>>%VS = fullv}}.
Proof.

@pi8027
Copy link
Member Author

pi8027 commented Sep 10, 2020

The source of CI failure seems to be coq/coq#9508 which has been fixed in coq/coq#9509. I see there is a possible fix, but since we are about to drop compatibility with Coq (< 8.10), I would rather stop supporting Coq 8.7 and 8.8.

@pi8027 pi8027 force-pushed the primitive-class-records branch from 53d25cd to a4cb449 Compare September 10, 2020 13:14
@CohenCyril CohenCyril self-assigned this Sep 16, 2020
@pi8027
Copy link
Member Author

pi8027 commented Sep 17, 2020

Here are some benchmarking results using Coq 8.12.0:

  • MathComp master (c2e7bad)
    make test-suite  1072.06s user 20.45s system 99% cpu 18:19.47 total
    
  • MathComp primitive-class-records (a4cb449)
    make test-suite  1004.05s user 21.26s system 99% cpu 17:12.96 total
    
  • Odd order (689f07b) with MathComp master
    make  1323.96s user 11.20s system 99% cpu 22:24.46 total
    
  • Odd order with MathComp primitive-class-records
    make  1318.69s user 11.22s system 99% cpu 22:18.43 total
    

So it seems that this PR improves the performance of conversion and/or inference regarding packed classes.

@CohenCyril
Copy link
Member

CohenCyril commented Sep 17, 2020

Here are some benchmarking results using Coq 8.12.0:

  • MathComp master (c2e7bad)
    make test-suite  1072.06s user 20.45s system 99% cpu 18:19.47 total
    
  • MathComp primitive-class-records (a4cb449)
    make test-suite  1004.05s user 21.26s system 99% cpu 17:12.96 total
    
  • Odd order (689f07b) with MathComp master
    make  1323.96s user 11.20s system 99% cpu 22:24.46 total
    
  • Odd order with MathComp primitive-class-records
    make  1318.69s user 11.22s system 99% cpu 22:18.43 total
    

So it seems that this PR improves the performance of conversion and/or inference regarding packed classes.

I would rather say the benchmark does not reveal significant impact.
What about:

(*Coq 8.3 processes this shorter proof correctly, but then crashes on Qed.
In Coq 8.4 Qed takes about 18s.
In Coq 8.7, everything seems to be all right *)
(*
Lemma Xirredp_FAdjoin' (F : fieldType) (p : {poly F}) :
irreducible_poly p ->
{L : fieldExtType F & Vector.dim L = (size p).-1 &
{z | root (map_poly (in_alg L) p) z & <<1; z>>%VS = fullv}}.
Proof.
?

@pi8027
Copy link
Member Author

pi8027 commented Sep 23, 2020

I would rather say the benchmark does not reveal significant impact.
What about:

(*Coq 8.3 processes this shorter proof correctly, but then crashes on Qed.
In Coq 8.4 Qed takes about 18s.
In Coq 8.7, everything seems to be all right *)
(*
Lemma Xirredp_FAdjoin' (F : fieldType) (p : {poly F}) :
irreducible_poly p ->
{L : fieldExtType F & Vector.dim L = (size p).-1 &
{z | root (map_poly (in_alg L) p) z & <<1; z>>%VS = fullv}}.
Proof.

?

In Coq 8.12 and MathComp master, it takes 4.304 seconds. In this PR, it takes 3.284 seconds.

@pi8027 pi8027 force-pushed the primitive-class-records branch 2 times, most recently from a0490c1 to 49a1ee2 Compare September 25, 2020 00:21
Copy link
Member

@CohenCyril CohenCyril left a comment

Choose a reason for hiding this comment

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

Looks promising. I'm in favor of merging.

@CohenCyril
Copy link
Member

@ggonthier do you agree?

@pi8027 pi8027 force-pushed the primitive-class-records branch 4 times, most recently from d701396 to 8c1328d Compare September 29, 2020 11:13
@CohenCyril
Copy link
Member

Since merging this PR will make us drop support for Coq < 8.9, I think it should be approved in the next meeting before being merged. But as for me, it's all good.

@pi8027 pi8027 force-pushed the primitive-class-records branch from 89df582 to a7a8ada Compare October 7, 2020 14:25
@pi8027
Copy link
Member Author

pi8027 commented Oct 7, 2020

Shall we merge?

@CohenCyril CohenCyril merged commit 9bfc983 into math-comp:master Oct 7, 2020
@pi8027 pi8027 deleted the primitive-class-records branch October 8, 2020 05:58
@CohenCyril CohenCyril added this to the 1.12.0 milestone Oct 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ambiguous coercion paths in fieldext.v
2 participants