Fast git status via a file system watcher

Add a test utility (test-drop-caches) that enables dropping the file system cache on Windows.

Add a perf test (p7519-fsmonitor.sh) for fsmonitor.

Signed-off-by Ben Peart <[email protected]>

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

---
  Makefile                    |   1 +
  t/helper/test-drop-caches.c | 107 +++++++++++++++++++++++++++++
  t/perf/p7519-fsmonitor.sh   | 161 ++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 269 insertions(+)

create mode 100644 t/helper/test-drop-caches.c
create mode 100755 t/perf/p7519-fsmonitor.sh

Makefile +1 -0

index 992dd58801..893947839f 100644
@@ -648,6 +648,7 @@ TEST_PROGRAMS_NEED_X += test-subprocess
  TEST_PROGRAMS_NEED_X += test-svn-fe
  TEST_PROGRAMS_NEED_X += test-urlmatch-normalization
  TEST_PROGRAMS_NEED_X += test-wildmatch
+TEST_PROGRAMS_NEED_X += test-drop-caches
 
  TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
 

t/helper/test-drop-caches.c new file +107 -0

new file mode 100644
index 0000000000..80830d920b
@@ -0,0 +1,107 @@
+#include "git-compat-util.h"
+#include <stdio.h>
+
+typedef DWORD NTSTATUS;
+
+#ifdef GIT_WINDOWS_NATIVE
+#include <tchar.h>
+
+#define STATUS_SUCCESS			(0x00000000L)
+#define STATUS_PRIVILEGE_NOT_HELD	(0xC0000061L)
+
+typedef enum _SYSTEM_INFORMATION_CLASS {
+	SystemMemoryListInformation = 80, // 80, q: SYSTEM_MEMORY_LIST_INFORMATION; s: SYSTEM_MEMORY_LIST_COMMAND (requires SeProfileSingleProcessPrivilege)
+} SYSTEM_INFORMATION_CLASS;
+
+// private
+typedef enum _SYSTEM_MEMORY_LIST_COMMAND
+{
+	MemoryCaptureAccessedBits,
+	MemoryCaptureAndResetAccessedBits,
+	MemoryEmptyWorkingSets,
+	MemoryFlushModifiedList,
+	MemoryPurgeStandbyList,
+	MemoryPurgeLowPriorityStandbyList,
+	MemoryCommandMax
+} SYSTEM_MEMORY_LIST_COMMAND;
+
+BOOL GetPrivilege(HANDLE TokenHandle, LPCSTR lpName, int flags)
+{
+	BOOL bResult;
+	DWORD dwBufferLength;
+	LUID luid;
+	TOKEN_PRIVILEGES tpPreviousState;
+	TOKEN_PRIVILEGES tpNewState;
+
+	dwBufferLength = 16;
+	bResult = LookupPrivilegeValueA(0, lpName, &luid);
+	if (bResult)
+	{
+		tpNewState.PrivilegeCount = 1;
+		tpNewState.Privileges[0].Luid = luid;
+		tpNewState.Privileges[0].Attributes = 0;
+		bResult = AdjustTokenPrivileges(TokenHandle, 0, &tpNewState, (DWORD)((LPBYTE)&(tpNewState.Privileges[1]) - (LPBYTE)&tpNewState), &tpPreviousState, &dwBufferLength);
+		if (bResult)
+		{
+			tpPreviousState.PrivilegeCount = 1;
+			tpPreviousState.Privileges[0].Luid = luid;
+			tpPreviousState.Privileges[0].Attributes = flags != 0 ? 2 : 0;
+			bResult = AdjustTokenPrivileges(TokenHandle, 0, &tpPreviousState, dwBufferLength, 0, 0);
+		}
+	}
+	return bResult;
+}
+#endif
+
+int cmd_main(int argc, const char **argv)
+{
+	NTSTATUS status = 1;
+#ifdef GIT_WINDOWS_NATIVE
+	HANDLE hProcess = GetCurrentProcess();
+	HANDLE hToken;
+	if (!OpenProcessToken(hProcess, TOKEN_QUERY | TOKEN_ADJUST_PRIVILEGES, &hToken))
+	{
+		_ftprintf(stderr, _T("Can't open current process token\n"));
+		return 1;
+	}
+
+	if (!GetPrivilege(hToken, "SeProfileSingleProcessPrivilege", 1))
+	{
+		_ftprintf(stderr, _T("Can't get SeProfileSingleProcessPrivilege\n"));
+		return 1;
+	}
+
+	CloseHandle(hToken);
+
+	HMODULE ntdll = LoadLibrary(_T("ntdll.dll"));
+	if (!ntdll)
+	{
+		_ftprintf(stderr, _T("Can't load ntdll.dll, wrong Windows version?\n"));
+		return 1;
+	}
+
+	NTSTATUS(WINAPI *NtSetSystemInformation)(INT, PVOID, ULONG) = (NTSTATUS(WINAPI *)(INT, PVOID, ULONG))GetProcAddress(ntdll, "NtSetSystemInformation");
+	if (!NtSetSystemInformation)
+	{
+		_ftprintf(stderr, _T("Can't get function addresses, wrong Windows version?\n"));
+		return 1;
+	}
+
+	SYSTEM_MEMORY_LIST_COMMAND command = MemoryPurgeStandbyList;
+	status = NtSetSystemInformation(
+		SystemMemoryListInformation,
+		&command,
+		sizeof(SYSTEM_MEMORY_LIST_COMMAND)
+	);
+	if (status == STATUS_PRIVILEGE_NOT_HELD)
+	{
+		_ftprintf(stderr, _T("Insufficient privileges to execute the memory list command"));
+	}
+	else if (status != STATUS_SUCCESS)
+	{
+		_ftprintf(stderr, _T("Unable to execute the memory list command %lX"), status);
+	}
+#endif
+
+	return status;
+}

t/perf/p7519-fsmonitor.sh new file +161 -0

new file mode 100755
index 0000000000..e41905cb9b
@@ -0,0 +1,161 @@
+#!/bin/sh
+
+test_description="Test core.fsmonitor"
+
+. ./perf-lib.sh
+
+# This has to be run with GIT_PERF_REPEAT_COUNT=1 to generate valid results.
+# Otherwise the caching that happens for the nth run will negate the validity
+# of the comparisons.
+if [ "$GIT_PERF_REPEAT_COUNT" -ne 1 ]
+then
+	echo "warning: This test must be run with GIT_PERF_REPEAT_COUNT=1 to generate valid results." >&2
+	echo "warning: Setting GIT_PERF_REPEAT_COUNT=1" >&2
+	GIT_PERF_REPEAT_COUNT=1
+fi
+
+test_perf_large_repo
+test_checkout_worktree
+
+# Convert unix style paths to what Watchman expects
+case "$(uname -s)" in
+MINGW*|MSYS_NT*)
+  GIT_WORK_TREE="$(cygpath -aw "$PWD" | sed 's,\\,/,g')"
+  ;;
+*)
+  GIT_WORK_TREE="$PWD"
+  ;;
+esac
+
+# The big win for using fsmonitor is the elimination of the need to scan
+# the working directory looking for changed files and untracked files. If
+# the file information is all cached in RAM, the benefits are reduced.
+
+flush_disk_cache () {
+	case "$(uname -s)" in
+	MINGW*|MSYS_NT*)
+	  sync && test-drop-caches
+	  ;;
+	*)
+	  sudo sync && echo 3 | sudo tee /proc/sys/vm/drop_caches
+	  ;;
+	esac
+
+}
+
+test_lazy_prereq UNTRACKED_CACHE '
+	{ git update-index --test-untracked-cache; ret=$?; } &&
+	test $ret -ne 1
+'
+
+test_expect_success "setup" '
+	# Maybe set untrackedCache & splitIndex depending on the environment
+	if test -n "$GIT_PERF_7519_UNTRACKED_CACHE"
+	then
+		git config core.untrackedCache "$GIT_PERF_7519_UNTRACKED_CACHE"
+	else
+		if test_have_prereq UNTRACKED_CACHE
+		then
+			git config core.untrackedCache true
+		else
+			git config core.untrackedCache false
+		fi
+	fi &&
+
+	if test -n "$GIT_PERF_7519_SPLIT_INDEX"
+	then
+		git config core.splitIndex "$GIT_PERF_7519_SPLIT_INDEX"
+	fi &&
+
+	# Hook scaffolding
+	mkdir .git/hooks &&
+	cp ../../../templates/hooks--query-fsmonitor.sample .git/hooks/query-fsmonitor &&
+
+	# have Watchman monitor the test folder
+	watchman watch "$GIT_WORK_TREE" &&
+	watchman watch-list | grep -q -F "$GIT_WORK_TREE"
+'
+
+# Worst case without fsmonitor
+test_expect_success "clear fs cache" '
+	git config core.fsmonitor false &&
+	flush_disk_cache
+'
+test_perf "status (fsmonitor=false, cold fs cache)" '
+	git status
+'
+
+# Best case without fsmonitor
+test_perf "status (fsmonitor=false, warm fs cache)" '
+	git status
+'
+
+# Let's see if -uno & -uall make any difference
+test_expect_success "clear fs cache" '
+	flush_disk_cache
+'
+test_perf "status -uno (fsmonitor=false, cold fs cache)" '
+	git status -uno
+'
+
+test_expect_success "clear fs cache" '
+	flush_disk_cache
+'
+test_perf "status -uall (fsmonitor=false, cold fs cache)" '
+	git status -uall
+'
+
+# The first run with core.fsmonitor=true has to do a normal scan and write
+# out the index extension.
+test_expect_success "populate extension" '
+	# core.preloadIndex defeats the benefits of core.fsMonitor as it
+	# calls lstat for the index entries. Turn it off as _not_ doing
+	# the work is faster than doing the work across multiple threads.
+	git config core.fsmonitor true &&
+	git config core.preloadIndex false &&
+	git status
+'
+
+# Worst case with fsmonitor
+test_expect_success "shutdown fsmonitor, clear fs cache" '
+	watchman shutdown-server &&
+	flush_disk_cache
+'
+test_perf "status (fsmonitor=true, cold fs cache, cold fsmonitor)" '
+	git status
+'
+
+# Best case with fsmonitor
+test_perf "status (fsmonitor=true, warm fs cache, warm fsmonitor)" '
+	git status
+'
+
+# Best improved with fsmonitor (compare to worst case without fsmonitor)
+test_expect_success "clear fs cache" '
+	flush_disk_cache
+'
+test_perf "status (fsmonitor=true, cold fs cache, warm fsmonitor)" '
+	git status
+'
+
+# Let's see if -uno & -uall make any difference
+test_expect_success "clear fs cache" '
+	flush_disk_cache
+'
+test_perf "status -uno (fsmonitor=true, cold fs cache)" '
+	git status -uno
+'
+
+test_expect_success "clear fs cache" '
+	flush_disk_cache
+'
+test_perf "status -uall (fsmonitor=true, cold fs cache)" '
+	git status -uall
+'
+
+test_expect_success "cleanup" '
+	watchman watch-del "$GIT_WORK_TREE" &&
+	watchman shutdown-server
+'
+
+test_done

t/helper/test-drop-caches.c new file +4 -0

+} SYSTEM_MEMORY_LIST_COMMAND;
+
+BOOL GetPrivilege(HANDLE TokenHandle, LPCSTR lpName, int flags)
+{

t/helper/test-drop-caches.c new file +34 -0

+		tpNewState.PrivilegeCount = 1;
+		tpNewState.Privileges[0].Luid = luid;
+		tpNewState.Privileges[0].Attributes = 0;
+		bResult = AdjustTokenPrivileges(TokenHandle, 0, &tpNewState, (DWORD)((LPBYTE)&(tpNewState.Privileges[1]) - (LPBYTE)&tpNewState), &tpPreviousState, &dwBufferLength);
+		if (bResult)
+		{
+			tpPreviousState.PrivilegeCount = 1;
+			tpPreviousState.Privileges[0].Luid = luid;
+			tpPreviousState.Privileges[0].Attributes = flags != 0 ? 2 : 0;
+			bResult = AdjustTokenPrivileges(TokenHandle, 0, &tpPreviousState, dwBufferLength, 0, 0);
+		}
+	}
+	return bResult;
+}
+#endif
+
+int cmd_main(int argc, const char **argv)
+{
+	NTSTATUS status = 1;
+#ifdef GIT_WINDOWS_NATIVE
+	HANDLE hProcess = GetCurrentProcess();
+	HANDLE hToken;
+	if (!OpenProcessToken(hProcess, TOKEN_QUERY | TOKEN_ADJUST_PRIVILEGES, &hToken))
+	{
+		_ftprintf(stderr, _T("Can't open current process token\n"));
+		return 1;
+	}
+
+	if (!GetPrivilege(hToken, "SeProfileSingleProcessPrivilege", 1))
+	{
+		_ftprintf(stderr, _T("Can't get SeProfileSingleProcessPrivilege\n"));
+		return 1;
+	}
+

t/helper/test-drop-caches.c new file +14 -0

+	if (!NtSetSystemInformation)
+	{
+		_ftprintf(stderr, _T("Can't get function addresses, wrong Windows version?\n"));
+		return 1;
+	}
+
+	SYSTEM_MEMORY_LIST_COMMAND command = MemoryPurgeStandbyList;
+	status = NtSetSystemInformation(
+		SystemMemoryListInformation,
+		&command,
+		sizeof(SYSTEM_MEMORY_LIST_COMMAND)
+	);
+	if (status == STATUS_PRIVILEGE_NOT_HELD)
+	{

t/perf/p7519-fsmonitor.sh new file +1 -0

new file mode 100755
index 0000000000..e41905cb9b
@@ -0,0 +1,161 @@
+#!/bin/sh

t/helper/test-drop-caches.c line 2

Quoted 8 lines · Ben Peart

t/helper/test-drop-caches.c new file +2 -0

new file mode 100644
index 0000000000..80830d920b
@@ -0,0 +1,107 @@
+#include "git-compat-util.h"
+#include <stdio.h>

I thought compat-util should already include stdio?

t/helper/test-drop-caches.c line 4

Quoted 2 lines

t/helper/test-drop-caches.c new file +2 -0

+
+typedef DWORD NTSTATUS;

Is this safe to have outside "#ifdef GIT_WINDOWS_NATIVE"?

t/helper/test-drop-caches.c line 13

Quoted 10 lines

t/helper/test-drop-caches.c new file +9 -0

+
+#ifdef GIT_WINDOWS_NATIVE
+#include <tchar.h>
+
+#define STATUS_SUCCESS			(0x00000000L)
+#define STATUS_PRIVILEGE_NOT_HELD	(0xC0000061L)
+
+typedef enum _SYSTEM_INFORMATION_CLASS {
+	SystemMemoryListInformation = 80, // 80, q: SYSTEM_MEMORY_LIST_INFORMATION; s: SYSTEM_MEMORY_LIST_COMMAND (requires SeProfileSingleProcessPrivilege)

I would have said "Please avoid // comment in this codebase unless we know we only use the compilers that grok it". This particular one may be OK, as it is inside GIT_WINDOWS_NATIVE and I assume everybody on that platform uses recent GCC (or VStudio groks it I guess)?

t/helper/test-drop-caches.c line 18

Quoted 5 lines

t/helper/test-drop-caches.c new file +5 -0

+} SYSTEM_INFORMATION_CLASS;
+
+// private
+typedef enum _SYSTEM_MEMORY_LIST_COMMAND
+{

Style: '{' comes at the end of the previous line, with a single SP immediately before it, unless it is the beginning of the function body.

What you did for _SYSTEM_INFORMATION_CLASS above is correct.

t/helper/test-drop-caches.c line 25

Quoted 7 lines

t/helper/test-drop-caches.c new file +7 -0

+	MemoryCaptureAccessedBits,
+	MemoryCaptureAndResetAccessedBits,
+	MemoryEmptyWorkingSets,
+	MemoryFlushModifiedList,
+	MemoryPurgeStandbyList,
+	MemoryPurgeLowPriorityStandbyList,
+	MemoryCommandMax

Style: avoid CamelCase.

So status is initialized to 1 and anybody without GIT_WINDOWS_NATIVE unconditionally get exit(1)?

Given that 'status' is the return value of this function that returns 'int', I wonder if we need NTSTATUS type here.

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

Quoted 8 lines

t/perf/p7519-fsmonitor.sh new file +1 -0

new file mode 100755
index 0000000000..e41905cb9b
@@ -0,0 +1,161 @@
+#!/bin/sh

t/perf/p7519-fsmonitor.sh line 11

Quoted 10 lines

t/perf/p7519-fsmonitor.sh new file +10 -0

+
+test_description="Test core.fsmonitor"
+
+. ./perf-lib.sh
+
+# This has to be run with GIT_PERF_REPEAT_COUNT=1 to generate valid results.
+# Otherwise the caching that happens for the nth run will negate the validity
+# of the comparisons.
+if [ "$GIT_PERF_REPEAT_COUNT" -ne 1 ]
+then

Style:

if test "$GIT_PERF_REPEAT_COUNT" -ne 1
then

t/perf/p7519-fsmonitor.sh line 72

Quoted 6 lines

t/perf/p7519-fsmonitor.sh new file +6 -0

+ ...
+test_expect_success "setup" '
+...
+	# Hook scaffolding
+	mkdir .git/hooks &&
+	cp ../../../templates/hooks--query-fsmonitor.sample .git/hooks/query-fsmonitor &&

Does this assume $TRASH_DIRECTORY must be in $TEST_DIRECTORY/perf/? Shouldn't t/perf/p[0-9][0-9][0-9][0-9]-*.sh scripts be capable of running with the --root=<ramdisk> option? Perhaps take the copy relative to $TEST_DIRECTORY?

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

fsmonitor.c new file +14 -0

+       index += sizeof(uint32_t);
+
+       istate->fsmonitor_dirty = ewah_new();
+       ret = ewah_read_mmap(istate->fsmonitor_dirty, index, ewah_size);
+       if (ret != ewah_size) {
+               ewah_free(istate->fsmonitor_dirty);
+               istate->fsmonitor_dirty = NULL;
+               return error("failed to parse ewah bitmap reading fsmonitor index extension");
+       }
+
+       return 0;
+}
+
+void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)

fsmonitor.c new file +4 -0

+       strbuf_add(sb, &tm, sizeof(uint64_t));
+       fixup = sb->len;
+       strbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */
+

fsmonitor.c new file +11 -0

+       memcpy(sb->buf + fixup, &ewah_size, sizeof(uint32_t));
+}
 
+/*
+ * Call the query-fsmonitor hook passing the time of the last saved results.
+ */
+static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
+{
+       struct child_process cp = CHILD_PROCESS_INIT;
+       char ver[64];
+       char date[64];
+       const char *argv[4];

fsmonitor.c new file +23 -0

+       cp.out = -1;
+
+       return capture_command(&cp, query_result, 1024);
+}
+
+static void mark_file_dirty(struct index_state *istate, const char *name)
+{
+       struct untracked_cache_dir *dir;
+       int pos;
+
+       /* find it in the index and mark that entry as dirty */
+       pos = index_name_pos(istate, name, strlen(name));
+       if (pos >= 0) {
+               if (!(istate->cache[pos]->ce_flags & CE_FSMONITOR_DIRTY)) {
+                       istate->cache[pos]->ce_flags |= CE_FSMONITOR_DIRTY;
+                       istate->cache_changed |= FSMONITOR_CHANGED;
+               }
+       }
+
+       /*
+        * Find the corresponding directory in the untracked cache
+        * and mark it as invalid
+        */

fsmonitor.c new file +29 -0

+}
+
+void refresh_by_fsmonitor(struct index_state *istate)
+{
+       static int has_run_once = 0;
+       struct strbuf query_result = STRBUF_INIT;
+       int query_success = 0;
+       size_t bol = 0; /* beginning of line */
+       uint64_t last_update;
+       char *buf, *entry;
+       int i;
+
+       if (!core_fsmonitor || has_run_once)
+               return;
+       has_run_once = 1;
+
+       /*
+        * This could be racy so save the date/time now and the hook
+        * should be inclusive to ensure we don't miss potential changes.
+        */
+       last_update = getnanotime();
+
+       /*
+        * If we have a last update time, call query-monitor for the set of
+        * changes since that time.
+        */
+       if (istate->fsmonitor_last_update) {
+               query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,
+                       istate->fsmonitor_last_update, &query_result);

fsmonitor.c new file +7 -0

+               /* Mark all clean entries up-to-date */
+               for (i = 0; i < istate->cache_nr; i++) {
+                       struct cache_entry *ce = istate->cache[i];
+                       if (ce_stage(ce) || (ce->ce_flags & CE_FSMONITOR_DIRTY))
+                               continue;
+                       ce_mark_uptodate(ce);
+               }

t/t7519-status-fsmonitor.sh new file +4 -0

+# fsmonitor works correctly with or without the untracked cache
+# but if it is available, we'll turn it on to ensure we test that
+# codepath as well.
+

Changes from V4 include:

fsmonitor.c line 22

Quoted 12 lines · Ben Peart

fsmonitor.c new file +12 -0

+int read_fsmonitor_extension(struct index_state *istate, const void *data,
+       unsigned long sz)
+{
+       const char *index = data;
+       uint32_t hdr_version;
+       uint32_t ewah_size;
+       int ret;
+
+       if (sz < sizeof(uint32_t) + sizeof(uint64_t) + sizeof(uint32_t))
+               return error("corrupt fsmonitor extension (too short)");
+
+       hdr_version = get_be32(index);

Here we use get_be32(), ...

fsmonitor.c line 27

Quoted 5 lines

fsmonitor.c new file +5 -0

+       index += sizeof(uint32_t);
+       if (hdr_version != INDEX_EXTENSION_VERSION)
+               return error("bad fsmonitor version %d", hdr_version);
+
+       istate->fsmonitor_last_update = get_be64(index);

...get_be64(), ...

fsmonitor.c line 30

Quoted 3 lines

fsmonitor.c new file +3 -0

+       index += sizeof(uint64_t);
+
+       ewah_size = get_be32(index);

... and get_be32 again, ...

Quoted 14 lines

fsmonitor.c new file +14 -0

+       index += sizeof(uint32_t);
+
+       istate->fsmonitor_dirty = ewah_new();
+       ret = ewah_read_mmap(istate->fsmonitor_dirty, index, ewah_size);
+       if (ret != ewah_size) {
+               ewah_free(istate->fsmonitor_dirty);
+               istate->fsmonitor_dirty = NULL;
+               return error("failed to parse ewah bitmap reading fsmonitor index extension");
+       }
+
+       return 0;
+}
+
+void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)

fsmonitor.c line 54

Quoted 10 lines

fsmonitor.c new file +10 -0

+{
+       uint32_t hdr_version;
+       uint64_t tm;
+       struct ewah_bitmap *bitmap;
+       int i;
+       uint32_t ewah_start;
+       uint32_t ewah_size = 0;
+       int fixup = 0;
+
+       hdr_version = htonl(INDEX_EXTENSION_VERSION);

... but here we use htonl() instead of put_be32(), ...

fsmonitor.c line 57

Quoted 3 lines

fsmonitor.c new file +3 -0

+       strbuf_add(sb, &hdr_version, sizeof(uint32_t));
+
+       tm = htonll((uint64_t)istate->fsmonitor_last_update);

... htonll(), ...

Quoted 4 lines

fsmonitor.c new file +4 -0

+       strbuf_add(sb, &tm, sizeof(uint64_t));
+       fixup = sb->len;
+       strbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */
+

fsmonitor.c line 71

Quoted 10 lines

fsmonitor.c new file +10 -0

+       ewah_start = sb->len;
+       bitmap = ewah_new();
+       for (i = 0; i < istate->cache_nr; i++)
+               if (istate->cache[i]->ce_flags & CE_FSMONITOR_DIRTY)
+                       ewah_set(bitmap, i);
+       ewah_serialize_strbuf(bitmap, sb);
+       ewah_free(bitmap);
+
+       /* fix up size field */
+       ewah_size = htonl(sb->len - ewah_start);

... and htonl() again.

It would be more consistent (and perhaps more correct) to use put_beXX() functions, instead of the htonl() and htonll() functions.

Quoted 17 lines

fsmonitor.c new file +15 -0

+       memcpy(sb->buf + fixup, &ewah_size, sizeof(uint32_t));
+}
 
+/*
+ * Call the query-fsmonitor hook passing the time of the last saved results.
+ */
+static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
+{
+       struct child_process cp = CHILD_PROCESS_INIT;
+       char ver[64];
+       char date[64];
+       const char *argv[4];
+
+       if (!(argv[0] = find_hook("query-fsmonitor")))
+               return -1;
+

fsmonitor.c line 155

Quoted 6 lines

fsmonitor.c new file +6 -0

+       snprintf(ver, sizeof(version), "%d", version);
+       snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
+       argv[1] = ver;
+       argv[2] = date;
+       argv[3] = NULL;
+       cp.argv = argv;

Maybe it would be nicer using the argv_array_pushX() functions.

Quoted 29 lines

fsmonitor.c new file +28 -0

+       cp.out = -1;
+
+       return capture_command(&cp, query_result, 1024);
+}
+
+static void mark_file_dirty(struct index_state *istate, const char *name)
+{
+       struct untracked_cache_dir *dir;
+       int pos;
+
+       /* find it in the index and mark that entry as dirty */
+       pos = index_name_pos(istate, name, strlen(name));
+       if (pos >= 0) {
+               if (!(istate->cache[pos]->ce_flags & CE_FSMONITOR_DIRTY)) {
+                       istate->cache[pos]->ce_flags |= CE_FSMONITOR_DIRTY;
+                       istate->cache_changed |= FSMONITOR_CHANGED;
+               }
+       }
+
+       /*
+        * Find the corresponding directory in the untracked cache
+        * and mark it as invalid
+        */
+       if (!istate->untracked || !istate->untracked->root)
+               return;
+
+       dir = find_untracked_cache_dir(istate->untracked, istate->untracked->root, name);
+       if (dir) {

fsmonitor.c line 188

Quoted 5 lines

fsmonitor.c new file +5 -0

+               if (dir->valid) {
+                       dir->valid = 0;
+                       istate->cache_changed |= FSMONITOR_CHANGED;
+               }
+       }

The code above is quite similar as what is in mark_fsmonitor_dirty(), so I wonder if a refactoring is possible.

Quoted 30 lines

fsmonitor.c new file +29 -0

+}
+
+void refresh_by_fsmonitor(struct index_state *istate)
+{
+       static int has_run_once = 0;
+       struct strbuf query_result = STRBUF_INIT;
+       int query_success = 0;
+       size_t bol = 0; /* beginning of line */
+       uint64_t last_update;
+       char *buf, *entry;
+       int i;
+
+       if (!core_fsmonitor || has_run_once)
+               return;
+       has_run_once = 1;
+
+       /*
+        * This could be racy so save the date/time now and the hook
+        * should be inclusive to ensure we don't miss potential changes.
+        */
+       last_update = getnanotime();
+
+       /*
+        * If we have a last update time, call query-monitor for the set of
+        * changes since that time.
+        */
+       if (istate->fsmonitor_last_update) {
+               query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,
+                       istate->fsmonitor_last_update, &query_result);

fsmonitor.c line 227

Quoted 10 lines

fsmonitor.c new file +10 -0

+               trace_performance_since(last_update, "query-fsmonitor");
+       }
+
+       if (query_success) {
+               /* Mark all entries returned by the monitor as dirty */
+               buf = entry = query_result.buf;
+               for (i = 0; i < query_result.len; i++) {
+                       if (buf[i] != '\0')
+                               continue;
+                       mark_file_dirty(istate, buf + bol);

It looks like bol is always equal to i here...

fsmonitor.c line 231

Quoted 4 lines

fsmonitor.c new file +4 -0

+                       bol = i + 1;
+               }
+               if (bol < query_result.len)
+                       mark_file_dirty(istate, buf + bol);

... and here too. As it is not used below, I wonder if you really need the bol variable.

Quoted 8 lines

fsmonitor.c new file +7 -0

+               /* Mark all clean entries up-to-date */
+               for (i = 0; i < istate->cache_nr; i++) {
+                       struct cache_entry *ce = istate->cache[i];
+                       if (ce_stage(ce) || (ce->ce_flags & CE_FSMONITOR_DIRTY))
+                               continue;
+                       ce_mark_uptodate(ce);
+               }

Quoted 8 lines · Ben Peart

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, with sections protected by "#ifdef LINUX", "#ifdef WINDOWS", etc. That way, the platform detection and switching between running this program and echoing something into /proc filesystem performed in p7519 can be removed (this test-helper program will be responsible to implement that logic instead). When p7519 wants to drop the filesystem cache, regardless of the platform, it can call this test-helper without having to know how the filesystem cache is dropped.

I do not have strong preference either way, but I have a slight suspicion that the "another approach" above may give us a better result. For one thing, the test-helper can be reused in new perf scripts people will write in the future.

Thanks.

Quoted 11 lines · Junio C Hamano

Quoted 8 lines · Ben Peart

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

Quoted 8 lines

If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, ...

Quoted 4 lines · Ben Peart

t/t7519-status-fsmonitor.sh new file +4 -0

+# fsmonitor works correctly with or without the untracked cache
+# but if it is available, we'll turn it on to ensure we test that
+# codepath as well.
+

t/t7519-status-fsmonitor.sh line 44

Quoted 10 lines

t/t7519-status-fsmonitor.sh new file +10 -0

+test_lazy_prereq UNTRACKED_CACHE '
+       { git update-index --test-untracked-cache; ret=$?; } &&
+       test $ret -ne 1
+'
+
+if test_have_prereq UNTRACKED_CACHE; then
+       git config core.untrackedcache true
+else
+       git config core.untrackedcache false
+fi

I wonder if it would be better to just do something like:

=====================

test_expect_success 'setup' '

         ....

'

uc_values="false"
test_have_prereq UNTRACKED_CACHE && uc_values="false true"

for uc_val in $uc_values
do

     test_expect_success "setup untracked cache to $uc_val" '
          git config core.untrackedcache $uc_val
     '

     test_expect_success 'refresh_index() invalidates fsmonitor cache' '
           ...
     '

     test_expect_success "status doesn't detect unreported modifications" '
           ...
     '

...

done

=====================

Quoted 12 lines · Ben Peart

Quoted 11 lines · Junio C Hamano

Quoted 8 lines · Ben Peart

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

Quoted 8 lines

Quoted 8 lines

If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, ...

... and having said all that, I think it is perfectly fine to do such a clean-up long after the series gets more exposure to wider audiences as a follow-up patch. Let's get the primary part that affects people's everyday use of Git right and then worry about the test details later.

A quick show of hands to the list audiences. How many of you guys actually tried this series on 'pu' and checked to see its performance (and correctness ;-) characteristics?

Do you folks like it? Rather not have such complexity in the core part of the system? A good first step to start adding more performance improvements? No opinion?

-----Original Message-----
From: Junio C Hamano [mailto:[email protected]] On Behalf Of Junio C Hamano
Sent: Friday, July 7, 2017 2:35 PM
To: Ben Peart <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; David Turner <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5 7/7] fsmonitor: add a performance test

Quoted 12 lines · Ben Peart

Quoted 11 lines · Junio C Hamano

Quoted 8 lines · Ben Peart

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

Quoted 8 lines

Quoted 8 lines

If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, ...

... and having said all that, I think it is perfectly fine to do such a clean-up long after the series gets more exposure to wider audiences as a follow-up patch. Let's get the primary part that affects people's everyday use of Git right and then worry about the test details later.

A quick show of hands to the list audiences. How many of you guys actually tried this series on 'pu' and checked to see its performance (and correctness ;-) characteristics?

Do you folks like it? Rather not have such complexity in the core part of the system? A good first step to start adding more performance improvements? No opinion?

Quoted 12 lines · Ben Peart

Quoted 11 lines · Junio C Hamano

Quoted 8 lines · Ben Peart

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

Quoted 8 lines

Quoted 8 lines

If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, ...

... and having said all that, I think it is perfectly fine to do such a clean-up long after the series gets more exposure to wider audiences as a follow-up patch. Let's get the primary part that affects people's everyday use of Git right and then worry about the test details later.

A quick show of hands to the list audiences. How many of you guys actually tried this series on 'pu' and checked to see its performance (and correctness ;-) characteristics?

Do you folks like it? Rather not have such complexity in the core part of the system? A good first step to start adding more performance improvements? No opinion?

Quoted 1 line · Ben Peart

Changes from V4 include:

...

I took a look at this patch series except the last patch ([PATCH v5 7/7] fsmonitor: add a performance test) as Junio reviewed it already, and had only a few comments on patches 3/7 and 4/7.

I am still not convinced by the discussions following v2
(http://public-inbox.org/git/[email protected]/)
about using a hook instead of for example a "core.fsmonitorcommand".

I think using a hook is not necessary and might not be a good match for later optimizations. For example people might want to use a library or some OS specific system calls to do what the hook does.

AEvar previously reported some not so great performance numbers on some big Booking.com boxes with a big monorepo and it seems that using taskset for example to make sure that the hook is run on the same CPU improves these numbers significantly. So avoiding to run a separate process can be important in some cases.

Quoted 17 lines · Christian Couder

Quoted 1 line · Ben Peart

Changes from V4 include:

...

I took a look at this patch series except the last patch ([PATCH v5 7/7] fsmonitor: add a performance test) as Junio reviewed it already, and had only a few comments on patches 3/7 and 4/7.

I am still not convinced by the discussions following v2
(http://public-inbox.org/git/[email protected]/)

about using a hook instead of for example a "core.fsmonitorcommand".

I think using a hook is not necessary and might not be a good match for later optimizations. For example people might want to use a library or some OS specific system calls to do what the hook does.

Quoted 6 lines

AEvar previously reported some not so great performance numbers on some big Booking.com boxes with a big monorepo and it seems that using taskset for example to make sure that the hook is run on the same CPU improves these numbers significantly. So avoiding to run a separate process can be important in some cases.

Using a hook is the only pattern I've seen in git that provides a way to enable OS specific calls. I used a hook so that different file monitoring services could be plugged in depending on the OS or tools available. The Watchman integration script was mostly intended as a sample that could be used where Watchman is available and works well.

I had not heard about the taskset issues you mention above. If there is something else that can be done to make this work better, please let me know the details.

If I'm understanding you correctly, you are suggesting that someone should be able to configure a setting (core.fsmonitorcommand) that gives a custom command line that would be run instead of running the query-fsmonitor hook.

I'm not entirely sure how that should work. There are command line options that need to be passed (currently the interface version as well as the current clock in nanoseconds). How would those passed when using the custom command?

Is it OK to just append them to the given command line? Does there need to be some substitution token to indicate where they should be inserted (ie "mycustomcommand --custom --options %version% --more-options %timestamp%"). Are there any other tokens that should be supported (ie PID or processor mask?).

Is there a design pattern already used somewhere in git that I can follow or is this all blazing a new trail?

Thanks for continuing to look into this. Feedback is good!

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Friday, September 15, 2017 3:21 PM
To: [email protected]
Cc: David Turner <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v6 05/12] fsmonitor: add documentation for the fsmonitor extension.

This includes the core.fsmonitor setting, the query-fsmonitor hook, and the fsmonitor index extension.

Signed-off-by Ben Peart <[email protected]>

---
 Documentation/config.txt                 |  6 ++++++
 Documentation/githooks.txt               | 23 +++++++++++++++++++++++
 Documentation/technical/index-format.txt | 19 +++++++++++++++++++
 3 files changed, 48 insertions(+)

Documentation/config.txt index renamed from Documentation/config.txt +0 -0

dc4e3f58a2..c196007a27 100644

Documentation/config.txt +2 -0

+The paths should be relative to the root of the working directory and
+be separated by a single NUL.

Documentation/config.txt +4 -0

+  - 32-bit bitmap size: the size of the CE_FSMONITOR_VALID bitmap.
+
+  - An ewah bitmap, the n-th bit indicates whether the n-th index entry
+    is not CE_FSMONITOR_VALID.

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Friday, September 15, 2017 3:21 PM
To: [email protected]
Cc: David Turner <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v6 06/12] ls-files: Add support in ls-files to display the fsmonitor valid bit

Add a new command line option (-f) to ls-files to have it use lowercase letters for 'fsmonitor valid' files

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Friday, September 15, 2017 3:21 PM
To: [email protected]
Cc: David Turner <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

config.c +3 -0

+int git_config_get_fsmonitor(void)
+{
+	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))

fsmonitor.c new file +4 -0

+void add_fsmonitor(struct index_state *istate) {
+	int i;
+
+	if (!istate->fsmonitor_last_update) {

+/*
+ * Clear the given cache entries CE_FSMONITOR_VALID bit and invalidate

t/t7519/fsmonitor-watchman new file +9 -0

new file mode 100755
index 0000000000..aaee5d1fe3
@@ -0,0 +1,128 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+use IPC::Open2;
+ ...
+	open (my $fh, ">", ".git/watchman-query.json");
+	print $fh "[\"query\", \"$git_work_tree\", { \
+	\"since\": $time, \

t/helper/test-dump-fsmonitor.c new file +5 -0

new file mode 100644
index 0000000000..482d749bb9
@@ -0,0 +1,21 @@
+#include "cache.h"
+
+int cmd_main(int ac, const char **av)
+{
+	struct index_state *istate = &the_index;

+[[fsmonitor-watchman]]
+fsmonitor-watchman
+~~~~~~~~~~~~~~~

Quoted 9 lines

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Friday, September 15, 2017 3:21 PM
To: [email protected]
Cc: David Turner <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

config.c int git_config_get_max_percent_split_change(void) · line 2180

Quoted 13 lines

config.c +13 -0

+int git_config_get_fsmonitor(void)
+{
+	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
+		core_fsmonitor = getenv("GIT_FSMONITOR_TEST");
+
+	if (core_fsmonitor && !*core_fsmonitor)
+		core_fsmonitor = NULL;
+
+	if (core_fsmonitor)
+		return 1;
+
+	return 0;
+}

This functions return values are backwards relative to the rest of the git_config_* functions.