Re: [PATCH v5 7/7] fsmonitor: add a performance test
From: Junio C Hamano <hidden>
Date: 2017-06-12 22:04:47
Ben Peart [off-list ref] writes:
quoted hunk
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?
quoted hunk
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?