Improve waiting for processes on Windows

[ghost]

This is another small improvement that could help Windows builds. Currently, on Windows we resort to busy polling as Unix doesn't provide a way to wait for multiple processes:

  exception Finished of running_job * Unix.process_status

  let wait_nonblocking_win32 () =
    match
      Hashtbl.iter all ~f:(fun ~key:pid ~data:job ->
        let pid, status = Unix.waitpid [WNOHANG] pid in
        if pid <> 0 then
          raise_notrace (Finished (job, status)))
    with
    | () -> None
    | exception (Finished (job, status)) ->
      Hashtbl.remove all job.pid;
      Some (job, status)

  let rec wait_win32 () =
    match wait_nonblocking_win32 () with
    | None ->
      ignore (Unix.select [] [] [] 0.001);
      wait_win32 ()
    | Some x -> x

This could be avoided by using WaitForMultipleObjects.

[ghost]

/cc @nojb

[nojb]

Interesting! Assuming it works out well to use it, is it OK for dune to contain C stubs?

[ghost]

Yes. However, for every C stub we need an alternative OCaml implementation to use during bootstrap.

[rgrinberg]

ping @nojb. we're still polling and you always seem to run into performance problems with dune :)

[nojb]

@rgrinberg Thanks for the ping; I had forgotten about this issue. I'll try to give this one a try and report back.

[rgrinberg]

Np. In particular, it would be nice to have this as a feature in the spawn library. That way, I could make use of it in lsp and drop the polling there as well.

[nojb]

I have a first try at https://github.com/ocaml/dune/compare/main...nojb:WaitForMultipleObjects?expand=1. However in a quick test I didn't see any speedup (and it even seemed to be a tiny bit slower). Anyway I need to do more testing and make sure I didn't miss anything.

[rgrinberg]

Indeed your implementation seems fishy to me. When a new process is created, your call to WaitForMultipleObjects should be interrupted and done again with the new process list.

[nojb]

Indeed your implementation seems fishy to me. When a new process is created, your call to WaitForMultipleObjects should be interrupted and done again with the new process list.

Thanks, sounds reasonable; I'll try to take another look soon.

[rgrinberg]

This stackoverflow hint is possibly useful:

https://stackoverflow.com/a/9217483/1424711

If you spawn these child process yourself you could use Job objects, they can notify you when a process that is part of your job has ended.

Use JobObjectAssociateCompletionPortInformation and catch JOB_OBJECT_MSG_ABNORMAL_EXIT_PROCESS or JOB_OBJECT_MSG_EXIT_PROCESS.

So it seems like we can assign all of our processes to a job, and then wait for this event. Also maybe we can pull out useful stats information from there as well using Resource Accounting. Might be useful to make --trace-file work on Windows.