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

faster PCUICSR #1081

Merged
merged 1 commit into from
Jun 12, 2024
Merged

faster PCUICSR #1081

merged 1 commit into from
Jun 12, 2024

Conversation

mrhaandi
Copy link
Contributor

@mrhaandi mrhaandi commented May 8, 2024

Replaced slow firstorder calls by appropriate constructor, left, right calls.
This improves performance from
5m49.68s | 2644580 ko | PCUICSR.vo
to
0m49.37s | 1443968 ko | PCUICSR.vo,
also improving overall project compilation speed.

@TheoWinterhalter
Copy link
Member

I'm amazed that it's only two occurrences that take up that long!

@ppedrot
Copy link
Collaborator

ppedrot commented May 9, 2024

FTR I have had firstorder on my radar for a while. It's very inefficient because it strongly normalizes all terms from the context. (It's a bit sad that these instances will disappear for benching purposes, btw.)

@TheoWinterhalter
Copy link
Member

Would that be reason enough to keep them though? Could we keep them as a special CI that only runs for benchmarks?

@mattam82
Copy link
Member

It does not seem reasonable to me to not fix it so that the firstorder issue stays in the CI. @ppedrot It should be easy to make a specific test-case for that no?

@mattam82 mattam82 merged commit b9739c1 into MetaCoq:main Jun 12, 2024
5 checks passed
@mattam82
Copy link
Member

Thanks @mrhaandi !

@ppedrot
Copy link
Collaborator

ppedrot commented Jun 13, 2024

Note that coq/coq#19126 has probably made this PR much less usefull in practice.

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.

4 participants