From: Ben Peart Date: Fri, 07 Jul 2017 19:07:31 GMT Subject: Re: [PATCH v5 7/7] fsmonitor: add a performance test Message-ID: In-Reply-To: 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? > >