-
Notifications
You must be signed in to change notification settings - Fork 640
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
Fix the relevance generated by the set / pose tactics. #15090
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
That seems like a big increase in the stdlib. |
coq-stlidb has a strong tendency to be noisy, I am more concerned by bedrock2 which does appear in the per-line diff. |
I'm not sure it's that noisy, at least when I looked at some past benches stdlib didn't seem to create outliers. |
6fc01a0
to
e9ae01b
Compare
Let's get another bench: https://gitlab.com/coq/coq/-/jobs/1744108867 |
Hmm, was it noise in the end? Bedrock does not appear any more in the per-line diff. 🐙 |
By the way, should we trust more the time or the CPU instructions? |
Cycles, even more than instructions. |
I said instructions because they seem more stable from one run to the other, while cycles seem more in line with times. So, should I conclude from your answer that running several times and computing an average time is eventually more faithful to the reality than counting the instructions? |
Averaging is always better, but I think that despite the PR that removed the noise from opam resolution, the new machines are still quite noisy. I don't know why but pendulum was clearly around a 1% noise threshold, whereas we get move movement now. |
What do we do with this PR? The code change is definitely sound and desired by anybody using SProp (I can testify personally) but I am still a bit concerned this might be a bit slower in practice. Should we run another bench, or one with several runs? cc @SkySkimmer |
Using |
I am not sure about that. Since we already compute the type, the latter should be more compact in general than the term so that the relevance is going to require less term crawling. Relatedly, I had a look at |
relevance_of_type needs reduction, relevance_of_term doesn't
PR when? |
This is my opinion too. I did not look at details, so this is a bit speculative, but can't the computation of relevance be only in the From a practical point of view, I'm more actually also worried about |
Rather, typing and friends should return not only the type but also the sort it lives in. |
Already, right. Maybe not too complicated to do actually?? |
e9ae01b
to
3ec355c
Compare
Bench with relevance_of_term: https://gitlab.com/coq/coq/-/jobs/1776762641 |
Nothing out of the ordinary. |
@coqbot merge now |
No description provided.