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

Monad-thunk every nontrivial nontac_expr #19

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Monad-thunk every nontrivial nontac_expr #19

merged 1 commit into from
Dec 4, 2023

Conversation

SkySkimmer
Copy link
Owner

@SkySkimmer SkySkimmer commented Dec 4, 2023

Fix #17

I am not fully sure that this is a compiler bug as the

  let l = List.map (fun f -> Proofview.tclIGNORE (thaw f)) l in

in the implementation of dispatch could also be blamed for not thunking properly (ie should be

  let l = List.map (fun f -> tclUNIT () >>= fun () -> tclIGNORE (thaw f)) l in

) but it does seem hard or impossible to trigger a bug with non compiled ltac2.

This change does cause some slowdown (bug_10107 goes from 0.04s to 0.06s, non compiled baseline being 0.25s)

Fix #17

I am not fully sure that this is a compiler bug as the
~~~ocaml
  let l = List.map (fun f -> Proofview.tclIGNORE (thaw f)) l in
~~~
in the implementation of `dispatch` could also be blamed for not
thunking properly (ie should be
~~~ocaml
  let l = List.map (fun f -> tclUNIT () >>= fun () -> tclIGNORE (thaw f)) l in
~~~
) but it does seem hard or impossible to trigger a bug with non
compiled ltac2.

This change does cause some slowdown (bug_10107 goes from 0.04s to
0.06s, non compiled baseline being 0.25s)
@SkySkimmer SkySkimmer merged commit 4e1a3f7 into main Dec 4, 2023
1 check passed
@SkySkimmer SkySkimmer deleted the bug-17 branch December 4, 2023 14:00
@SkySkimmer
Copy link
Owner Author

This change does cause some slowdown

also about doubles timings in tests/minibench (still much faster than non compiled)

@Janno
Copy link

Janno commented Dec 4, 2023

Would fixing the code in Coq reduce the performance overhead of this change?

SkySkimmer added a commit that referenced this pull request Dec 4, 2023
Improves perf after #19
@SkySkimmer
Copy link
Owner Author

IDK
#21 improves perf some, I think it's still correct.

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.

Compilation produces code in incorrect order
2 participants