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
Avoid roundtrip [fconstr] -> [constr] -> [fconstr] in the kernel #16497
Conversation
@coqbot bench |
9e67a05
to
e5a3cca
Compare
🏁 Bench results:
🐢 Top 25 slow downs
🐇 Top 25 speed ups
|
@coqbot bench |
Not sure how useful it wil be with this many failures |
Thanks. I'm going to try to look into the failures, they are all quite mysterious to me. |
🏁 Bench results:
🐢 Top 25 slow downs
🐇 Top 25 speed ups
|
0a7d1a5
to
83aa5c0
Compare
The latest change fixed most of the issues. I'm not sure about fiat-crypto, but unimath is a timeout. This seems to suggest that the cost of maintaining sharing is too high compared to the (hypothetical) benefit. It might be possible to regain some efficiency with a lazy copy. |
@coqbot bench |
🏁 Bench results:
🐢 Top 25 slow downs
🐇 Top 25 speed ups
|
Those slowdowns in prennial are looking pretty worrying. I wonder why they are happening.
|
@Alizter the issue is that the copy of the |
This is indeed starting to look like a failed experiment... |
@ppedrot I concur. My current take is that |
Closing this, since this PR seems to fulfilled its goal. |
This is based on the discussion here: https://coq.zulipchat.com/#narrow/stream/237656-Coq-devs-.26-plugin-devs/topic/reduction.20in.20the.20kernel
It should be completely backwards compatible. I would like to get a benchmark run to determine if there is any impact. The code is a hackish right now (and contains some extra comments) just to test the potential performance difference, I will clean up before merge if we determine if it is useful.