Recurse less in extraction#16206
Conversation
|
@coqbot bench |
|
@JasonGross could you test this on your original example? |
|
My laptop is currently broken and I don't have access to Coq, but the code is available at mit-plv/fiat-crypto#1293, just run |
|
🏁 Bench results: 🐢 Top 25 slow downs🐇 Top 25 speed ups |
399e865 to
2168253
Compare
|
Messed up the testing overlay the first time, let's try this again |
|
It timed out in the Assembly/Symbolic that @ppedrot mentioned on zulip |
|
Ah! Told you so. I have a partially written fix for this though, I can push it somewhere after some polishing. |
|
That is very strange, it seems to work fine on our CI https://github.com/mit-plv/fiat-crypto/runs/6943380653?check_suite_focus=true , taking only 34s on master (https://github.com/mit-plv/fiat-crypto/runs/6943380973?check_suite_focus=true#step:7:3159). This might be using an outdated version of master, though, since I haven't gotten around to updating my launchpad packages to have the latest dune, not to fix the issue with configure failing to detect the lablgtk version |
|
I would be very surprised if this has to do with some recent change on master, though. The problem is basically the same as the exponential blowup of the reduced type for extraction. Here we're performing a (nonsensical) recursive analysis of nested inductive types to compute equality schemes, and since it reduces the constructor types, you get the blowup as well. |
|
It's the Symbolic version in mit-plv/fiat-crypto#1293 that's got an issue not the fiat-crypto master version. You can see it fail on that PR's CI too. |
|
Oh, I see, that makes sense. I guess the version I was timing the commit on was old enough (8.14, maybe? Or 8.11?) that it wasn't generating equality schemes for records, or something like that, and hence on that certain Symbolic went from 30s to 26s. I'll probably disable scheme generation for that record when I actually merge the PR (and/or will make the type not exponentially sized). |
Should help with performance issues of rocq-prover/rocq#16206 (comment)
@ppedrot Should we open a separate issue to track this? I've pushed an update to mit-plv/fiat-crypto#1293 disabling the equality generation and I've restarted the CI here. I've also opened rocq-community/coq-performance-tests#22 with the smaller examples |
Should help with performance issues of rocq-prover/rocq#16206 (comment)
2168253 to
e88f586
Compare
|
Seems to work! |
|
Er, wait, was that run without the overlay pointing it at the right branch? |
|
Could you re-add the overlay and we can test this here? |
|
Ping @SkySkimmer |
e88f586 to
48d0b8a
Compare
|
@JasonGross looks like something extracted wrongly, can you minimize? |
|
Gah, I did not realize that the CI merges the overlay with the current master, I guess I'll have to adjust the overlay branch |
|
I still have to look into the failures at the OCaml job and the underlying fiat-crypto job, but let me try running this with the intended branch |
|
@SkySkimmer Minimized: #16288 This issue is already present in master, it was just exposed here by merging the overlay branch into fiat-crypto's master |
|
@SkySkimmer CI looks to be building fiat-crypto fine now, and this PR seems to fix the issue: Shall we merge, or do you want a formal bench? |
|
@JasonGross Do you want this in 8.16? If we ask @ppedrot nicely he might agree. |
|
It would be nice to have in 8.16, but I don't feel particularly strongly |
|
@ppedrot Please consider this for 8.16, but no biggie. |
|
@coqbot merge now |
|
@Alizter: Please take care of the following overlays:
|
|
@SkySkimmer Where is the overlay? |
|
It's not a real overlay, it's only for testing |
Fix #16172