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

On 7/7/2017 2:35 PM, Junio C Hamano wrote:

Ben Peart [off-list ref] writes:

quoted

On 6/14/2017 2:36 PM, Junio C Hamano wrote:

quoted

Ben Peart [off-list ref] writes:

quoted

quoted

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?