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

From: Ben Peart <peartben@gmail.com>
Date: Jun 14, 2017, 14:12 UTC
Message-ID: <9c1ed8d4-6bdb-e709-758d-4b010525e9e3@gmail.com>
In-Reply-To: <xmqqo9tsn9qg.fsf@gitster.mtv.corp.google.com>

On 6/12/2017 6:04 PM, Junio C Hamano wrote:

Show 17 quoted lines

> 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"?

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

Show 17 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)?
>

I'll remove them to be sure.

Show 9 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.
>

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.

Show 24 quoted lines

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

Show 36 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
> 	
>> + ...
>> +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.