From: Ben Peart <peartben@gmail.com>
Date: Wed, 14 Jun 2017 14:12:42 GMT
Subject: Re: [PATCH v5 7/7] fsmonitor: add a performance test
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:
> 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.

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