subject:"\[PATCH v6 12\/12\] fsmonitor\: add a performance test"

RE: [PATCH v6 12/12] fsmonitor: add a performance test

2017-09-19 Thread Johannes Schindelin

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

2017-09-18 Thread Ben Peart


> -Original Message-
> From: Johannes Schindelin [mailto:johannes.schinde...@gmx.de]
> Sent: Monday, September 18, 2017 10:25 AM
> To: Ben Peart <ben.pe...@microsoft.com>
> 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, ))
> > +   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=02%7C01%7CBen.Peart%40microsoft.com%7C96
> 4027bdc1f34a033c1f08d4fea1056e%7C72f988bf86f141af91ab2d7cd011db47
> %7C1%7C0%7C636413414914282865=jyvu6G7myRY9UA1XxWx2tDZ%
> 2BWsIWqLTRMT8WfzEGe5g%3D=0):
> 
> -- snip --
> From 25c4dc3a73352e72e995594cf1b4afa46e93d040 Mon Sep 17 00:00:00
> 2001
> From: Johannes Schindelin <johannes.schinde...@gmx.de>
> 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 <bl...@dcon.de>
> Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
> ---
>  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 DL

Re: [PATCH v6 12/12] fsmonitor: add a performance test

2017-09-18 Thread Johannes Schindelin

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, 
> ))
> + 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(_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

RE: [PATCH v6 12/12] fsmonitor: add a performance test

2017-09-15 Thread David Turner


> -Original Message-
> + # Choose integration script based on existance of Watchman.

Spelling: existence

[PATCH v6 12/12] fsmonitor: add a performance test

2017-09-15 Thread Ben Peart

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, );
+   if (bResult) {
+   tpNewState.PrivilegeCount = 1;
+   tpNewState.Privileges[0].Luid = luid;
+   tpNewState.Privileges[0].Attributes = 0;
+   bResult = AdjustTokenPrivileges(TokenHandle, 0, ,
+   (DWORD)((LPBYTE)&(tpNewState.Privileges[1]) - 
(LPBYTE)),
+   , );
+   if (bResult) {
+   tpPreviousState.PrivilegeCount = 1;
+

RE: [PATCH v6 12/12] fsmonitor: add a performance test

RE: [PATCH v6 12/12] fsmonitor: add a performance test

Re: [PATCH v6 12/12] fsmonitor: add a performance test

RE: [PATCH v6 12/12] fsmonitor: add a performance test

[PATCH v6 12/12] fsmonitor: add a performance test

5 matches

Site Navigation

Mail list logo

Footer information