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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 6/12/2017 6:04 PM, Junio C Hamano wrote:
Ben Peart <peartben@xxxxxxxxx> 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"?

I think it's safe but it isn't required.  I remove it to be sure.


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


I'll remove them to be sure.

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


Old habits. :) Started writing Windows code and adopted their coding style without even thinking about it. I'll 'gitify' the style as much as possible. This should address the various style comments below.

What you did for _SYSTEM_INFORMATION_CLASS above is correct.

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

Style: avoid CamelCase.


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?


I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.


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?


I'll copy it relative to $TEST_DIRECTORY in the next iteration.




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]