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

improve presup performance #94

Merged
merged 1 commit into from
Jun 3, 2024
Merged

improve presup performance #94

merged 1 commit into from
Jun 3, 2024

Conversation

HuStmpHrrr
Copy link
Member

The current performance of presup kills me. This is the minimal effort I can do to improve the performance. The idea is to lower the search level to reduce useless search effort.

@HuStmpHrrr HuStmpHrrr requested a review from Ailrun June 2, 2024 18:28
@Ailrun
Copy link
Member

Ailrun commented Jun 2, 2024

How much time does this save (in your machine)?

I once did similar to get a better performance, but in my machine the improvement was not significant

@HuStmpHrrr
Copy link
Member Author

How much time does this save (in your machine)?

I once did similar to get a better performance, but in my machine the improvement was not significant

My machine is probably too old (10 years) to a point that it has been slower than github's action runner, but these changes turn the proof checking from very long to short enough for me to stare at the buffer to wait for it to finish (but still long).

@HuStmpHrrr
Copy link
Member Author

though it could have been faster before, checking the CI /~https://github.com/Beluga-lang/McLTT/actions

It was 9m ish before this change #91 making the CI above 12m. now this PR drops it down to 11m.

If you have some ideas maybe you can do something.

@Ailrun
Copy link
Member

Ailrun commented Jun 3, 2024

though it could have been faster before, checking the CI /~https://github.com/Beluga-lang/McLTT/actions

It was 9m ish before this change #91 making the CI above 12m. now this PR drops it down to 11m.

It is strange... That PR added just one module and a lemma for syntax, which should not take 2 or 3 mins. I will also try some optimizations.

My machine is probably too old (10 years) to a point that it has been slower than github's action runner, but these changes turn the proof checking from very long to short enough for me to stare at the buffer to wait for it to finish (but still long).

OK, if that's the case, let's merge this PR.

@HuStmpHrrr
Copy link
Member Author

though it could have been faster before, checking the CI /~https://github.com/Beluga-lang/McLTT/actions
It was 9m ish before this change #91 making the CI above 12m. now this PR drops it down to 11m.

It is strange... That PR added just one module and a lemma for syntax, which should not take 2 or 3 mins. I will also try some optimizations.

My machine is probably too old (10 years) to a point that it has been slower than github's action runner, but these changes turn the proof checking from very long to short enough for me to stare at the buffer to wait for it to finish (but still long).

OK, if that's the case, let's merge this PR.

I know. I also find it strange, but the drop is sudden and clear. if you compare this PR and the other one, you will see that there is an improvement in this PR.

@Ailrun Ailrun merged commit f8feff0 into main Jun 3, 2024
2 checks passed
@Ailrun Ailrun deleted the feature/improve-presup branch June 3, 2024 01:43
@Ailrun
Copy link
Member

Ailrun commented Jun 3, 2024

@HuStmpHrrr Actually, that added 3 min is not from library checking. It is from OCaml setup! What...

C.f. /~https://github.com/Beluga-lang/McLTT/actions/runs/9343409604/job/25712951655?pr=95 of #95

It seems like it was a problem of opam depext to do with cache. Updating cache name should fix the issue of CI.

@Ailrun
Copy link
Member

Ailrun commented Jun 3, 2024

It should be fixed in #96 (together with a bit of improvement in NatCases time)

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.

2 participants