Re: [PATCH v5 7/7] fsmonitor: add a performance test

[Junio C Hamano <gitster@pobox.com>]

Ben Peart <peartben@gmail.com> writes:

> diff --git a/t/helper/test-drop-caches.c b/t/helper/test-drop-caches.c
> new file mode 100644
> index 0000000000..80830d920b
> --- /dev/null
> +++ b/t/helper/test-drop-caches.c
> @@ -0,0 +1,107 @@
> +#include "git-compat-util.h"
> +#include <stdio.h>

I thought compat-util should already include stdio?

> +
> +typedef DWORD NTSTATUS;

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

> +
> +#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)?

> +} 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.

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

Style: avoid CamelCase.

> +} 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)
> +	{

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.

> +		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");

Is this "decl-after-stmt"?  Avoid it.

> +	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;
> +}

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?


> diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
> new file mode 100755
> index 0000000000..e41905cb9b
> --- /dev/null
> +++ b/t/perf/p7519-fsmonitor.sh
> @@ -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

Style: 

	if test "$GIT_PERF_REPEAT_COUNT" -ne 1
	then
	
> + ...
> +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?