Re: [PATCH v5 7/7] fsmonitor: add a performance test
On Fri, Jul 7, 2017 at 8:35 PM, Junio C Hamano wrote: > Ben Peart writes: > >> On 6/14/2017 2:36 PM, Junio C Hamano wrote: >>> Ben Peart writes: >>> > 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. >>> >>> If this will become Windows-only, then I have no problem with >>> platform specfic typedef ;-) I have no problem with CamelCase, >>> either, as that follows the local convention on the platform >>> (similar to those in compat/* that are only for Windows). >>> >>> Having said all that. >>> >>> Another approach is to build this helper on all platforms, ... > > ... and having said all that, I think it is perfectly fine to do > such a clean-up long after the series gets more exposure to wider > audiences as a follow-up patch. Let's get the primary part that > affects people's everyday use of Git right and then worry about the > test details later. > > A quick show of hands to the list audiences. How many of you guys > actually tried this series on 'pu' and checked to see its > performance (and correctness ;-) characteristics? As you can guess from my previous replies to this thread (and the previous version of this patch series), I lightly tried it and checked its performance for Booking.com. > Do you folks like it? Rather not have such complexity in the core > part of the system? A good first step to start adding more > performance improvements? No opinion? I already gave my opinion which I think is shared with Ævar. In short I don't think it should be a hook, as that limits the performance and is not necessary, but it is going in the right direction.
RE: [PATCH v5 7/7] fsmonitor: add a performance test
> -Original Message- > From: Junio C Hamano [mailto:jch2...@gmail.com] On Behalf Of Junio C > Hamano > Sent: Friday, July 7, 2017 2:35 PM > To: Ben Peart > Cc: git@vger.kernel.org; benpe...@microsoft.com; pclo...@gmail.com; > johannes.schinde...@gmx.de; David Turner ; > p...@peff.net; christian.cou...@gmail.com; ava...@gmail.com > Subject: Re: [PATCH v5 7/7] fsmonitor: add a performance test > > Ben Peart writes: > > > On 6/14/2017 2:36 PM, Junio C Hamano wrote: > >> Ben Peart writes: > >> > >>>> 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. > >> > >> If this will become Windows-only, then I have no problem with > >> platform specfic typedef ;-) I have no problem with CamelCase, > >> either, as that follows the local convention on the platform (similar > >> to those in compat/* that are only for Windows). > >> > >> Having said all that. > >> > >> Another approach is to build this helper on all platforms, ... > > ... and having said all that, I think it is perfectly fine to do such a > clean-up long > after the series gets more exposure to wider audiences as a follow-up patch. > Let's get the primary part that affects people's everyday use of Git right > and then > worry about the test details later. > > A quick show of hands to the list audiences. How many of you guys actually > tried this series on 'pu' and checked to see its performance (and correctness > ;-) > characteristics? > > Do you folks like it? Rather not have such complexity in the core part of the > system? A good first step to start adding more performance improvements? No > opinion? I have not had the chance to test the latest version out yet. The idea, broadly, seems sound, but as Ben notes in a later mail, the details are important. Since he's going to re-roll with more interesting invalidation logic, I'll wait to try it again until a new version is available.
Re: [PATCH v5 7/7] fsmonitor: add a performance test
On 7/7/2017 2:35 PM, Junio C Hamano wrote: Ben Peart writes: On 6/14/2017 2:36 PM, Junio C Hamano wrote: Ben Peart writes: 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. If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows). Having said all that. Another approach is to build this helper on all platforms, ... ... and having said all that, I think it is perfectly fine to do such a clean-up long after the series gets more exposure to wider audiences as a follow-up patch. Let's get the primary part that affects people's everyday use of Git right and then worry about the test details later. A quick show of hands to the list audiences. How many of you guys actually tried this series on 'pu' and checked to see its performance (and correctness ;-) characteristics? TLDR: the current version isn't correct. One of the things I did was hack up the test script to enable running all the tests with fsmonitor enabled. I found a number of bugs in Watchman on Windows and have been working with Wez to get them fixed. Just last week Watchman got to the point where I could run the complete git test suite. As a result, I found that fsmonitor is overly aggressive in marking things with ce_mark_uptodate. Submodules are currently broken as are commands that pass "--ignore-missing." I started reworking the logic to fix these bugs and realized I was duplicating the set of tests that already exist in preload_thread. I'm currently working on integrating the logic into preload_thread so that both options can be used in combination and so I can avoid doing the (nearly identical) loop twice. Do you folks like it? Rather not have such complexity in the core part of the system? A good first step to start adding more performance improvements? No opinion?
Re: [PATCH v5 7/7] fsmonitor: add a performance test
Ben Peart writes: > On 6/14/2017 2:36 PM, Junio C Hamano wrote: >> Ben Peart writes: >> 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. >> >> If this will become Windows-only, then I have no problem with >> platform specfic typedef ;-) I have no problem with CamelCase, >> either, as that follows the local convention on the platform >> (similar to those in compat/* that are only for Windows). >> >> Having said all that. >> >> Another approach is to build this helper on all platforms, ... ... and having said all that, I think it is perfectly fine to do such a clean-up long after the series gets more exposure to wider audiences as a follow-up patch. Let's get the primary part that affects people's everyday use of Git right and then worry about the test details later. A quick show of hands to the list audiences. How many of you guys actually tried this series on 'pu' and checked to see its performance (and correctness ;-) characteristics? Do you folks like it? Rather not have such complexity in the core part of the system? A good first step to start adding more performance improvements? No opinion?
Re: [PATCH v5 7/7] fsmonitor: add a performance test
On 6/14/2017 2:36 PM, Junio C Hamano wrote: Ben Peart writes: 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. If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows). Having said all that. Another approach is to build this helper on all platforms, with sections protected by "#ifdef LINUX", "#ifdef WINDOWS", etc. That way, the platform detection and switching between running this program and echoing something into /proc filesystem performed in p7519 can be removed (this test-helper program will be responsible to implement that logic instead). When p7519 wants to drop the filesystem cache, regardless of the platform, it can call this test-helper without having to know how the filesystem cache is dropped. I'll take a cut at doing this but it is obviously very platform dependent and I'm unfamiliar with platforms other than Windows. For everything other than Windows, my implementation will be calling "system" for external utilities based on what I can find on the web. Oh, and I have no way to test it other than on Windows so could use some review/testing from others. :) I do not have strong preference either way, but I have a slight suspicion that the "another approach" above may give us a better result. For one thing, the test-helper can be reused in new perf scripts people will write in the future. Thanks.
Re: [PATCH v5 7/7] fsmonitor: add a performance test
Ben Peart writes: >> 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. If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows). Having said all that. Another approach is to build this helper on all platforms, with sections protected by "#ifdef LINUX", "#ifdef WINDOWS", etc. That way, the platform detection and switching between running this program and echoing something into /proc filesystem performed in p7519 can be removed (this test-helper program will be responsible to implement that logic instead). When p7519 wants to drop the filesystem cache, regardless of the platform, it can call this test-helper without having to know how the filesystem cache is dropped. I do not have strong preference either way, but I have a slight suspicion that the "another approach" above may give us a better result. For one thing, the test-helper can be reused in new perf scripts people will write in the future. Thanks.
Re: [PATCH v5 7/7] fsmonitor: add a performance test
On 6/12/2017 6:04 PM, Junio C Hamano wrote: Ben Peart writes: diff --git a/t/helper/test-drop-caches.c b/t/helper/test-drop-caches.c new file mode 100644 index 00..80830d920b --- /dev/null +++ b/t/helper/test-drop-caches.c @@ -0,0 +1,107 @@ +#include "git-compat-util.h" +#include 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 + +#define STATUS_SUCCESS (0xL) +#define STATUS_PRIVILEGE_NOT_HELD (0xC061L) + +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 00..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= option? Perhaps take the copy relative to $TEST_DIRECTORY? I'll copy it relative to $TEST_DIRECTORY in the next iteration.
Re: [PATCH v5 7/7] fsmonitor: add a performance test
Ben Peart writes: > diff --git a/t/helper/test-drop-caches.c b/t/helper/test-drop-caches.c > new file mode 100644 > index 00..80830d920b > --- /dev/null > +++ b/t/helper/test-drop-caches.c > @@ -0,0 +1,107 @@ > +#include "git-compat-util.h" > +#include 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 > + > +#define STATUS_SUCCESS (0xL) > +#define STATUS_PRIVILEGE_NOT_HELD(0xC061L) > + > +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 wond
Re: [PATCH v5 7/7] fsmonitor: add a performance test
Here are some perf test results for repos of various size generated with many-files.sh. Comparing cold fs cache times on a fast SSD running Windows: # files preloadindexfsmonitor reduction = 10,000 0.690.4633% 100,000 3.750.7 81% 1,000,000 35.07 3.2491% 10,000 files GIT_PERF_REPEAT_COUNT=1 GIT_PERF_LARGE_REPO=/c/Repos/gen-many-files-3.10.9.git/ ./run p7519-fsmonitor.sh Test this tree - 7519.3: status (fsmonitor=false, cold fs cache) 0.69(0.03+0.06) 7519.4: status (fsmonitor=false, warm fs cache) 0.45(0.01+0.06) 7519.6: status -uno (fsmonitor=false, cold fs cache) 0.53(0.03+0.04) 7519.8: status -uall (fsmonitor=false, cold fs cache) 0.57(0.04+0.01) 7519.11: status (fsmonitor=true, cold fs cache, cold fsmonitor) 2.16(0.01+0.06) 7519.12: status (fsmonitor=true, warm fs cache, warm fsmonitor) 0.36(0.01+0.07) 7519.14: status (fsmonitor=true, cold fs cache, warm fsmonitor) 0.46(0.04+0.06) 7519.16: status -uno (fsmonitor=true, cold fs cache) 0.46(0.03+0.04) 7519.18: status -uall (fsmonitor=true, cold fs cache) 0.76(0.04+0.04) 100,000 files GIT_PERF_REPEAT_COUNT=1 GIT_PERF_LARGE_REPO=/c/Repos/gen-many-files-4.10.9.git/ ./run p7519-fsmonitor.sh Test this tree -- 7519.3: status (fsmonitor=false, cold fs cache) 3.75(0.01+0.04) 7519.4: status (fsmonitor=false, warm fs cache) 2.74(0.06+0.06) 7519.6: status -uno (fsmonitor=false, cold fs cache) 2.88(0.01+0.06) 7519.8: status -uall (fsmonitor=false, cold fs cache) 3.24(0.00+0.06) 7519.11: status (fsmonitor=true, cold fs cache, cold fsmonitor) 17.90(0.00+0.10) 7519.12: status (fsmonitor=true, warm fs cache, warm fsmonitor) 0.59(0.00+0.09) 7519.14: status (fsmonitor=true, cold fs cache, warm fsmonitor) 0.70(0.00+0.07) 7519.16: status -uno (fsmonitor=true, cold fs cache) 0.69(0.01+0.01) 7519.18: status -uall (fsmonitor=true, cold fs cache) 4.51(0.00+0.06) 1,000,000 files GIT_PERF_REPEAT_COUNT=1 GIT_PERF_LARGE_REPO=/c/Repos/gen-many-files-5.10.9.git/ ./run p7519-fsmonitor.sh Test this tree --- 7519.3: status (fsmonitor=false, cold fs cache) 35.07(0.01+0.03) 7519.4: status (fsmonitor=false, warm fs cache) 26.58(0.03+0.07) 7519.6: status -uno (fsmonitor=false, cold fs cache) 26.46(0.03+0.06) 7519.8: status -uall (fsmonitor=false, cold fs cache) 31.55(0.01+0.03) 7519.11: status (fsmonitor=true, cold fs cache, cold fsmonitor) 193.15(0.01+0.04) 7519.12: status (fsmonitor=true, warm fs cache, warm fsmonitor) 3.03(0.01+0.07) 7519.14: status (fsmonitor=true, cold fs cache, warm fsmonitor) 3.24(0.01+0.04) 7519.16: status -uno (fsmonitor=true, cold fs cache) 2.99(0.03+0.03) 7519.18: status -uall (fsmonitor=true, cold fs cache) 35.07(0.03+0.07) On 6/10/2017 9:40 AM, Ben Peart wrote: Add a test utility (test-drop-caches) that enables dropping the file system cache on Windows. Add a perf test (p7519-fsmonitor.sh) for fsmonitor. Signed-off-by: Ben Peart Signed-off-by: Ævar Arnfjörð Bjarmason --- Makefile| 1 + t/helper/test-drop-caches.c | 107 + t/perf/p7519-fsmonitor.sh | 161 3 files changed, 269 insertions(+) create mode 100644 t/helper/test-drop-caches.c create mode 100755 t/perf/p7519-fsmonitor.sh diff --git a/Makefile b/Makefile index 992dd58801..893947839f 100644 --- a/Makefile +++ b/Makefile @@ -648,6 +648,7 @@ TEST_PROGRAMS_NEED_X += test-subprocess TEST_PROGRAMS_NEED_X += test-svn-fe TEST_PROGRAMS_NEED_X += test-urlmatch-normalization TEST_PROGRAMS_NEED_X += test-wildmatch +TEST_PROGRAMS_NEED_X += test-drop-caches TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X)) diff --git a/t/helper/test-drop-caches.c b/t/helper/test-drop-caches.c new file mode 100644 index 00..80830d920b --- /dev/null +++ b/t/helper/test-drop-caches.c @@ -0,0 +1,107 @@ +#include "git-compat-util.h" +#include + +typedef DWORD NTSTATUS; + +#ifdef GIT_WINDOWS_NATIVE +#include + +#define STATUS_SUCCESS (0xL) +#define STATUS_PRIVILEGE_NOT_HELD (0xC061L) + +typedef enum _SYSTEM_INFORMATION_CLASS { + SystemMemoryListInformation = 80, // 80, q: SYSTEM_MEMORY_LIST_INFORMATION; s: SYSTEM_MEMORY_LIST_COMMAND (requires SeProfileSingleProcessPrivilege) +} SYSTEM_INFORMATION_CLASS; + +// private +typedef enum _SYSTEM_MEMORY_LIST_COMMAND +{ + MemoryCaptureAccessedBits, + MemoryCaptureAndReset