RE: [PATCH v6 12/12] fsmonitor: add a performance test
Hi Ben, On Mon, 18 Sep 2017, Ben Peart wrote: > > From: Johannes Schindelin [mailto:johannes.schinde...@gmx.de] > > On Fri, 15 Sep 2017, Ben Peart wrote: > > > > > + DWORD(WINAPI *NtSetSystemInformation)(INT, PVOID, ULONG) = > > > + (DWORD(WINAPI *)(INT, PVOID, > > ULONG))GetProcAddress(ntdll, "NtSetSystemInformation"); > > > + if (!NtSetSystemInformation) > > > + return error("Can't get function addresses, wrong Windows > > > +version?"); > > > > It turns out that we have seen this plenty of times in Git for Windows' > > fork, so much so that we came up with a nice helper to make this all a bit > > more robust and a bit more obvious, too: the DECLARE_PROC_ADDR and > > INIT_PROC_ADDR helpers in compat/win32/lazyload.h. > > > > Maybe this would be the perfect excuse to integrate this patch into upstream > > Git? > > This patch is pretty hefty already. How about you push this capability > upstream and I take advantage of it in a later patch. :) Deal: https://public-inbox.org/git/f5a3add27206df3e7f39efeac8a3c3b47f2b79f2.1505834586.git.johannes.schinde...@gmx.de Ciao, Johannes
RE: [PATCH v6 12/12] fsmonitor: add a performance test
> -Original Message- > From: Johannes Schindelin [mailto:johannes.schinde...@gmx.de] > Sent: Monday, September 18, 2017 10:25 AM > To: Ben Peart > Cc: david.tur...@twosigma.com; ava...@gmail.com; > christian.cou...@gmail.com; git@vger.kernel.org; gits...@pobox.com; > pclo...@gmail.com; p...@peff.net > Subject: Re: [PATCH v6 12/12] fsmonitor: add a performance test > > Hi Ben, > > sorry for not catching this earlier: > > On Fri, 15 Sep 2017, Ben Peart wrote: > > > [...] > > + > > +int cmd_dropcaches(void) > > +{ > > + HANDLE hProcess = GetCurrentProcess(); > > + HANDLE hToken; > > + int status; > > + > > + if (!OpenProcessToken(hProcess, TOKEN_QUERY | > TOKEN_ADJUST_PRIVILEGES, &hToken)) > > + return error("Can't open current process token"); > > + > > + if (!GetPrivilege(hToken, "SeProfileSingleProcessPrivilege", 1)) > > + return error("Can't get SeProfileSingleProcessPrivilege"); > > + > > + CloseHandle(hToken); > > + > > + HMODULE ntdll = LoadLibrary("ntdll.dll"); > Thanks Johannes, I'll fix that. > Git's source code still tries to abide by C90, and for simplicity's sake, this > extends to the Windows-specific part. Therefore, the `ntdll` variable needs to > be declared at the beginning of the function (I do agree that it makes for > better code to reduce the scope of variables, but C90 simply did not allow > variables to be declared in the middle of functions). > > I wanted to send a patch address this in the obvious way, but then I > encountered these lines: > > > + DWORD(WINAPI *NtSetSystemInformation)(INT, PVOID, ULONG) = > > + (DWORD(WINAPI *)(INT, PVOID, > ULONG))GetProcAddress(ntdll, "NtSetSystemInformation"); > > + if (!NtSetSystemInformation) > > + return error("Can't get function addresses, wrong Windows > > +version?"); > > It turns out that we have seen this plenty of times in Git for Windows' > fork, so much so that we came up with a nice helper to make this all a bit > more robust and a bit more obvious, too: the DECLARE_PROC_ADDR and > INIT_PROC_ADDR helpers in compat/win32/lazyload.h. > > Maybe this would be the perfect excuse to integrate this patch into upstream > Git? This patch is pretty hefty already. How about you push this capability upstream and I take advantage of it in a later patch. :) This would be the patch (you can also cherry-pick it from > 25c4dc3a73352e72e995594cf1b4afa46e93d040 in > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub. > com%2Fdscho%2Fgit&data=02%7C01%7CBen.Peart%40microsoft.com%7C96 > 4027bdc1f34a033c1f08d4fea1056e%7C72f988bf86f141af91ab2d7cd011db47 > %7C1%7C0%7C636413414914282865&sdata=jyvu6G7myRY9UA1XxWx2tDZ% > 2BWsIWqLTRMT8WfzEGe5g%3D&reserved=0): > > -- snip -- > From 25c4dc3a73352e72e995594cf1b4afa46e93d040 Mon Sep 17 00:00:00 > 2001 > From: Johannes Schindelin > Date: Tue, 10 Jan 2017 23:14:20 +0100 > Subject: [PATCH] Win32: simplify loading of DLL functions > > Dynamic loading of DLL functions is duplicated in several places in Git for > Windows' source code. > > This patch adds a pair of macros to simplify the process: the > DECLARE_PROC_ADDR(, , , > ..) macro to be used at the beginning of a > code block, and the INIT_PROC_ADDR() macro to call before > using the declared function. The return value of the INIT_PROC_ADDR() call > has to be checked; If it is NULL, the function was not found in the specified > DLL. > > Example: > > DECLARE_PROC_ADDR(kernel32.dll, BOOL, CreateHardLinkW, > LPCWSTR, LPCWSTR, LPSECURITY_ATTRIBUTES); > > if (!INIT_PROC_ADDR(CreateHardLinkW)) > return error("Could not find CreateHardLinkW() function"; > > if (!CreateHardLinkW(source, target, NULL)) > return error("could not create hardlink from %S to %S", >source, target); > return 0; > > Signed-off-by: Karsten Blees > Signed-off-by: Johannes Schindelin > --- > compat/win32/lazyload.h | 44 > > 1 file changed, 44 insertions(+) > create mode 100644 compat/win32/lazyload.h > > diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h new file > mode 100644 index 000..91c10dad2fb > --- /dev/null > +++ b/compat/win32/lazyload.h > @@ -0,0 +1,44 @@ > +#ifndef LAZYLOAD_H > +#define LAZYLOAD_H > + > +/* simplify loading of DLL functions */ > + > +struct proc_a
Re: [PATCH v6 12/12] fsmonitor: add a performance test
Hi Ben, sorry for not catching this earlier: On Fri, 15 Sep 2017, Ben Peart wrote: > [...] > + > +int cmd_dropcaches(void) > +{ > + HANDLE hProcess = GetCurrentProcess(); > + HANDLE hToken; > + int status; > + > + if (!OpenProcessToken(hProcess, TOKEN_QUERY | TOKEN_ADJUST_PRIVILEGES, > &hToken)) > + return error("Can't open current process token"); > + > + if (!GetPrivilege(hToken, "SeProfileSingleProcessPrivilege", 1)) > + return error("Can't get SeProfileSingleProcessPrivilege"); > + > + CloseHandle(hToken); > + > + HMODULE ntdll = LoadLibrary("ntdll.dll"); Git's source code still tries to abide by C90, and for simplicity's sake, this extends to the Windows-specific part. Therefore, the `ntdll` variable needs to be declared at the beginning of the function (I do agree that it makes for better code to reduce the scope of variables, but C90 simply did not allow variables to be declared in the middle of functions). I wanted to send a patch address this in the obvious way, but then I encountered these lines: > + DWORD(WINAPI *NtSetSystemInformation)(INT, PVOID, ULONG) = > + (DWORD(WINAPI *)(INT, PVOID, ULONG))GetProcAddress(ntdll, > "NtSetSystemInformation"); > + if (!NtSetSystemInformation) > + return error("Can't get function addresses, wrong Windows > version?"); It turns out that we have seen this plenty of times in Git for Windows' fork, so much so that we came up with a nice helper to make this all a bit more robust and a bit more obvious, too: the DECLARE_PROC_ADDR and INIT_PROC_ADDR helpers in compat/win32/lazyload.h. Maybe this would be the perfect excuse to integrate this patch into upstream Git? This would be the patch (you can also cherry-pick it from 25c4dc3a73352e72e995594cf1b4afa46e93d040 in https://github.com/dscho/git): -- snip -- >From 25c4dc3a73352e72e995594cf1b4afa46e93d040 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 10 Jan 2017 23:14:20 +0100 Subject: [PATCH] Win32: simplify loading of DLL functions Dynamic loading of DLL functions is duplicated in several places in Git for Windows' source code. This patch adds a pair of macros to simplify the process: the DECLARE_PROC_ADDR(, , , ..) macro to be used at the beginning of a code block, and the INIT_PROC_ADDR() macro to call before using the declared function. The return value of the INIT_PROC_ADDR() call has to be checked; If it is NULL, the function was not found in the specified DLL. Example: DECLARE_PROC_ADDR(kernel32.dll, BOOL, CreateHardLinkW, LPCWSTR, LPCWSTR, LPSECURITY_ATTRIBUTES); if (!INIT_PROC_ADDR(CreateHardLinkW)) return error("Could not find CreateHardLinkW() function"; if (!CreateHardLinkW(source, target, NULL)) return error("could not create hardlink from %S to %S", source, target); return 0; Signed-off-by: Karsten Blees Signed-off-by: Johannes Schindelin --- compat/win32/lazyload.h | 44 1 file changed, 44 insertions(+) create mode 100644 compat/win32/lazyload.h diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h new file mode 100644 index 000..91c10dad2fb --- /dev/null +++ b/compat/win32/lazyload.h @@ -0,0 +1,44 @@ +#ifndef LAZYLOAD_H +#define LAZYLOAD_H + +/* simplify loading of DLL functions */ + +struct proc_addr { + const char *const dll; + const char *const function; + FARPROC pfunction; + unsigned initialized : 1; +}; + +/* Declares a function to be loaded dynamically from a DLL. */ +#define DECLARE_PROC_ADDR(dll, rettype, function, ...) \ + static struct proc_addr proc_addr_##function = \ + { #dll, #function, NULL, 0 }; \ + static rettype (WINAPI *function)(__VA_ARGS__) + +/* + * Loads a function from a DLL (once-only). + * Returns non-NULL function pointer on success. + * Returns NULL + errno == ENOSYS on failure. + */ +#define INIT_PROC_ADDR(function) \ + (function = get_proc_addr(&proc_addr_##function)) + +static inline void *get_proc_addr(struct proc_addr *proc) +{ + /* only do this once */ + if (!proc->initialized) { + HANDLE hnd; + proc->initialized = 1; + hnd = LoadLibraryExA(proc->dll, NULL, +LOAD_LIBRARY_SEARCH_SYSTEM32); + if (hnd) + proc->pfunction = GetProcAddress(hnd, proc->function); + } + /* set ENOSYS if DLL or function was not found */ + if (!proc->pfunction) + errno = ENOSYS; + return proc->pfunction; +} + +#endif -- snap -- With this patch, this fixup to your patch would make things compile (you can also cherry-pick d05996fb61027512b8ab31a36c4a7a677dea11bb from my fork): -- snipsnap -- >From d05996fb61027512b8ab31a36c4a7a677dea11bb Mon Sep 17 00:00:00 2001 From: Johannes Schinde
RE: [PATCH v6 12/12] fsmonitor: add a performance test
> -Original Message- > + # Choose integration script based on existance of Watchman. Spelling: existence
[PATCH v6 12/12] fsmonitor: add a performance test
Add a test utility (test-drop-caches) that flushes all changes to disk then drops file system cache on Windows, Linux, and OSX. Add a perf test (p7519-fsmonitor.sh) for fsmonitor. By default, the performance test will utilize the Watchman file system monitor if it is installed. If Watchman is not installed, it will use a dummy integration script that does not report any new or modified files. The dummy script has very little overhead which provides optimistic results. The performance test will also use the untracked cache feature if it is available as fsmonitor uses it to speed up scanning for untracked files. There are 3 environment variables that can be used to alter the default behavior of the performance test: GIT_PERF_7519_UNTRACKED_CACHE: used to configure core.untrackedCache GIT_PERF_7519_SPLIT_INDEX: used to configure core.splitIndex GIT_PERF_7519_FSMONITOR: used to configure core.fsMonitor The big win for using fsmonitor is the elimination of the need to scan the working directory looking for changed and untracked files. If the file information is all cached in RAM, the benefits are reduced. GIT_PERF_7519_DROP_CACHE: if set, the OS caches are dropped between tests Signed-off-by: Ben Peart Signed-off-by: Ævar Arnfjörð Bjarmason --- Makefile| 1 + t/helper/.gitignore | 1 + t/helper/test-drop-caches.c | 161 ++ t/perf/p7519-fsmonitor.sh | 184 4 files changed, 347 insertions(+) create mode 100644 t/helper/test-drop-caches.c create mode 100755 t/perf/p7519-fsmonitor.sh diff --git a/Makefile b/Makefile index d970cd00e9..b2653ee64f 100644 --- a/Makefile +++ b/Makefile @@ -638,6 +638,7 @@ TEST_PROGRAMS_NEED_X += test-ctype TEST_PROGRAMS_NEED_X += test-config TEST_PROGRAMS_NEED_X += test-date TEST_PROGRAMS_NEED_X += test-delta +TEST_PROGRAMS_NEED_X += test-drop-caches TEST_PROGRAMS_NEED_X += test-dump-cache-tree TEST_PROGRAMS_NEED_X += test-dump-fsmonitor TEST_PROGRAMS_NEED_X += test-dump-split-index diff --git a/t/helper/.gitignore b/t/helper/.gitignore index 721650256e..f9328eebdd 100644 --- a/t/helper/.gitignore +++ b/t/helper/.gitignore @@ -3,6 +3,7 @@ /test-config /test-date /test-delta +/test-drop-caches /test-dump-cache-tree /test-dump-split-index /test-dump-untracked-cache diff --git a/t/helper/test-drop-caches.c b/t/helper/test-drop-caches.c new file mode 100644 index 00..717079865c --- /dev/null +++ b/t/helper/test-drop-caches.c @@ -0,0 +1,161 @@ +#include "git-compat-util.h" + +#if defined(GIT_WINDOWS_NATIVE) + +int cmd_sync(void) +{ + char Buffer[MAX_PATH]; + DWORD dwRet; + char szVolumeAccessPath[] = ".\\X:"; + HANDLE hVolWrite; + int success = 0; + + dwRet = GetCurrentDirectory(MAX_PATH, Buffer); + if ((0 == dwRet) || (dwRet > MAX_PATH)) + return error("Error getting current directory"); + + if ((Buffer[0] < 'A') || (Buffer[0] > 'Z')) + return error("Invalid drive letter '%c'", Buffer[0]); + + szVolumeAccessPath[4] = Buffer[0]; + hVolWrite = CreateFile(szVolumeAccessPath, GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL); + if (INVALID_HANDLE_VALUE == hVolWrite) + return error("Unable to open volume for writing, need admin access"); + + success = FlushFileBuffers(hVolWrite); + if (!success) + error("Unable to flush volume"); + + CloseHandle(hVolWrite); + + return !success; +} + +#define STATUS_SUCCESS (0xL) +#define STATUS_PRIVILEGE_NOT_HELD (0xC061L) + +typedef enum _SYSTEM_INFORMATION_CLASS { + SystemMemoryListInformation = 80, +} SYSTEM_INFORMATION_CLASS; + +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; + tpP