Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor

From: Utsav Shah <utsav@dropbox.com>
To: Kevin Willford <Kevin.Willford@microsoft.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Utsav Shah via GitGitGadget <gitgitgadget@gmail.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>,
	Utsav Shah <ukshah2@illinois.edu>
Subject: Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor
Date: Tue, 29 Oct 2019 16:50:54 -0700	[thread overview]
Message-ID: <CAPYzU3Mv9fHG_WhCOfsA8KGeegdUCoEzfDCt8-DQ+CEjs=V62Q@mail.gmail.com> (raw)
In-Reply-To: <BN6PR21MB078689FFE9ED95A31998F41A91610@BN6PR21MB0786.namprd21.prod.outlook.com>

Thanks for testing it out. The unpack_trees bugfix is especially useful.

There's tons of places where we're using ce_uptodate(ce) that could be
optimized by checking CE_FSMONITOR_VALID. One example is in
run_diff_files in diff-lib.c

Should we add a check for CE_FSMONITOR_VALID in all of them? Should we
do that in this patch? Or should we take the time to refactor and
flesh out bugs in unifying it with CE_UPTODATE? It would be nice to
get more opinions. I've taken a look and believe that it will make
things a little more complicated to merge it with CE_UPTODATE,
especially since it's used in a few places for other reasons like
sparse checkouts. On the other hand, I'm a first time contributor, so
my perspective towards a large refactor like might be too
conservative.

On Tue, Oct 29, 2019 at 1:12 PM Kevin Willford
<Kevin.Willford@microsoft.com> wrote:
>
> > On Tuesday, October 29, 2019 1:07 PM Utsav Shah <utsav@dropbox.com>
> > wrote:
> >
> > I'm getting the same test failures with or without
> > GIT_TEST_FSMONITOR=t/t7519/fsmonitor-all and calling refresh_fsmonitor
> > in tweak_fsmonitor. Could you share your patch? I'm probably messing
> > something up, and I can try taking a look at fixing test cases as well.
>
> I have the tests passing with the following commit.
>
> https://github.com/kewillford/git/commit/3b1fdf5a4b1cd1d654b1733ce058faa4f087f75f
>
> Things to note:
> 1. Not sure if fsmonitor was tested with split index so for now I removed that from the
> check of entries in fsmonitor bitmap vs the number of cache entries
> 2. With these changes update-index was triggering the post-index-change hook with the
> updated_skipworktree flag set which it wasn't before.
> 3. Copied the fsmonitor_last_update to the result index so the fsmonitor data will be
> carried over to the new index in unpack_trees.  This is to make sure that the next call
> to git will have the fsmonitor data to use.  We found that running `git status` after any
> command that ran unpack_trees (checkout, reset --hard, etc.) was very slow the first
> call but and subsequent calls were fast.
>
> I'm still testing and reviewing these changes to make sure there isn't something I
> have missed and that I made the right changes to the tests that were failing.
>
> Kevin