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

From: Ben Peart <peartben@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, benpeart@microsoft.com, pclouds@gmail.com,
	johannes.schindelin@gmx.de, David.Turner@twosigma.com,
	peff@peff.net, christian.couder@gmail.com, avarab@gmail.com
Subject: Re: [PATCH v5 7/7] fsmonitor: add a performance test
Date: Fri, 7 Jul 2017 15:07:31 -0400	[thread overview]
Message-ID: <bade1166-e646-b05a-f65b-adb8da8ba0a7@gmail.com> (raw)
In-Reply-To: <xmqqeftsayeg.fsf@gitster.mtv.corp.google.com>

On 7/7/2017 2:35 PM, Junio C Hamano wrote:
> Ben Peart <peartben@gmail.com> writes:
> 
>> On 6/14/2017 2:36 PM, Junio C Hamano wrote:
>>> Ben Peart <peartben@gmail.com> 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?
> 
> 