Re: [PATCH v5 7/7] fsmonitor: add a performance test
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 12, 2017, 22:04 UTC
- Message-ID
- <xmqqo9tsn9qg.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170610134026.104552-8-benpeart@microsoft.com>
Ben Peart <peartben@gmail.com> writes:
Show 8 quoted lines
> 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"?
Show 9 quoted lines
> +
> +#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)?
Show 5 quoted lines
> +} 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.
Show 7 quoted lines
> + MemoryCaptureAccessedBits, > + MemoryCaptureAndResetAccessedBits, > + MemoryEmptyWorkingSets, > + MemoryFlushModifiedList, > + MemoryPurgeStandbyList, > + MemoryPurgeLowPriorityStandbyList, > + MemoryCommandMax
Style: avoid CamelCase.
Show 14 quoted lines
> +} 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.Show 44 quoted lines
> + 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.
Show 24 quoted lines
> + 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?
Show 17 quoted lines
> 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
Show 6 quoted lines
> + ... > +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?