Adds native polling mode support on Windows #6087

yams-yams · 2022-08-18T20:59:51Z

Adds support for polling mode on Windows using a new internal library winwatch, which takes inspiration for its structure from fsevents. winwatch uses the Windows ReadDirectoryChangesW API to watch for file changes and an I/O Completion Port to report notifications in a callback. dune_file_watcher now uses winwatch instead of fswatch on native Windows, and notifications received in dune_file_watcher are filtered for relevance before they are passed to scheduler.thread_safe_send_emit_events_job.

One additional change was made for compatibility on Windows:
The rpc server typically uses Unix.pipe in non-blocking mode, but non-blocking mode is not supported on Windows. The proposed solution instead uses a blocking pipe that times out every 0.1 second and reblocks.

Potential areas for improvement:
The proposed solution does not prevent the same path from being watched multiple times. A potential solution would be a trie that stores paths, like Watch_trie used by for fsevents.

Signed-off-by: Uma Kothuri [email protected]

nojb · 2022-08-18T21:14:08Z

Just to add a little background information: @yams-yams worked on this project as part of an internship at LexiFi, and we have already discussed the general appraoch internally. I am planning to do a review of the PR, but of course additional reviewers are welcome!

nojb

I did a first pass over the C code; I will do another pass later over the Caml code.

nojb · 2022-08-19T17:03:39Z

src/csexp_rpc/csexp_rpc.ml

+      | [], [], [] ->
+        (* Timeout *)
+        if peek_named_pipe t.r_interrupt_accept then (
+          print_endline "data available!";


Debug left-over, can be removed.

nojb · 2022-08-23T04:42:38Z

src/winwatch/winwatch_stubs.c

@@ -0,0 +1,207 @@
+#include <stdio.h>


I think stdio.h can be removed.

Suggested change

#include <stdio.h>

nojb · 2022-08-23T04:43:37Z

src/winwatch/winwatch_stubs.c

+#include <caml/threads.h>
+#include <caml/unixsupport.h>
+#include <caml/custom.h>


Can these three be removed?

Suggested change

#include <caml/threads.h>

#include <caml/unixsupport.h>

#include <caml/custom.h>

nojb · 2022-08-23T04:46:30Z

src/winwatch/winwatch_stubs.c

+            if (event->NextEntryOffset)
+            {
+                *((char**)&event) += event->NextEntryOffset;
+            }
+            else
+            {
+                break;
+            }


Can be simplified a bit:

Suggested change

if (event->NextEntryOffset)

{

*((char**)&event) += event->NextEntryOffset;

}

else

{

break;

}

if (event->NextEntryOffset == 0) break;

*((char**)&event) += event->NextEntryOffset;

nojb · 2022-08-23T04:47:02Z

src/winwatch/winwatch_stubs.c

+
+        for (;;)
+        {
+            name_len = event->FileNameLength / sizeof(WCHAR);


I would move this to below the switch so that it is closer to where it is used.

nojb · 2022-08-23T04:54:50Z

src/winwatch/winwatch_stubs.c

+        winwatch_watch(t);
+    }
+
+    CAMLreturn(Val_unit);


The function signature specifies that it returns (unit, exn) result, so this is wrong here. It should either return Ok () or Error exn (if the callback raises an exception). Currently it doesn't matter because we never exit the main loop.

nojb · 2022-08-23T04:56:38Z

src/winwatch/winwatch_stubs.c

+            file_name = malloc(sizeof(WCHAR) * (name_len + 1));
+            memcpy(file_name, event->FileName, sizeof(WCHAR) * name_len);
+            file_name[name_len] = 0;
+            v_file_name = caml_copy_string_of_utf16(file_name);


Could use WideCharToMultiByte directly here to convert from UTF-16 to UTF-8 to avoid the extra copy.

https://docs.microsoft.com/en-us/windows/win32/api/stringapiset/nf-stringapiset-widechartomultibyte

agree, you can allocate and copy directly into the ocaml value (if that's what you meant @nojb ?). no need to nul-terminate this way.

nojb · 2022-08-23T04:57:02Z

src/winwatch/winwatch_stubs.c

+        if (ok == FALSE)
+            /* Ignore errors */
+            continue;


Cosmetic:

Suggested change

if (ok == FALSE)

/* Ignore errors */

continue;

if (ok == FALSE) /* Ignore errors */

continue;

nojb · 2022-08-23T04:58:36Z

src/winwatch/winwatch.mli

+(** return None if the file does not exit *)
+val create : string -> f:(Event.t -> string -> unit) -> t option


Suggested change

(** return None if the file does not exit *)

val create : string -> f:(Event.t -> string -> unit) -> t option

val create : string -> f:(Event.t -> string -> unit) -> t option

(** Returns [None] if the file does not exit. *)

nojb · 2022-08-23T05:02:06Z

src/csexp_rpc/csexp_rpc.ml

+      | [], [], [] ->
+        (* Timeout *)
+        if peek_named_pipe t.r_interrupt_accept then (
+          print_endline "data available!";


Left-over debug code.

Suggested change

print_endline "data available!";

nojb · 2022-08-23T05:05:23Z

The DCO check is failing because your git "Author" name (git config user.name) must match your Signed-Off-by line, ie it must be Uma Kothuri. To change it:

git config user.name "Uma Kothuri";
git commit --reset-author --amend (to update the existing commit at the top of the branch with the new name).

emillon · 2022-09-09T13:11:06Z

src/dune_file_watcher/dune_file_watcher.ml

@@ -575,6 +587,88 @@ let create_fsevents ?(latency = 0.2) ~(scheduler : Scheduler.t) () =
  ; sync_table
  }

+let winwatch_patterns =
+  List.rev_map ~f:Re.Str.regexp_case_fold
+    [ {|^_opam|}


Some of this list is already present in exclude_patterns in this file.

Moreover, there's duplication within this list. I'd like to see this regex defined in a non redundant way.

emillon · 2022-09-09T13:13:02Z

src/dune_file_watcher/dune_file_watcher.ml

+  scheduler.spawn_thread (fun () ->
+      match Winwatch.Iocp.run iocp with
+      | Ok () -> ()
+      | Error _ -> (* Winwatch.Iocp.run never returns *) assert false);


What are exactly the semantics of run? It looks like we could make it return _, or some kind of empty type.

emillon · 2022-09-09T13:13:38Z

src/dune_file_watcher/dune_file_watcher.ml

@@ -590,11 +684,12 @@ let create_default ?fsevents_debounce ~scheduler () =
      ~debounce_interval:(Some 0.5 (* seconds *)) ~backend
  | `Fsevents -> create_fsevents ?latency:fsevents_debounce ~scheduler ()
  | `Inotify_lib -> create_inotifylib ~scheduler
+  | `Winwatch -> create_winwatch ~scheduler ~debounce_interval:0.5


let's group this debounce_interval constant with the fswatch one in a module global.

emillon · 2022-09-09T13:26:57Z

src/winwatch/winwatch_stubs.c

+                    break;
+            }
+
+            file_name = malloc(sizeof(WCHAR) * (name_len + 1));


file_name is not freed (it can be right after it's been copied in v_file_name)

emillon · 2022-09-09T13:27:49Z

src/winwatch/winwatch_stubs.c

+            file_name = malloc(sizeof(WCHAR) * (name_len + 1));
+            memcpy(file_name, event->FileName, sizeof(WCHAR) * name_len);
+            file_name[name_len] = 0;
+            v_file_name = caml_copy_string_of_utf16(file_name);


agree, you can allocate and copy directly into the ocaml value (if that's what you meant @nojb ?). no need to nul-terminate this way.

emillon · 2022-09-09T13:30:11Z

src/winwatch/winwatch_stubs.c

+    t->overlapped = caml_stat_alloc(sizeof(OVERLAPPED));
+    t->handle = handle;
+
+    caml_register_generational_global_root(&t->func);


is this necessary? this is in t which is returned through v_res.

Signed-off-by: Uma Kothuri <[email protected]> Signed-off-by: Nicolás Ojeda Bär <[email protected]>

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

rgrinberg · 2022-10-12T14:37:40Z

@nojb could you move the RPC changes to a separate PR?

rgrinberg · 2022-10-12T14:39:14Z

src/winwatch/winwatch.mli

@@ -0,0 +1,25 @@
+module Iocp : sig


Could you add some docs to this module?

rgrinberg · 2022-10-12T14:41:14Z

src/dune_file_watcher/dune_file_watcher.ml

@@ -583,6 +595,88 @@ let create_fsevents ?(latency = 0.2) ~(scheduler : Scheduler.t) () =
  ; sync_table
  }

+let winwatch_patterns =
+  List.rev_map ~f:Re.Str.regexp_case_fold


Can you switch from Re.Str to Re combinators?

nojb · 2022-10-12T14:44:23Z

Thanks for the review @rgrinberg. I am in the process of rebasing it (I started a few days ago, but didn't get enough time to finish). Will amend according to your suggestions.

nojb · 2023-02-06T00:47:05Z

Superceded by #7010

nojb reviewed Aug 23, 2022

View reviewed changes

nojb mentioned this pull request Aug 23, 2022

fix(fsevents): remove build dir notifications #6094

Merged

yams-yams force-pushed the winwatch branch from 4292be0 to 491373e Compare August 23, 2022 14:14

emillon reviewed Sep 9, 2022

View reviewed changes

Add native polling mode support on Windows

d9a52b3

Signed-off-by: Uma Kothuri <[email protected]> Signed-off-by: Nicolás Ojeda Bär <[email protected]>

nojb force-pushed the winwatch branch from 491373e to d9a52b3 Compare October 9, 2022 22:19

nojb added 4 commits October 10, 2022 00:20

Remove debug

3aeec6d

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

Remove unneeded header

d186ab8

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

_

6494632

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

_

36005df

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

rgrinberg reviewed Oct 12, 2022

View reviewed changes

src/winwatch/winwatch.mli

@@ -0,0 +1,25 @@

module Iocp : sig

Copy link

Member

rgrinberg Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some docs to this module?

rgrinberg reviewed Oct 12, 2022

View reviewed changes

rgrinberg marked this pull request as draft October 19, 2022 23:46

nojb mentioned this pull request Nov 18, 2022

Segfault on windows ocaml/ocaml-lsp#929

Closed

mlasson mentioned this pull request Nov 21, 2022

Mitigate ocaml/ocaml#11737 (windows-only) ocaml/ocaml-lsp#935

Merged

rgrinberg added the windows label Dec 18, 2022

rgrinberg requested a review from jonahbeckford December 18, 2022 18:37

nojb mentioned this pull request Feb 6, 2023

Add native polling mode support on Windows #7010

Merged

nojb closed this Feb 6, 2023

nojb deleted the winwatch branch February 6, 2023 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds native polling mode support on Windows #6087

Adds native polling mode support on Windows #6087

yams-yams commented Aug 18, 2022

nojb commented Aug 18, 2022

nojb left a comment

nojb Aug 19, 2022

nojb Aug 23, 2022

nojb Aug 23, 2022

nojb Aug 23, 2022

nojb Aug 23, 2022

nojb Aug 23, 2022

nojb Aug 23, 2022

emillon Sep 9, 2022

nojb Aug 23, 2022

nojb Aug 23, 2022

nojb Aug 23, 2022

nojb commented Aug 23, 2022 •

edited

Loading

emillon Sep 9, 2022

rgrinberg Oct 12, 2022

emillon Sep 9, 2022

emillon Sep 9, 2022

emillon Sep 9, 2022

emillon Sep 9, 2022

emillon Sep 9, 2022

rgrinberg commented Oct 12, 2022

rgrinberg Oct 12, 2022

rgrinberg Oct 12, 2022

nojb commented Oct 12, 2022

nojb commented Feb 6, 2023 •

edited

Loading

	#include <caml/threads.h>
	#include <caml/unixsupport.h>
	#include <caml/custom.h>

		(** return None if the file does not exit *)
		val create : string -> f:(Event.t -> string -> unit) -> t option

Adds native polling mode support on Windows #6087

Adds native polling mode support on Windows #6087

Conversation

yams-yams commented Aug 18, 2022

nojb commented Aug 18, 2022

nojb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojb commented Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Oct 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojb commented Oct 12, 2022

nojb commented Feb 6, 2023 • edited Loading

nojb commented Aug 23, 2022 •

edited

Loading

nojb commented Feb 6, 2023 •

edited

Loading