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

[Ben Peart <Ben.Peart@microsoft.com>]

> -----Original Message-----
> From: Johannes Schindelin [mailto:Johannes.Schindelin@gmx.de]
> Sent: Monday, September 18, 2017 10:25 AM
> To: Ben Peart <Ben.Peart@microsoft.com>
> Cc: David.Turner@twosigma.com; avarab@gmail.com;
> christian.couder@gmail.com; git@vger.kernel.org; gitster@pobox.com;
> pclouds@gmail.com; peff@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 <johannes.schindelin@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(<dll>, <return-type>, <function-name>,
> ...<function-parameter-types>...) macro to be used at the beginning of a
> code block, and the INIT_PROC_ADDR(<function-name>) 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 <blees@dcon.de>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@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 00000000000..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 Schindelin <johannes.schindelin@gmx.de>
> Date: Mon, 18 Sep 2017 14:56:40 +0200
> Subject: [PATCH] fixup! fsmonitor: add a performance test
> 
> ---
>  t/helper/test-drop-caches.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/t/helper/test-drop-caches.c b/t/helper/test-drop-caches.c index
> 717079865cb..b27358528f7 100644
> --- a/t/helper/test-drop-caches.c
> +++ b/t/helper/test-drop-caches.c
> @@ -1,6 +1,7 @@
>  #include "git-compat-util.h"
> 
>  #if defined(GIT_WINDOWS_NATIVE)
> +#include "compat/win32/lazyload.h"
> 
>  int cmd_sync(void)
>  {
> @@ -82,6 +83,9 @@ int cmd_dropcaches(void)
>  	HANDLE hProcess = GetCurrentProcess();
>  	HANDLE hToken;
>  	int status;
> +	SYSTEM_MEMORY_LIST_COMMAND command;
> +	DECLARE_PROC_ADDR(ntll,
> +			  DWORD, NtSetSystemInformation, INT, PVOID,
> ULONG);
> 
>  	if (!OpenProcessToken(hProcess, TOKEN_QUERY |
> TOKEN_ADJUST_PRIVILEGES, &hToken))
>  		return error("Can't open current process token"); @@ -91,16
> +95,10 @@ int cmd_dropcaches(void)
> 
>  	CloseHandle(hToken);
> 
> -	HMODULE ntdll = LoadLibrary("ntdll.dll");
> -	if (!ntdll)
> -		return error("Can't load ntdll.dll, wrong Windows
>  		version?");
> -
> -	DWORD(WINAPI *NtSetSystemInformation)(INT, PVOID, ULONG) =
> -		(DWORD(WINAPI *)(INT, PVOID,
> ULONG))GetProcAddress(ntdll,
>  		"NtSetSystemInformation");
> -	if (!NtSetSystemInformation)
> +	if (!INIT_PROC_ADDR(NtSetSystemInformation))
>  		return error("Can't get function addresses, wrong Windows
> version?");
> 
> -	SYSTEM_MEMORY_LIST_COMMAND command =
> MemoryPurgeStandbyList;
> +	command = MemoryPurgeStandbyList;
>  	status = NtSetSystemInformation(
>  		SystemMemoryListInformation,
>  		&command,
> @@ -111,8 +109,6 @@ int cmd_dropcaches(void)
>  	else if (status != STATUS_SUCCESS)
>  		error("Unable to execute the memory list command %d",
> status);
> 
> -	FreeLibrary(ntdll);
> -
>  	return status;
>  }
> 
> --
> 2.14.1.windows.1.510.g0cb6d35d23