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

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

[Utsav Shah via GitGitGadget]

git stash runs git reset --hard as its final step, which can be fairly slow
on a large repository. This change lets us skip that if fsmonitor thinks
those files aren't modified.

git stash goes from ~8s -> 2s on my repository after this change.

Utsav Shah (1):
  unpack-trees: skip lstat based on fsmonitor

 unpack-trees.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/424
-- 
gitgitgadget

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

[Utsav Shah via GitGitGadget]

From: Utsav Shah <utsav@dropbox.com>

git stash runs git reset --hard as its final step, which can be fairly slow on a large repository.
This change lets us skip that if fsmonitor thinks those files aren't modified.

git stash goes from ~8s -> 2s on my repository after this change.

Signed-off-by: Utsav Shah <utsav@dropbox.com>
---
 unpack-trees.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 33ea7810d8..5d07a2d0ea 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2384,7 +2384,8 @@ int oneway_merge(const struct cache_entry * const *src,
 
 	if (old && same(old, a)) {
 		int update = 0;
-		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
+		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
+		    !(old->ce_flags & CE_FSMONITOR_VALID)) {
 			struct stat st;
 			if (lstat(old->name, &st) ||
 			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
-- 
gitgitgadget

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

[Junio C Hamano]

"Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Utsav Shah <utsav@dropbox.com>
>
> git stash runs git reset --hard as its final step, which can be fairly slow on a large repository.
> This change lets us skip that if fsmonitor thinks those files aren't modified.
>
> git stash goes from ~8s -> 2s on my repository after this change.

Please line-wrap overlong lines.

More importantly, "stash" may be a mere symptom that does not
deserve this much emphasis.  What you improved directly is "git
reset --hard" isn't it?

    The fsmonitor may know that a path hasn't been modified but 
    "git reset --hard" did not pay attention to it and performed
    its own check based on ie_match_stat(), which was inefficient.

or something like that?

>  	if (old && same(old, a)) {
>  		int update = 0;
> -		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
> +		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
> +		    !(old->ce_flags & CE_FSMONITOR_VALID)) {

I wonder if !ce_uptodate(old) should say "this one is up to date and
not modified" when CE_FSMONITOR_VALID bit is set.  Are there other
codepaths that use ce_uptodate(ce) to decide to do X without paying
attention to CE_FSMONITOR_VALID bit?  If there are, are they buggy
in the same way as you found this instance, or do they have legitimate
reason why they only check ce_uptodate(ce) and ignore fsmonitor?

If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID
bit and have fsmonitor directly set CE_UPTODATE bit instead?  That
would make this fix unnecessary and fix other codepaths that check
only ce_uptodate() without checking fsmonitor.

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

[Utsav Shah]

Thanks for the review.

On Sun, Oct 27, 2019 at 8:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Utsav Shah <utsav@dropbox.com>
> >
> > git stash runs git reset --hard as its final step, which can be fairly slow on a large repository.
> > This change lets us skip that if fsmonitor thinks those files aren't modified.
> >
> > git stash goes from ~8s -> 2s on my repository after this change.
>
> Please line-wrap overlong lines.
>
> More importantly, "stash" may be a mere symptom that does not
> deserve this much emphasis.  What you improved directly is "git
> reset --hard" isn't it?
>
>     The fsmonitor may know that a path hasn't been modified but
>     "git reset --hard" did not pay attention to it and performed
>     its own check based on ie_match_stat(), which was inefficient.
>
> or something like that?
>
> >       if (old && same(old, a)) {
> >               int update = 0;
> > -             if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
> > +             if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
> > +                 !(old->ce_flags & CE_FSMONITOR_VALID)) {
>
> I wonder if !ce_uptodate(old) should say "this one is up to date and
> not modified" when CE_FSMONITOR_VALID bit is set.  Are there other
> codepaths that use ce_uptodate(ce) to decide to do X without paying
> attention to CE_FSMONITOR_VALID bit?  If there are, are they buggy
> in the same way as you found this instance, or do they have legitimate
> reason why they only check ce_uptodate(ce) and ignore fsmonitor?
>

Yes, there are other code paths as well. After reading the code some
more, it seems like there's no legitimate need to ignore fsmonitor.

> If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID
> bit and have fsmonitor directly set CE_UPTODATE bit instead?  That
> would make this fix unnecessary and fix other codepaths that check
> only ce_uptodate() without checking fsmonitor.
>

There's a few issues with replacing it entirely that I've found.

One is the  "CE_MATCH_IGNORE_FSMONITOR" flag. This flag can be set to
let ie_match_stat skip calling refresh_fsmonitor repeatedly. This is
set only in one place right now in preload-index, and it's unclear how
necessary this optimization even is, given that refresh_fsmonitor has
a check whether it's been called already, and returns if true.

The second is that git ls-files has an "f" option that makes it "use
lowercase letters for 'fsmonitor clean' files". I think this can
simply be replaced by checking if a file is up to date instead of
specifically via fsmonitor.

If we do go ahead with the replace, we will have to be diligent about
calling refresh_fsmonitor everywhere, or we will have correctness
issues. I patched git locally to do this, and immediately saw a bug in
git stash where the underlying git reset --hard skipped modifying a
file it should have. In my opinion refresh_fsmonitor should be called
somewhere top level, like an initialization, but I'm not sure if that
makes sense for all git subcommands.

Do you think it's worth cleaning up and sending this patch instead? It
will reduce the surface area of bugs and remove a bunch of functions
like mark_fsmonitor_valid/mark_fsmonitor_invalid

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

[Kevin Willford]

On Monday, October 28, 2019 12:40 AM Utsav Shah <utsav@dropbox.com> 
wrote:

> > I wonder if !ce_uptodate(old) should say "this one is up to date and
> > not modified" when CE_FSMONITOR_VALID bit is set.  Are there other
> > codepaths that use ce_uptodate(ce) to decide to do X without paying
> > attention to CE_FSMONITOR_VALID bit?  If there are, are they buggy in
> > the same way as you found this instance, or do they have legitimate
> > reason why they only check ce_uptodate(ce) and ignore fsmonitor?
> >
> 
> Yes, there are other code paths as well. After reading the code some more, it
> seems like there's no legitimate need to ignore fsmonitor.
> 
> > If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID
> > bit and have fsmonitor directly set CE_UPTODATE bit instead?  That
> > would make this fix unnecessary and fix other codepaths that check
> > only ce_uptodate() without checking fsmonitor.
> >
> 

I would need to go back and see if there was some reasoning why the
new flag was added but using CE_UPTODATE makes sense especially when
most calls to ce_mark_uptodate is followed directly by a call to
mark_fsmonitor_valid.  There is a little more going on in the
mark_fsmonitor_X than just setting the bit though and the invalid
calls are not matched with code to clear the CE_UPTODATE flag.

The change to use CE_UPTODATE would have
more extensive effects and like you said we would need to make sure it
would not cause correctness issues in some corner case.

Did you run all the git tests with GIT_TEST_FSMONITOR set to
t/t7519/fsmonitor-all?  This will run the tests with fsmonitor on.  I was
getting multiple failures with this change and fsmonitor on.

I added the refresh_fsmonitor call to the tweak_fsmonitor after
using the bitmap to set the dirty entries.  This fixed most of the test  
failures but there are still some failures that I haven't tracked down the
reason for.

I will do some more digging and testing to see what other pitfalls there
might be with this change.

Thanks,
Kevin

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

[Utsav Shah]

On Mon, Oct 28, 2019 at 12:23 PM Kevin Willford
<Kevin.Willford@microsoft.com> wrote:
>
> On Monday, October 28, 2019 12:40 AM Utsav Shah <utsav@dropbox.com>
> wrote:
>
> > > I wonder if !ce_uptodate(old) should say "this one is up to date and
> > > not modified" when CE_FSMONITOR_VALID bit is set.  Are there other
> > > codepaths that use ce_uptodate(ce) to decide to do X without paying
> > > attention to CE_FSMONITOR_VALID bit?  If there are, are they buggy in
> > > the same way as you found this instance, or do they have legitimate
> > > reason why they only check ce_uptodate(ce) and ignore fsmonitor?
> > >
> >
> > Yes, there are other code paths as well. After reading the code some more, it
> > seems like there's no legitimate need to ignore fsmonitor.
> >
> > > If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID
> > > bit and have fsmonitor directly set CE_UPTODATE bit instead?  That
> > > would make this fix unnecessary and fix other codepaths that check
> > > only ce_uptodate() without checking fsmonitor.
> > >
> >
>
> I would need to go back and see if there was some reasoning why the
> new flag was added but using CE_UPTODATE makes sense especially when
> most calls to ce_mark_uptodate is followed directly by a call to
> mark_fsmonitor_valid.

I've been playing around with the patch and trying to get the tests pass.

I've found that we set CE_UPTODATE when we try to skip worktrees to
stat in the case of sparse checkouts, and there are cases where we
mark cache entries up to date without consulting fsmonitor or stating
them.

It seems like making fsmonitor only modify CE_UPTODATE makes it hard
to verify and test correct fsmonitor behavior and debugging fsmonitor
with git ls-files -f. I think the patch also makes things overall
slightly more complicated.

There is a little more going on in the
> mark_fsmonitor_X than just setting the bit though and the invalid
> calls are not matched with code to clear the CE_UPTODATE flag.

Yeah. The patch to replace CE_FSMONITOR_VALID doesn't remove the need
for calling mark_fsmonitor_valid/mark_fsmonitor_invalid, since there's
special behavior like modifying the untracked cache which doesn't make
sense in a more general mark_ce_not_uptodate function.

>
> The change to use CE_UPTODATE would have
> more extensive effects and like you said we would need to make sure it
> would not cause correctness issues in some corner case.
>
> Did you run all the git tests with GIT_TEST_FSMONITOR set to
> t/t7519/fsmonitor-all?  This will run the tests with fsmonitor on.  I was
> getting multiple failures with this change and fsmonitor on.
>
> I added the refresh_fsmonitor call to the tweak_fsmonitor after
> using the bitmap to set the dirty entries.  This fixed most of the test
> failures but there are still some failures that I haven't tracked down the
> reason for.

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 will do some more digging and testing to see what other pitfalls there
> might be with this change.
>
> Thanks,
> Kevin

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

[Kevin Willford]

> 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

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

[Utsav Shah]

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

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

[Junio C Hamano]

Utsav Shah <utsav@dropbox.com> writes:

> 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?

If we rephrase the first question slightly, i.e. "should these
places all be avoiding lstat() based check when fsmonitor says the
path is up to date?", I would imagine the answer is absolutely yes.

I would further imagine that the implementation of the interface to
external fsmonitor itself may have to distinguish "we know/have known
this path is clean" vs "we just got told by fsmonitor that this path
is clean", so losing FSMONITOR_VALID bit might not be an easy or
clean conversion, in which case my earlier "can we perhaps lose it
and have fsmonitor interfacing code to directly set UPTODATE bit?"
would lead us in a wrong direction.

But ce_uptodate(ce) being the primary way for the callers that care
about "is the path known to be up to date?", it is unsatisfying that
all of them have to ask

	if (!ce_uptodate(ce) && !(ce->ce_flags & CE_FSMONITOR_VALID))
		... process ce that is not up-to-date ...

So I would say that the longer term goal should be to let them ask
ce_uptodate(ce) and have that macro automatically take FSMONITOR bit
into account (in other words, those who want to know if ce is fresh
should not have to even know about what fsmonitor is).

Perhaps we can take a polished version of this "'reset --hard' can
and should notice paths known-to-be-uptodate via fsmonitor" as an
independent patch (to reduce the number of things we have to worry
by one) for now?  Taking this patch means we would now have one more
place that checks both ce_uptodate() and FSMONITOR_VALID bit, but if
we would be auditing all such places before we can decide what the
best way to reach the goal of allowing them to just say ce_uptodate()
without having to spell FSMONITOR_VALID, that probably is a cost
worth paying.

Thanks for working on this topic.

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

[Utsav Shah]

Thanks, that makes a lot of sense. ce_uptodate doesn't have too many
callers either, so modifying it and checking CE_FSMONITOR_VALID there
should not be hard to audit.



On Tue, Oct 29, 2019 at 5:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Utsav Shah <utsav@dropbox.com> writes:
>
> > 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?
>
> If we rephrase the first question slightly, i.e. "should these
> places all be avoiding lstat() based check when fsmonitor says the
> path is up to date?", I would imagine the answer is absolutely yes.
>
> I would further imagine that the implementation of the interface to
> external fsmonitor itself may have to distinguish "we know/have known
> this path is clean" vs "we just got told by fsmonitor that this path
> is clean", so losing FSMONITOR_VALID bit might not be an easy or
> clean conversion, in which case my earlier "can we perhaps lose it
> and have fsmonitor interfacing code to directly set UPTODATE bit?"
> would lead us in a wrong direction.
>
> But ce_uptodate(ce) being the primary way for the callers that care
> about "is the path known to be up to date?", it is unsatisfying that
> all of them have to ask
>
>         if (!ce_uptodate(ce) && !(ce->ce_flags & CE_FSMONITOR_VALID))
>                 ... process ce that is not up-to-date ...
>
> So I would say that the longer term goal should be to let them ask
> ce_uptodate(ce) and have that macro automatically take FSMONITOR bit
> into account (in other words, those who want to know if ce is fresh
> should not have to even know about what fsmonitor is).
>
> Perhaps we can take a polished version of this "'reset --hard' can
> and should notice paths known-to-be-uptodate via fsmonitor" as an
> independent patch (to reduce the number of things we have to worry
> by one) for now?  Taking this patch means we would now have one more
> place that checks both ce_uptodate() and FSMONITOR_VALID bit, but if
> we would be auditing all such places before we can decide what the
> best way to reach the goal of allowing them to just say ce_uptodate()
> without having to spell FSMONITOR_VALID, that probably is a cost
> worth paying.
>
> Thanks for working on this topic.
>
>
>

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

[Junio C Hamano]

Utsav Shah <utsav@dropbox.com> writes:

[jc: we avoid top-posting for readability, so swapped paragraphs in
this quote]

>> Perhaps we can take a polished version of this "'reset --hard' can
>> and should notice paths known-to-be-uptodate via fsmonitor" as an
>> independent patch (to reduce the number of things we have to worry
>> by one) for now?  Taking this patch means we would now have one more
>> place that checks both ce_uptodate() and FSMONITOR_VALID bit, but if
>> we would be auditing all such places before we can decide what the
>> best way to reach the goal of allowing them to just say ce_uptodate()
>> without having to spell FSMONITOR_VALID, that probably is a cost
>> worth paying.
>>
>> Thanks for working on this topic.
>
> Thanks, that makes a lot of sense. ce_uptodate doesn't have too many
> callers either, so modifying it and checking CE_FSMONITOR_VALID there
> should not be hard to audit.

OK, so let's see an updated and hopefully final version of [*1*],
perhaps with Kevin's help you mentioned in [*2*] for now?


[References]

*1* <pull.424.git.1572017008.gitgitgadget@gmail.com>
*2* <CAPYzU3Mv9fHG_WhCOfsA8KGeegdUCoEzfDCt8-DQ+CEjs=V62Q@mail.gmail.com>

[PATCH v2 0/1] unpack-trees: skip stat on fsmonitor-valid files

[Utsav Shah via GitGitGadget]

The index might be aware that a file hasn't modified via fsmonitor, but
unpack-trees did not pay attention to it and checked via ie_match_stat which
can be inefficient on certain filesystems. This significantly slows down
commands that run oneway_merge, like checkout and reset --hard.

This patch makes oneway_merge check whether a file is considered unchanged
through fsmonitor and skips ie_match_stat on it. unpack-trees also now
correctly copies over fsmonitor validity state from the source index.
Finally, for correctness, we force a refresh of fsmonitor state in
tweak_fsmonitor.

After this change, commands like stash (that use reset --hard internally) go
from 8s or more to ~2s on a 250k file repository on a mac.

Signed-off-by: Utsav Shah utsav@dropbox.com [utsav@dropbox.com]

Utsav Shah (1):
  unpack-trees: skip stat on fsmonitor-valid files

 fsmonitor.c                       | 20 +++++++++++---------
 t/t7113-post-index-change-hook.sh |  3 ---
 t/t7519-status-fsmonitor.sh       |  9 +++++++--
 unpack-trees.c                    |  6 +++++-
 4 files changed, 23 insertions(+), 15 deletions(-)


base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/424

Range-diff vs v1:

 1:  609c7c5047 ! 1:  f76ba554ed unpack-trees: skip lstat based on fsmonitor
     @@ -1,24 +1,147 @@
      Author: Utsav Shah <utsav@dropbox.com>
      
     -    unpack-trees: skip lstat based on fsmonitor
     +    unpack-trees: skip stat on fsmonitor-valid files
      
     -    git stash runs git reset --hard as its final step, which can be fairly slow on a large repository.
     -    This change lets us skip that if fsmonitor thinks those files aren't modified.
     +    The index might be aware that a file hasn't modified via fsmonitor, but
     +    unpack-trees did not pay attention to it and checked via ie_match_stat
     +    which can be inefficient on certain filesystems. This significantly slows
     +    down commands that run oneway_merge, like checkout and reset --hard.
      
     -    git stash goes from ~8s -> 2s on my repository after this change.
     +    This patch makes oneway_merge check whether a file is considered
     +    unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees
     +    also now correctly copies over fsmonitor validity state from the source
     +    index. Finally, for correctness, we force a refresh of fsmonitor state in
     +    tweak_fsmonitor.
     +
     +    After this change, commands like stash (that use reset --hard
     +    internally) go from 8s or more to ~2s on a 250k file repository on a
     +    mac.
      
          Signed-off-by: Utsav Shah <utsav@dropbox.com>
      
     + diff --git a/fsmonitor.c b/fsmonitor.c
     + --- a/fsmonitor.c
     + +++ b/fsmonitor.c
     +@@
     + 	}
     + 	istate->fsmonitor_dirty = fsmonitor_dirty;
     + 
     +-	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
     +-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
     +-		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
     ++	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
     ++		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
     ++		    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
     ++
     + 
     + 	trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful");
     + 	return 0;
     +@@
     + 	uint32_t ewah_size = 0;
     + 	int fixup = 0;
     + 
     +-	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
     +-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
     +-		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
     ++	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
     ++		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
     ++		    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
     + 
     + 	put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
     + 	strbuf_add(sb, &hdr_version, sizeof(uint32_t));
     +@@
     + 		}
     + 		if (bol < query_result.len)
     + 			fsmonitor_refresh_callback(istate, buf + bol);
     ++
     ++		if (istate->untracked)
     ++			istate->untracked->use_fsmonitor = 1;
     + 	} else {
     + 		/* Mark all entries invalid */
     + 		for (i = 0; i < istate->cache_nr; i++)
     +@@
     + 				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
     + 			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
     + 
     +-			/* Now mark the untracked cache for fsmonitor usage */
     +-			if (istate->untracked)
     +-				istate->untracked->use_fsmonitor = 1;
     ++			refresh_fsmonitor(istate);
     + 		}
     + 
     + 		ewah_free(istate->fsmonitor_dirty);
     +
     + diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-hook.sh
     + --- a/t/t7113-post-index-change-hook.sh
     + +++ b/t/t7113-post-index-change-hook.sh
     +@@
     + 	git checkout -- dir1/file1.txt &&
     + 	test_path_is_file testsuccess && rm -f testsuccess &&
     + 	test_path_is_missing testfailure &&
     +-	git update-index &&
     +-	test_path_is_missing testsuccess &&
     +-	test_path_is_missing testfailure &&
     + 	git reset --soft &&
     + 	test_path_is_missing testsuccess &&
     + 	test_path_is_missing testfailure
     +
     + diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
     + --- a/t/t7519-status-fsmonitor.sh
     + +++ b/t/t7519-status-fsmonitor.sh
     +@@
     + 
     + # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit
     + test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor valid bit' '
     ++	write_script .git/hooks/fsmonitor-test<<-\EOF &&
     ++	EOF
     + 	git update-index --fsmonitor &&
     + 	git update-index --fsmonitor-valid dir1/modified &&
     + 	git update-index --fsmonitor-valid dir2/modified &&
     +@@
     + 
     + # test that newly added files are marked valid
     + test_expect_success 'newly added files are marked valid' '
     ++	write_script .git/hooks/fsmonitor-test<<-\EOF &&
     ++	EOF
     + 	git add new &&
     + 	git add dir1/new &&
     + 	git add dir2/new &&
     +@@
     + # Ensure commands that call refresh_index() to move the index back in time
     + # properly invalidate the fsmonitor cache
     + test_expect_success 'refresh_index() invalidates fsmonitor cache' '
     +-	write_script .git/hooks/fsmonitor-test<<-\EOF &&
     +-	EOF
     + 	clean_repo &&
     ++	write_integration_script &&
     + 	dirty_repo &&
     + 	git add . &&
     ++	write_script .git/hooks/fsmonitor-test<<-\EOF &&
     ++	EOF
     + 	git commit -m "to reset" &&
     + 	git reset HEAD~1 &&
     + 	git status >actual &&
     +
       diff --git a/unpack-trees.c b/unpack-trees.c
       --- a/unpack-trees.c
       +++ b/unpack-trees.c
      @@
     + 	o->merge_size = len;
     + 	mark_all_ce_unused(o->src_index);
     + 
     ++	if (o->src_index->fsmonitor_last_update)
     ++		o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;
     ++
     + 	/*
     + 	 * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries
     + 	 */
     +@@
       
       	if (old && same(old, a)) {
       		int update = 0;
      -		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
      +		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
     -+		    !(old->ce_flags & CE_FSMONITOR_VALID)) {
     ++			!(old->ce_flags & CE_FSMONITOR_VALID)) {
       			struct stat st;
       			if (lstat(old->name, &st) ||
       			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))

-- 
gitgitgadget

[PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Utsav Shah via GitGitGadget]

From: Utsav Shah <utsav@dropbox.com>

The index might be aware that a file hasn't modified via fsmonitor, but
unpack-trees did not pay attention to it and checked via ie_match_stat
which can be inefficient on certain filesystems. This significantly slows
down commands that run oneway_merge, like checkout and reset --hard.

This patch makes oneway_merge check whether a file is considered
unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees
also now correctly copies over fsmonitor validity state from the source
index. Finally, for correctness, we force a refresh of fsmonitor state in
tweak_fsmonitor.

After this change, commands like stash (that use reset --hard
internally) go from 8s or more to ~2s on a 250k file repository on a
mac.

Signed-off-by: Utsav Shah <utsav@dropbox.com>
---
 fsmonitor.c                       | 20 +++++++++++---------
 t/t7113-post-index-change-hook.sh |  3 ---
 t/t7519-status-fsmonitor.sh       |  9 +++++++--
 unpack-trees.c                    |  6 +++++-
 4 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/fsmonitor.c b/fsmonitor.c
index 1f4aa1b150..4362bc6ee9 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
 	}
 	istate->fsmonitor_dirty = fsmonitor_dirty;
 
-	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
+		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
+		    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
+
 
 	trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful");
 	return 0;
@@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 	uint32_t ewah_size = 0;
 	int fixup = 0;
 
-	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
+		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
+		    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
 
 	put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
 	strbuf_add(sb, &hdr_version, sizeof(uint32_t));
@@ -189,6 +190,9 @@ void refresh_fsmonitor(struct index_state *istate)
 		}
 		if (bol < query_result.len)
 			fsmonitor_refresh_callback(istate, buf + bol);
+
+		if (istate->untracked)
+			istate->untracked->use_fsmonitor = 1;
 	} else {
 		/* Mark all entries invalid */
 		for (i = 0; i < istate->cache_nr; i++)
@@ -257,9 +261,7 @@ void tweak_fsmonitor(struct index_state *istate)
 				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
 			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
 
-			/* Now mark the untracked cache for fsmonitor usage */
-			if (istate->untracked)
-				istate->untracked->use_fsmonitor = 1;
+			refresh_fsmonitor(istate);
 		}
 
 		ewah_free(istate->fsmonitor_dirty);
diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-hook.sh
index f011ad7eec..5ca2279d0d 100755
--- a/t/t7113-post-index-change-hook.sh
+++ b/t/t7113-post-index-change-hook.sh
@@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others trigger hook without flags
 	git checkout -- dir1/file1.txt &&
 	test_path_is_file testsuccess && rm -f testsuccess &&
 	test_path_is_missing testfailure &&
-	git update-index &&
-	test_path_is_missing testsuccess &&
-	test_path_is_missing testfailure &&
 	git reset --soft &&
 	test_path_is_missing testsuccess &&
 	test_path_is_missing testfailure
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index d8df990972..9cac3d3d8e 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -106,6 +106,8 @@ EOF
 
 # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit
 test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor valid bit' '
+	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	EOF
 	git update-index --fsmonitor &&
 	git update-index --fsmonitor-valid dir1/modified &&
 	git update-index --fsmonitor-valid dir2/modified &&
@@ -164,6 +166,8 @@ EOF
 
 # test that newly added files are marked valid
 test_expect_success 'newly added files are marked valid' '
+	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	EOF
 	git add new &&
 	git add dir1/new &&
 	git add dir2/new &&
@@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged
 # Ensure commands that call refresh_index() to move the index back in time
 # properly invalidate the fsmonitor cache
 test_expect_success 'refresh_index() invalidates fsmonitor cache' '
-	write_script .git/hooks/fsmonitor-test<<-\EOF &&
-	EOF
 	clean_repo &&
+	write_integration_script &&
 	dirty_repo &&
 	git add . &&
+	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	EOF
 	git commit -m "to reset" &&
 	git reset HEAD~1 &&
 	git status >actual &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 33ea7810d8..fc5ceb932c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	o->merge_size = len;
 	mark_all_ce_unused(o->src_index);
 
+	if (o->src_index->fsmonitor_last_update)
+		o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;
+
 	/*
 	 * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries
 	 */
@@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,
 
 	if (old && same(old, a)) {
 		int update = 0;
-		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
+		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
+			!(old->ce_flags & CE_FSMONITOR_VALID)) {
 			struct stat st;
 			if (lstat(old->name, &st) ||
 			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
-- 
gitgitgadget

RE: [PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Kevin Willford]

> Sent: Tuesday, November 5, 2019 8:27 AM
> From: Utsav Shah <utsav@dropbox.com>
> 
> diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-
> hook.sh
> index f011ad7eec..5ca2279d0d 100755
> --- a/t/t7113-post-index-change-hook.sh
> +++ b/t/t7113-post-index-change-hook.sh
> @@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others
> trigger hook without flags
>  	git checkout -- dir1/file1.txt &&
>  	test_path_is_file testsuccess && rm -f testsuccess &&
>  	test_path_is_missing testfailure &&
> -	git update-index &&
> -	test_path_is_missing testsuccess &&
> -	test_path_is_missing testfailure &&
>  	git reset --soft &&
>  	test_path_is_missing testsuccess &&
>  	test_path_is_missing testfailure

Looking into this change and I wonder if instead we should be updating
refresh_fsmonitor to only update istate->cache_changed if there was an
entry where CE_FSMONITOR_VALID was turned off.

The reason I bring this up is because with this change, the post-index-change
hook will behave differently depending on fsmonitor.  It will pass if
GIT_TEST_FSMONITOR is unset or set to fsmonitor-watchman. But when set
to fsmonitor-all it will fail because it is going down the code path that
invalidates all the entries and sets istate->cache_changed.

> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index
> d8df990972..9cac3d3d8e 100755
> --- a/t/t7519-status-fsmonitor.sh
> +++ b/t/t7519-status-fsmonitor.sh
> @@ -106,6 +106,8 @@ EOF
> 
>  # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit
> test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor
> valid bit' '
> +	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +	EOF
>  	git update-index --fsmonitor &&
>  	git update-index --fsmonitor-valid dir1/modified &&
>  	git update-index --fsmonitor-valid dir2/modified && @@ -164,6
> +166,8 @@ EOF
> 
>  # test that newly added files are marked valid  test_expect_success 'newly
> added files are marked valid' '
> +	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +	EOF
>  	git add new &&
>  	git add dir1/new &&
>  	git add dir2/new &&
> @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the
> integration script get flagged  # Ensure commands that call refresh_index() to
> move the index back in time  # properly invalidate the fsmonitor cache
> test_expect_success 'refresh_index() invalidates fsmonitor cache' '
> -	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> -	EOF
>  	clean_repo &&
> +	write_integration_script &&
>  	dirty_repo &&
>  	git add . &&
> +	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +	EOF
>  	git commit -m "to reset" &&
>  	git reset HEAD~1 &&
>  	git status >actual &&

We need to take a close look at all the tests in
t7519-status-fsmonitor.sh and see if we are doing the right thing with
these changes because before most commands that read the
index would only apply the bits from the fsmonitor bitmap to
the cache entries.  Whereas now, it does that but also applies what the
fsmonitor hooks returns so the content of .git/hooks/fsmonitor-test is
now affecting tests and commands where it was not before.

So if .git/hooks/fsmonitor-test has paths even git ls-files gets those
paths marked dirty and that command is being used to validate the state of
the CE_FSMONITOR_VALID.  So I think in most cases for these tests we
want the .git/hooks/fsmonitor-test to be empty before calling git ls-files
so that doesn't change the index state.

> 
>  	if (old && same(old, a)) {
>  		int update = 0;
> -		if (o->reset && o->update && !ce_uptodate(old) &&
> !ce_skip_worktree(old)) {
> +		if (o->reset && o->update && !ce_uptodate(old) &&
> !ce_skip_worktree(old) &&
> +			!(old->ce_flags & CE_FSMONITOR_VALID)) {
>  			struct stat st;
>  			if (lstat(old->name, &st) ||
>  			    ie_match_stat(o->src_index, old, &st,
> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))

FYI I have been testing with the ce_uptodate macro checking the
CE_FSMONITOR_VALID flag instead and only have failures when using
the fsmonitor-watchman script which I'm not sure if all the tests were
ever passing using it.

Re: [PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Utsav Shah]

On Tue, Nov 5, 2019 at 1:40 PM Kevin Willford
<Kevin.Willford@microsoft.com> wrote:
>
> > Sent: Tuesday, November 5, 2019 8:27 AM
> > From: Utsav Shah <utsav@dropbox.com>
> >
> > diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-
> > hook.sh
> > index f011ad7eec..5ca2279d0d 100755
> > --- a/t/t7113-post-index-change-hook.sh
> > +++ b/t/t7113-post-index-change-hook.sh
> > @@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others
> > trigger hook without flags
> >       git checkout -- dir1/file1.txt &&
> >       test_path_is_file testsuccess && rm -f testsuccess &&
> >       test_path_is_missing testfailure &&
> > -     git update-index &&
> > -     test_path_is_missing testsuccess &&
> > -     test_path_is_missing testfailure &&
> >       git reset --soft &&
> >       test_path_is_missing testsuccess &&
> >       test_path_is_missing testfailure
>
> Looking into this change and I wonder if instead we should be updating
> refresh_fsmonitor to only update istate->cache_changed if there was an
> entry where CE_FSMONITOR_VALID was turned off.
>
> The reason I bring this up is because with this change, the post-index-change
> hook will behave differently depending on fsmonitor.  It will pass if
> GIT_TEST_FSMONITOR is unset or set to fsmonitor-watchman. But when set
> to fsmonitor-all it will fail because it is going down the code path that
> invalidates all the entries and sets istate->cache_changed.

Thanks, this observation was correct. v3 of this patch will check if
the index actually needs to mark its cache as changed, and this test
passes without modification.

>
> > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index
> > d8df990972..9cac3d3d8e 100755
> > --- a/t/t7519-status-fsmonitor.sh
> > +++ b/t/t7519-status-fsmonitor.sh
> > @@ -106,6 +106,8 @@ EOF
> >
> >  # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit
> > test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor
> > valid bit' '
> > +     write_script .git/hooks/fsmonitor-test<<-\EOF &&
> > +     EOF
> >       git update-index --fsmonitor &&
> >       git update-index --fsmonitor-valid dir1/modified &&
> >       git update-index --fsmonitor-valid dir2/modified && @@ -164,6
> > +166,8 @@ EOF
> >
> >  # test that newly added files are marked valid  test_expect_success 'newly
> > added files are marked valid' '
> > +     write_script .git/hooks/fsmonitor-test<<-\EOF &&
> > +     EOF
> >       git add new &&
> >       git add dir1/new &&
> >       git add dir2/new &&
> > @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the
> > integration script get flagged  # Ensure commands that call refresh_index() to
> > move the index back in time  # properly invalidate the fsmonitor cache
> > test_expect_success 'refresh_index() invalidates fsmonitor cache' '
> > -     write_script .git/hooks/fsmonitor-test<<-\EOF &&
> > -     EOF
> >       clean_repo &&
> > +     write_integration_script &&
> >       dirty_repo &&
> >       git add . &&
> > +     write_script .git/hooks/fsmonitor-test<<-\EOF &&
> > +     EOF
> >       git commit -m "to reset" &&
> >       git reset HEAD~1 &&
> >       git status >actual &&
>
> We need to take a close look at all the tests in
> t7519-status-fsmonitor.sh and see if we are doing the right thing with
> these changes because before most commands that read the
> index would only apply the bits from the fsmonitor bitmap to
> the cache entries.  Whereas now, it does that but also applies what the
> fsmonitor hooks returns so the content of .git/hooks/fsmonitor-test is
> now affecting tests and commands where it was not before.
>
> So if .git/hooks/fsmonitor-test has paths even git ls-files gets those
> paths marked dirty and that command is being used to validate the state of
> the CE_FSMONITOR_VALID.  So I think in most cases for these tests we
> want the .git/hooks/fsmonitor-test to be empty before calling git ls-files
> so that doesn't change the index state.

I audited these tests very closely, and to the best of my knowledge,
the modifications made are valid.

For test failures of

test_expect_success 'update-index --fsmonitor-valid sets the fsmonitor
valid bit'
test_expect_success 'newly added files are marked valid'

It's relatively straightforward that our patch now runs the fsmonitor
hook so we need to make sure the hook doesn't return anything.

The trickiest case was "refresh_index()" test, and I've made a slight
change to make it clearer why that test was failing.

@@ -218,11 +222,12 @@ test_expect_success '*only* files returned by
the integration script get flagged
 # Ensure commands that call refresh_index() to move the index back in time
 # properly invalidate the fsmonitor cache
 test_expect_success 'refresh_index() invalidates fsmonitor cache' '
-       write_script .git/hooks/fsmonitor-test<<-\EOF &&
-       EOF
        clean_repo &&
        dirty_repo &&
+       write_integration_script &&
        git add . &&
+       write_script .git/hooks/fsmonitor-test<<-\EOF &&
+       EOF
        git commit -m "to reset" &&
        git reset HEAD~1 &&
        git status >actual &&

With patch v2, git add was failing to add all files, since it now
consults the fsmonitor hook which wrongly implied that no files were
modified. This was rectified by the write_integration_script. After
that, we immediately ensure that the test fsmonitor doesn't return any
files, and the test passes.


>
> >
> >       if (old && same(old, a)) {
> >               int update = 0;
> > -             if (o->reset && o->update && !ce_uptodate(old) &&
> > !ce_skip_worktree(old)) {
> > +             if (o->reset && o->update && !ce_uptodate(old) &&
> > !ce_skip_worktree(old) &&
> > +                     !(old->ce_flags & CE_FSMONITOR_VALID)) {
> >                       struct stat st;
> >                       if (lstat(old->name, &st) ||
> >                           ie_match_stat(o->src_index, old, &st,
> > CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>
> FYI I have been testing with the ce_uptodate macro checking the
> CE_FSMONITOR_VALID flag instead and only have failures when using
> the fsmonitor-watchman script which I'm not sure if all the tests were
> ever passing using it.
>

Yeah, I see the same results.

The one part that I don't fully understand if safe is copying over the
o->src_index->fsmonitor_last_update to the result index in
unpack-trees. I don't understand the implications of that, and if
that's the only field worth copying over, or if we should be copying
over other fields like the bitmap as well.

RE: [PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Kevin Willford]

> From: Utsav Shah <utsav@dropbox.com>
> Sent: Tuesday, November 5, 2019 9:36 PM
> 
> The one part that I don't fully understand if safe is copying over the
> o->src_index->fsmonitor_last_update to the result index in
> unpack-trees. I don't understand the implications of that, and if that's the
> only field worth copying over, or if we should be copying over other fields
> like the bitmap as well.

The data from the bitmap has already been applied to the index entries at
this point so it is no longer needed after that.

fsmonitor_last_update is really just being used as the flag to fill the bitmap
and write out the fsmonitor extension. The fill_fsmonitor_bitmap will use
the current CE_FSMONITOR_VALID flags for the index entries to create the
bitmap.

if (istate->fsmonitor_last_update)
	fill_fsmonitor_bitmap(istate);

void fill_fsmonitor_bitmap(struct index_state *istate)
{
	unsigned int i, skipped = 0;
	istate->fsmonitor_dirty = ewah_new();
	for (i = 0; i < istate->cache_nr; i++) {
		if (istate->cache[i]->ce_flags & CE_REMOVE)
			skipped++;
		else if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
			ewah_set(istate->fsmonitor_dirty, i - skipped);
	}
}

if (!strip_extensions && istate->fsmonitor_last_update) {
	struct strbuf sb = STRBUF_INIT;

	write_fsmonitor_extension(&sb, istate);
	err = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_FSMONITOR, sb.len) < 0
		|| ce_write(&c, newfd, sb.buf, sb.len) < 0;
	strbuf_release(&sb);
	if (err)
		return -1;
}

[PATCH v3 0/1] unpack-trees: skip stat on fsmonitor-valid files

[Utsav Shah via GitGitGadget]

The index might be aware that a file hasn't modified via fsmonitor, but
unpack-trees did not pay attention to it and checked via ie_match_stat which
can be inefficient on certain filesystems. This significantly slows down
commands that run oneway_merge, like checkout and reset --hard.

This patch makes oneway_merge check whether a file is considered unchanged
through fsmonitor and skips ie_match_stat on it. unpack-trees also now
correctly copies over fsmonitor validity state from the source index.
Finally, for correctness, we force a refresh of fsmonitor state in
tweak_fsmonitor.

After this change, commands like stash (that use reset --hard internally) go
from 8s or more to ~2s on a 250k file repository on a mac.

Signed-off-by: Utsav Shah utsav@dropbox.com [utsav@dropbox.com]

Utsav Shah (1):
  unpack-trees: skip stat on fsmonitor-valid files

 fsmonitor.c                 | 39 ++++++++++++++++++++++++-------------
 t/t7519-status-fsmonitor.sh |  9 +++++++--
 unpack-trees.c              |  6 +++++-
 3 files changed, 37 insertions(+), 17 deletions(-)


base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/424

Range-diff vs v2:

 1:  f76ba554ed ! 1:  4bea7075cf unpack-trees: skip stat on fsmonitor-valid files
     @@ -23,6 +23,15 @@
       --- a/fsmonitor.c
       +++ b/fsmonitor.c
      @@
     + 
     + 	if (pos >= istate->cache_nr)
     + 		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
     +-		    (uintmax_t)pos, istate->cache_nr);
     ++			(uintmax_t)pos, istate->cache_nr);
     + 
     + 	ce = istate->cache[pos];
     + 	ce->ce_flags &= ~CE_FSMONITOR_VALID;
     +@@
       	}
       	istate->fsmonitor_dirty = fsmonitor_dirty;
       
     @@ -31,7 +40,7 @@
      -		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
      +	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
      +		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
     -+		    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
     ++			(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
      +
       
       	trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful");
     @@ -45,7 +54,7 @@
      -		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
      +	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
      +		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
     -+		    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
     ++			(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
       
       	put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
       	strbuf_add(sb, &hdr_version, sizeof(uint32_t));
     @@ -57,10 +66,33 @@
      +		if (istate->untracked)
      +			istate->untracked->use_fsmonitor = 1;
       	} else {
     ++
     ++		/* We only want to run the post index changed hook if we've actually changed entries, so keep track
     ++		 * if we actually changed entries or not */
     ++		int is_cache_changed = 0;
       		/* Mark all entries invalid */
     - 		for (i = 0; i < istate->cache_nr; i++)
     +-		for (i = 0; i < istate->cache_nr; i++)
     +-			istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
     ++		for (i = 0; i < istate->cache_nr; i++) {
     ++			if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {
     ++				is_cache_changed = 1;
     ++				istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
     ++			}
     ++		}
     + 
     + 		/* If we're going to check every file, ensure we save the results */
     +-		istate->cache_changed |= FSMONITOR_CHANGED;
     ++		if (is_cache_changed)
     ++			istate->cache_changed |= FSMONITOR_CHANGED;
     + 
     + 		if (istate->untracked)
     + 			istate->untracked->use_fsmonitor = 0;
      @@
     - 				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
     + 			/* Mark all previously saved entries as dirty */
     + 			if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
     + 				BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
     +-				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
     ++					(uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
       			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
       
      -			/* Now mark the untracked cache for fsmonitor usage */
     @@ -71,20 +103,6 @@
       
       		ewah_free(istate->fsmonitor_dirty);
      
     - diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-hook.sh
     - --- a/t/t7113-post-index-change-hook.sh
     - +++ b/t/t7113-post-index-change-hook.sh
     -@@
     - 	git checkout -- dir1/file1.txt &&
     - 	test_path_is_file testsuccess && rm -f testsuccess &&
     - 	test_path_is_missing testfailure &&
     --	git update-index &&
     --	test_path_is_missing testsuccess &&
     --	test_path_is_missing testfailure &&
     - 	git reset --soft &&
     - 	test_path_is_missing testsuccess &&
     - 	test_path_is_missing testfailure
     -
       diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
       --- a/t/t7519-status-fsmonitor.sh
       +++ b/t/t7519-status-fsmonitor.sh
     @@ -113,8 +131,8 @@
      -	write_script .git/hooks/fsmonitor-test<<-\EOF &&
      -	EOF
       	clean_repo &&
     -+	write_integration_script &&
       	dirty_repo &&
     ++	write_integration_script &&
       	git add . &&
      +	write_script .git/hooks/fsmonitor-test<<-\EOF &&
      +	EOF

-- 
gitgitgadget

[PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Utsav Shah via GitGitGadget]

From: Utsav Shah <utsav@dropbox.com>

The index might be aware that a file hasn't modified via fsmonitor, but
unpack-trees did not pay attention to it and checked via ie_match_stat
which can be inefficient on certain filesystems. This significantly slows
down commands that run oneway_merge, like checkout and reset --hard.

This patch makes oneway_merge check whether a file is considered
unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees
also now correctly copies over fsmonitor validity state from the source
index. Finally, for correctness, we force a refresh of fsmonitor state in
tweak_fsmonitor.

After this change, commands like stash (that use reset --hard
internally) go from 8s or more to ~2s on a 250k file repository on a
mac.

Signed-off-by: Utsav Shah <utsav@dropbox.com>
---
 fsmonitor.c                 | 39 ++++++++++++++++++++++++-------------
 t/t7519-status-fsmonitor.sh |  9 +++++++--
 unpack-trees.c              |  6 +++++-
 3 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/fsmonitor.c b/fsmonitor.c
index 1f4aa1b150..04d6232531 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)
 
 	if (pos >= istate->cache_nr)
 		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
-		    (uintmax_t)pos, istate->cache_nr);
+			(uintmax_t)pos, istate->cache_nr);
 
 	ce = istate->cache[pos];
 	ce->ce_flags &= ~CE_FSMONITOR_VALID;
@@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
 	}
 	istate->fsmonitor_dirty = fsmonitor_dirty;
 
-	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
+		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
+			(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
+
 
 	trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful");
 	return 0;
@@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 	uint32_t ewah_size = 0;
 	int fixup = 0;
 
-	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
+		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
+			(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
 
 	put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
 	strbuf_add(sb, &hdr_version, sizeof(uint32_t));
@@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate)
 		}
 		if (bol < query_result.len)
 			fsmonitor_refresh_callback(istate, buf + bol);
+
+		if (istate->untracked)
+			istate->untracked->use_fsmonitor = 1;
 	} else {
+
+		/* We only want to run the post index changed hook if we've actually changed entries, so keep track
+		 * if we actually changed entries or not */
+		int is_cache_changed = 0;
 		/* Mark all entries invalid */
-		for (i = 0; i < istate->cache_nr; i++)
-			istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
+		for (i = 0; i < istate->cache_nr; i++) {
+			if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {
+				is_cache_changed = 1;
+				istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
+			}
+		}
 
 		/* If we're going to check every file, ensure we save the results */
-		istate->cache_changed |= FSMONITOR_CHANGED;
+		if (is_cache_changed)
+			istate->cache_changed |= FSMONITOR_CHANGED;
 
 		if (istate->untracked)
 			istate->untracked->use_fsmonitor = 0;
@@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate)
 			/* Mark all previously saved entries as dirty */
 			if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
 				BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+					(uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
 			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
 
-			/* Now mark the untracked cache for fsmonitor usage */
-			if (istate->untracked)
-				istate->untracked->use_fsmonitor = 1;
+			refresh_fsmonitor(istate);
 		}
 
 		ewah_free(istate->fsmonitor_dirty);
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index d8df990972..69908b6a9b 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -106,6 +106,8 @@ EOF
 
 # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit
 test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor valid bit' '
+	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	EOF
 	git update-index --fsmonitor &&
 	git update-index --fsmonitor-valid dir1/modified &&
 	git update-index --fsmonitor-valid dir2/modified &&
@@ -164,6 +166,8 @@ EOF
 
 # test that newly added files are marked valid
 test_expect_success 'newly added files are marked valid' '
+	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	EOF
 	git add new &&
 	git add dir1/new &&
 	git add dir2/new &&
@@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged
 # Ensure commands that call refresh_index() to move the index back in time
 # properly invalidate the fsmonitor cache
 test_expect_success 'refresh_index() invalidates fsmonitor cache' '
-	write_script .git/hooks/fsmonitor-test<<-\EOF &&
-	EOF
 	clean_repo &&
 	dirty_repo &&
+	write_integration_script &&
 	git add . &&
+	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	EOF
 	git commit -m "to reset" &&
 	git reset HEAD~1 &&
 	git status >actual &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 33ea7810d8..fc5ceb932c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	o->merge_size = len;
 	mark_all_ce_unused(o->src_index);
 
+	if (o->src_index->fsmonitor_last_update)
+		o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;
+
 	/*
 	 * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries
 	 */
@@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,
 
 	if (old && same(old, a)) {
 		int update = 0;
-		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
+		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
+			!(old->ce_flags & CE_FSMONITOR_VALID)) {
 			struct stat st;
 			if (lstat(old->name, &st) ||
 			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
-- 
gitgitgadget

Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Junio C Hamano]

"Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Utsav Shah <utsav@dropbox.com>
>
> The index might be aware that a file hasn't modified via fsmonitor, but
> unpack-trees did not pay attention to it and checked via ie_match_stat
> which can be inefficient on certain filesystems. This significantly slows
> down commands that run oneway_merge, like checkout and reset --hard.

s/hasn't/& been/;

Otherwise, well written.

> This patch makes oneway_merge check whether a file is considered
> unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees
> also now correctly copies over fsmonitor validity state from the source
> index. Finally, for correctness, we force a refresh of fsmonitor state in
> tweak_fsmonitor.

s/This patch makes/Make/; order the person who is updating the code
what to do to the codebase in imperative mood.

Otherwise, well written.

> After this change, commands like stash (that use reset --hard
> internally) go from 8s or more to ~2s on a 250k file repository on a
> mac.
>
> Signed-off-by: Utsav Shah <utsav@dropbox.com>
> ---
>  fsmonitor.c                 | 39 ++++++++++++++++++++++++-------------
>  t/t7519-status-fsmonitor.sh |  9 +++++++--
>  unpack-trees.c              |  6 +++++-
>  3 files changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 1f4aa1b150..04d6232531 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)
>  
>  	if (pos >= istate->cache_nr)
>  		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
> -		    (uintmax_t)pos, istate->cache_nr);
> +			(uintmax_t)pos, istate->cache_nr);

Unintended whitespace change?

> @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
>  	}
>  	istate->fsmonitor_dirty = fsmonitor_dirty;
>  
> -	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> -		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> -		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> +	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> +		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
> +			(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
> +

The patch disables this sanity check under "split index" mode and it
must be done for good reasons, but readers (imagine, somebody found
a bug on this line 6 months down the road, ran "git blame" and found
this commit and reading it via "git show") would want to know why
this change was made.

I recall seeing no mention of "split index" in the proposed log
message.  Is this a fix for unrelated issue that needs to be
explained in a separate patch, perhaps?

The hunk also has the unintended whitespace change, it seems.

> @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
>  	uint32_t ewah_size = 0;
>  	int fixup = 0;
>  
> -	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> -		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> -		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> +	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> +		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
> +			(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);

Likewise (both indentation of the second line and the unexplained
change to the sanity check condition we saw above).

> @@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate)
>  		}
>  		if (bol < query_result.len)
>  			fsmonitor_refresh_callback(istate, buf + bol);
> +
> +		if (istate->untracked)
> +			istate->untracked->use_fsmonitor = 1;

Unexplained.  We used to do this in tweak_fsmonitor() but now we do
this here, as we are making tweak_fsmonitor() to call this function
anyway.  If there are other callers that call refresh_fsmonitor()
and they did not do this, this would be a behaviour change to them.
As there is no explanation why this change is done, readers cannot
tell if it is a good change.  If this were explained like so:

    Any caller of refresh_fsmonitor() must set use_fsmonitor bit in
    istate when istate->untracked exists FOR SUCH AND SUCH REASONS.
    Move the code to do so in tweak_fsmonitor() to near the
    beginning of refresh_fsmonitor(), which would fix SUCH AND SUCH
    other callers that forgets to do this.

in the proposed log message, that might help justifying the change.

If use_fsmonitor is not set, why is the caller calling
refresh_fsmonitor() in the first place, by the way?  

Isn't it more like "we are told to use fsmonitor via
istate->untracked->use_fsmonitor bit being true, so we call
refresh_fsmonitor, and if the bit is false, we do not have to bother
with fsmonitor and no point calling refresh_fsmonitor"?

If a careless caller makes a call to refresh_fsmonitor() when the
configuration tells us not to use fsmonitor, wouldn't this cause us
to use fsmonitor anyway?  Which sounds bad, so perhaps all callers
are careful to first check if use_fsmonitor is set before deciding
to call refresh_fsmonitor()---but if that is the case, is there a
point in setting it here to true?

Puzzled by an unexplained code...

>  	} else {
> +
> +		/* We only want to run the post index changed hook if we've actually changed entries, so keep track
> +		 * if we actually changed entries or not */
> +		int is_cache_changed = 0;
>  		/* Mark all entries invalid */
> -		for (i = 0; i < istate->cache_nr; i++)
> -			istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> +		for (i = 0; i < istate->cache_nr; i++) {
> +			if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {
> +				is_cache_changed = 1;
> +				istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> +			}
> +		}
>
>  		/* If we're going to check every file, ensure we save the results */
> -		istate->cache_changed |= FSMONITOR_CHANGED;
> +		if (is_cache_changed)
> +			istate->cache_changed |= FSMONITOR_CHANGED;

This part (and a call to refresh_fsmonitor() we see blow) is the
"Finally, we force a refresh" explained in the proposed log message,
I presume.

>  		if (istate->untracked)
>  			istate->untracked->use_fsmonitor = 0;
> @@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate)
>  			/* Mark all previously saved entries as dirty */
>  			if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
>  				BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> -				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> +					(uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);

This shares the same indentation issue but does not disable the
sanity check for split index case.  Intended?  Without explanation
in the proposed log message, readers cannot tell.

>  			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
>  
> -			/* Now mark the untracked cache for fsmonitor usage */
> -			if (istate->untracked)
> -				istate->untracked->use_fsmonitor = 1;
> +			refresh_fsmonitor(istate);
>  		}
>  
>  		ewah_free(istate->fsmonitor_dirty);
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 33ea7810d8..fc5ceb932c 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  	o->merge_size = len;
>  	mark_all_ce_unused(o->src_index);
>  
> +	if (o->src_index->fsmonitor_last_update)
> +		o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;
> +

This is the "correctly copies" part, which was well explained.

>  	/*
>  	 * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries
>  	 */
> @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,
>  
>  	if (old && same(old, a)) {
>  		int update = 0;
> -		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
> +		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
> +			!(old->ce_flags & CE_FSMONITOR_VALID)) {

This is the "skip when we know it is valid" part, which was well
explained.

>  			struct stat st;
>  			if (lstat(old->name, &st) ||
>  			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))

Thanks.

Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Utsav Shah]

On Wed, Nov 6, 2019 at 2:46 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Utsav Shah <utsav@dropbox.com>
> >
> > The index might be aware that a file hasn't modified via fsmonitor, but
> > unpack-trees did not pay attention to it and checked via ie_match_stat
> > which can be inefficient on certain filesystems. This significantly slows
> > down commands that run oneway_merge, like checkout and reset --hard.
>
> s/hasn't/& been/;
>
> Otherwise, well written.
>
> > This patch makes oneway_merge check whether a file is considered
> > unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees
> > also now correctly copies over fsmonitor validity state from the source
> > index. Finally, for correctness, we force a refresh of fsmonitor state in
> > tweak_fsmonitor.
>
> s/This patch makes/Make/; order the person who is updating the code
> what to do to the codebase in imperative mood.
>
> Otherwise, well written.
>
> > After this change, commands like stash (that use reset --hard
> > internally) go from 8s or more to ~2s on a 250k file repository on a
> > mac.
> >
> > Signed-off-by: Utsav Shah <utsav@dropbox.com>
> > ---
> >  fsmonitor.c                 | 39 ++++++++++++++++++++++++-------------
> >  t/t7519-status-fsmonitor.sh |  9 +++++++--
> >  unpack-trees.c              |  6 +++++-
> >  3 files changed, 37 insertions(+), 17 deletions(-)
> >
> > diff --git a/fsmonitor.c b/fsmonitor.c
> > index 1f4aa1b150..04d6232531 100644
> > --- a/fsmonitor.c
> > +++ b/fsmonitor.c
> > @@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)
> >
> >       if (pos >= istate->cache_nr)
> >               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
> > -                 (uintmax_t)pos, istate->cache_nr);
> > +                     (uintmax_t)pos, istate->cache_nr);
>
> Unintended whitespace change?
>
> > @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
> >       }
> >       istate->fsmonitor_dirty = fsmonitor_dirty;
> >
> > -     if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > -             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> > -                 (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> > +     if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > +             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
> > +                     (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
> > +
>
> The patch disables this sanity check under "split index" mode and it
> must be done for good reasons, but readers (imagine, somebody found
> a bug on this line 6 months down the road, ran "git blame" and found
> this commit and reading it via "git show") would want to know why
> this change was made.
>
> I recall seeing no mention of "split index" in the proposed log
> message.  Is this a fix for unrelated issue that needs to be
> explained in a separate patch, perhaps?
>
> The hunk also has the unintended whitespace change, it seems.
>
> > @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
> >       uint32_t ewah_size = 0;
> >       int fixup = 0;
> >
> > -     if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > -             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> > -                 (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> > +     if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > +             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
> > +                     (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
>
> Likewise (both indentation of the second line and the unexplained
> change to the sanity check condition we saw above).
>
> > @@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate)
> >               }
> >               if (bol < query_result.len)
> >                       fsmonitor_refresh_callback(istate, buf + bol);
> > +
> > +             if (istate->untracked)
> > +                     istate->untracked->use_fsmonitor = 1;
>
> Unexplained.  We used to do this in tweak_fsmonitor() but now we do
> this here, as we are making tweak_fsmonitor() to call this function
> anyway.  If there are other callers that call refresh_fsmonitor()
> and they did not do this, this would be a behaviour change to them.
> As there is no explanation why this change is done, readers cannot
> tell if it is a good change.  If this were explained like so:
>
>     Any caller of refresh_fsmonitor() must set use_fsmonitor bit in
>     istate when istate->untracked exists FOR SUCH AND SUCH REASONS.
>     Move the code to do so in tweak_fsmonitor() to near the
>     beginning of refresh_fsmonitor(), which would fix SUCH AND SUCH
>     other callers that forgets to do this.
>
> in the proposed log message, that might help justifying the change.
>
> If use_fsmonitor is not set, why is the caller calling
> refresh_fsmonitor() in the first place, by the way?
>
> Isn't it more like "we are told to use fsmonitor via
> istate->untracked->use_fsmonitor bit being true, so we call
> refresh_fsmonitor, and if the bit is false, we do not have to bother
> with fsmonitor and no point calling refresh_fsmonitor"?
>
> If a careless caller makes a call to refresh_fsmonitor() when the
> configuration tells us not to use fsmonitor, wouldn't this cause us
> to use fsmonitor anyway?  Which sounds bad, so perhaps all callers
> are careful to first check if use_fsmonitor is set before deciding
> to call refresh_fsmonitor()---but if that is the case, is there a
> point in setting it here to true?
>
> Puzzled by an unexplained code...
>
> >       } else {
> > +
> > +             /* We only want to run the post index changed hook if we've actually changed entries, so keep track
> > +              * if we actually changed entries or not */
> > +             int is_cache_changed = 0;
> >               /* Mark all entries invalid */
> > -             for (i = 0; i < istate->cache_nr; i++)
> > -                     istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> > +             for (i = 0; i < istate->cache_nr; i++) {
> > +                     if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {
> > +                             is_cache_changed = 1;
> > +                             istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> > +                     }
> > +             }
> >
> >               /* If we're going to check every file, ensure we save the results */
> > -             istate->cache_changed |= FSMONITOR_CHANGED;
> > +             if (is_cache_changed)
> > +                     istate->cache_changed |= FSMONITOR_CHANGED;
>
> This part (and a call to refresh_fsmonitor() we see blow) is the
> "Finally, we force a refresh" explained in the proposed log message,
> I presume.
>
> >               if (istate->untracked)
> >                       istate->untracked->use_fsmonitor = 0;
> > @@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate)
> >                       /* Mark all previously saved entries as dirty */
> >                       if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> >                               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> > -                                 (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> > +                                     (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
>
> This shares the same indentation issue but does not disable the
> sanity check for split index case.  Intended?  Without explanation
> in the proposed log message, readers cannot tell.
>
> >                       ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
> >
> > -                     /* Now mark the untracked cache for fsmonitor usage */
> > -                     if (istate->untracked)
> > -                             istate->untracked->use_fsmonitor = 1;
> > +                     refresh_fsmonitor(istate);
> >               }
> >
> >               ewah_free(istate->fsmonitor_dirty);
> > diff --git a/unpack-trees.c b/unpack-trees.c
> > index 33ea7810d8..fc5ceb932c 100644
> > --- a/unpack-trees.c
> > +++ b/unpack-trees.c
> > @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> >       o->merge_size = len;
> >       mark_all_ce_unused(o->src_index);
> >
> > +     if (o->src_index->fsmonitor_last_update)
> > +             o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;
> > +
>
> This is the "correctly copies" part, which was well explained.
>
> >       /*
> >        * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries
> >        */
> > @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,
> >
> >       if (old && same(old, a)) {
> >               int update = 0;
> > -             if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
> > +             if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
> > +                     !(old->ce_flags & CE_FSMONITOR_VALID)) {
>
> This is the "skip when we know it is valid" part, which was well
> explained.
>
> >                       struct stat st;
> >                       if (lstat(old->name, &st) ||
> >                           ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>
> Thanks.

Thanks for the feedback. The lines with the indentation changes had
tabs and spaces mixed up, but I'll revert those changes.

Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Utsav Shah]

On Wed, Nov 6, 2019 at 2:33 PM Utsav Shah <utsav@dropbox.com> wrote:
>
> On Wed, Nov 6, 2019 at 2:46 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > From: Utsav Shah <utsav@dropbox.com>
> > >
> > > The index might be aware that a file hasn't modified via fsmonitor, but
> > > unpack-trees did not pay attention to it and checked via ie_match_stat
> > > which can be inefficient on certain filesystems. This significantly slows
> > > down commands that run oneway_merge, like checkout and reset --hard.
> >
> > s/hasn't/& been/;
> >
> > Otherwise, well written.
> >
> > > This patch makes oneway_merge check whether a file is considered
> > > unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees
> > > also now correctly copies over fsmonitor validity state from the source
> > > index. Finally, for correctness, we force a refresh of fsmonitor state in
> > > tweak_fsmonitor.
> >
> > s/This patch makes/Make/; order the person who is updating the code
> > what to do to the codebase in imperative mood.
> >
> > Otherwise, well written.
> >
> > > After this change, commands like stash (that use reset --hard
> > > internally) go from 8s or more to ~2s on a 250k file repository on a
> > > mac.
> > >
> > > Signed-off-by: Utsav Shah <utsav@dropbox.com>
> > > ---
> > >  fsmonitor.c                 | 39 ++++++++++++++++++++++++-------------
> > >  t/t7519-status-fsmonitor.sh |  9 +++++++--
> > >  unpack-trees.c              |  6 +++++-
> > >  3 files changed, 37 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/fsmonitor.c b/fsmonitor.c
> > > index 1f4aa1b150..04d6232531 100644
> > > --- a/fsmonitor.c
> > > +++ b/fsmonitor.c
> > > @@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)
> > >
> > >       if (pos >= istate->cache_nr)
> > >               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
> > > -                 (uintmax_t)pos, istate->cache_nr);
> > > +                     (uintmax_t)pos, istate->cache_nr);
> >
> > Unintended whitespace change?
> >
> > > @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
> > >       }
> > >       istate->fsmonitor_dirty = fsmonitor_dirty;
> > >
> > > -     if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > > -             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> > > -                 (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> > > +     if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > > +             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
> > > +                     (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
> > > +
> >
> > The patch disables this sanity check under "split index" mode and it
> > must be done for good reasons, but readers (imagine, somebody found
> > a bug on this line 6 months down the road, ran "git blame" and found
> > this commit and reading it via "git show") would want to know why
> > this change was made.
> >
> > I recall seeing no mention of "split index" in the proposed log
> > message.  Is this a fix for unrelated issue that needs to be
> > explained in a separate patch, perhaps?

Yes. I think this is an important bug to fix, fsmonitor and split
indices don't seem to be interoperating well after these checks have
been added.

> >
> > The hunk also has the unintended whitespace change, it seems.
> >
> > > @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
> > >       uint32_t ewah_size = 0;
> > >       int fixup = 0;
> > >
> > > -     if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > > -             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> > > -                 (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> > > +     if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > > +             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
> > > +                     (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
> >
> > Likewise (both indentation of the second line and the unexplained
> > change to the sanity check condition we saw above).
> >
> > > @@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate)
> > >               }
> > >               if (bol < query_result.len)
> > >                       fsmonitor_refresh_callback(istate, buf + bol);
> > > +
> > > +             if (istate->untracked)
> > > +                     istate->untracked->use_fsmonitor = 1;
> >
> > Unexplained.  We used to do this in tweak_fsmonitor() but now we do
> > this here, as we are making tweak_fsmonitor() to call this function
> > anyway.  If there are other callers that call refresh_fsmonitor()
> > and they did not do this, this would be a behaviour change to them.
> > As there is no explanation why this change is done, readers cannot
> > tell if it is a good change.  If this were explained like so:
> >
> >     Any caller of refresh_fsmonitor() must set use_fsmonitor bit in
> >     istate when istate->untracked exists FOR SUCH AND SUCH REASONS.
> >     Move the code to do so in tweak_fsmonitor() to near the
> >     beginning of refresh_fsmonitor(), which would fix SUCH AND SUCH
> >     other callers that forgets to do this.
> >
> > in the proposed log message, that might help justifying the change.
> >
> > If use_fsmonitor is not set, why is the caller calling
> > refresh_fsmonitor() in the first place, by the way?
> >
> > Isn't it more like "we are told to use fsmonitor via
> > istate->untracked->use_fsmonitor bit being true, so we call
> > refresh_fsmonitor, and if the bit is false, we do not have to bother
> > with fsmonitor and no point calling refresh_fsmonitor"?
> >
> > If a careless caller makes a call to refresh_fsmonitor() when the
> > configuration tells us not to use fsmonitor, wouldn't this cause us
> > to use fsmonitor anyway?  Which sounds bad, so perhaps all callers
> > are careful to first check if use_fsmonitor is set before deciding
> > to call refresh_fsmonitor()---but if that is the case, is there a
> > point in setting it here to true?
> >
> > Puzzled by an unexplained code...

I might be misunderstanding, but wouldn't the if condition above make
sure we don't enter this codepath at all?

if (!core_fsmonitor || istate->fsmonitor_has_run_once)
        return;

istate->untracked->use_fsmonitor is only used in the untracked cache
to skip some lstats AFAICT. So callers refresh_fsmonitor should see no
difference.

I agree that this change deserves a better explanation.

> >
> > >       } else {
> > > +
> > > +             /* We only want to run the post index changed hook if we've actually changed entries, so keep track
> > > +              * if we actually changed entries or not */
> > > +             int is_cache_changed = 0;
> > >               /* Mark all entries invalid */
> > > -             for (i = 0; i < istate->cache_nr; i++)
> > > -                     istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> > > +             for (i = 0; i < istate->cache_nr; i++) {
> > > +                     if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {
> > > +                             is_cache_changed = 1;
> > > +                             istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> > > +                     }
> > > +             }
> > >
> > >               /* If we're going to check every file, ensure we save the results */
> > > -             istate->cache_changed |= FSMONITOR_CHANGED;
> > > +             if (is_cache_changed)
> > > +                     istate->cache_changed |= FSMONITOR_CHANGED;
> >
> > This part (and a call to refresh_fsmonitor() we see blow) is the
> > "Finally, we force a refresh" explained in the proposed log message,
> > I presume.
> >
> > >               if (istate->untracked)
> > >                       istate->untracked->use_fsmonitor = 0;
> > > @@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate)
> > >                       /* Mark all previously saved entries as dirty */
> > >                       if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > >                               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> > > -                                 (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> > > +                                     (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> >
> > This shares the same indentation issue but does not disable the
> > sanity check for split index case.  Intended?  Without explanation
> > in the proposed log message, readers cannot tell.

Yes, will add the sanity check here too.

> >
> > >                       ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
> > >
> > > -                     /* Now mark the untracked cache for fsmonitor usage */
> > > -                     if (istate->untracked)
> > > -                             istate->untracked->use_fsmonitor = 1;
> > > +                     refresh_fsmonitor(istate);
> > >               }
> > >
> > >               ewah_free(istate->fsmonitor_dirty);
> > > diff --git a/unpack-trees.c b/unpack-trees.c
> > > index 33ea7810d8..fc5ceb932c 100644
> > > --- a/unpack-trees.c
> > > +++ b/unpack-trees.c
> > > @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> > >       o->merge_size = len;
> > >       mark_all_ce_unused(o->src_index);
> > >
> > > +     if (o->src_index->fsmonitor_last_update)
> > > +             o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;
> > > +
> >
> > This is the "correctly copies" part, which was well explained.
> >
> > >       /*
> > >        * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries
> > >        */
> > > @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,
> > >
> > >       if (old && same(old, a)) {
> > >               int update = 0;
> > > -             if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
> > > +             if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
> > > +                     !(old->ce_flags & CE_FSMONITOR_VALID)) {
> >
> > This is the "skip when we know it is valid" part, which was well
> > explained.
> >
> > >                       struct stat st;
> > >                       if (lstat(old->name, &st) ||
> > >                           ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
> >
> > Thanks.
>
> Thanks for the feedback. The lines with the indentation changes had
> tabs and spaces mixed up, but I'll revert those changes.

I will try to figure out how to use gitgitgadget to send a smaller
patch within this thread to fix the fsmonitor and split index
interactions. I tried writing a test today that uses
t7519/fsmonitor-watchman to simulate the bug, but it is flaky.

Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Junio C Hamano]

Utsav Shah <utsav@dropbox.com> writes:

> I will try to figure out how to use gitgitgadget to send a smaller
> patch within this thread to fix the fsmonitor and split index
> interactions.

If the current one is trying to handle two issues, then you'd need
to reduce this one and add another spearate topic, probably.

> I tried writing a test today that uses
> t7519/fsmonitor-watchman to simulate the bug, but it is flaky.

Are you aware of Kevin Willford's recent work?

<52eaf20f405b53743e27935dfe89f62e6f824a33.1572889841.git.gitgitgadget@gmail.com>

Re: [PATCH v3 0/1] unpack-trees: skip stat on fsmonitor-valid files

[Junio C Hamano]

"Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes:

> The index might be aware that a file hasn't modified via fsmonitor, but
> unpack-trees did not pay attention to it and checked via ie_match_stat which
> can be inefficient on certain filesystems. This significantly slows down
> commands that run oneway_merge, like checkout and reset --hard.
>
> This patch makes oneway_merge check whether a file is considered unchanged
> through fsmonitor and skips ie_match_stat on it. unpack-trees also now
> correctly copies over fsmonitor validity state from the source index.
> Finally, for correctness, we force a refresh of fsmonitor state in
> tweak_fsmonitor.
>
> After this change, commands like stash (that use reset --hard internally) go
> from 8s or more to ~2s on a 250k file repository on a mac.

If the above is verbatim copy of what you wrote in 1/1, that is very
much unappreciated X-<.  A cover letter for a topic serves primarily
just one purpose:

   It is a good place to present a birds-eye-view of a multi-patch
   topic; a high level description of the problem (e.g. how the
   issue manifests to the end users), an explanation of division of
   the problem into subproblems you made (if applicable), and
   interesting highlights of the solution would all be good things
   to have in there.

And as a topic goes through iterations, it gives you a good place to
summarize what changed since the previously reviewed iterations.  It
could be just a single liner "addressed all the review comments for
the previous iteration".  A well-written multi-patch topic also uses
the same after-three-dash technique used for a single-patch topic
(see below) to summarize what changed since the corresponding patch
in the series in the previous iteration (or just says "no changes
since the previous round"---that helps the reviewers a lot).
   
For a single-patch topic, there is no place for "here is an overall
birds-eyes-view picture because the changes described in the
proposed log message of individual patches are so big and complex".
A single-patch topic has one patch, that solves one problem and only
that problem well.  When you want to summarize the changes since the
previous iteration, you would write it between the three-dash-line
(which appears after your sign-off) and the diffstat.

Thanks.

[PATCH v4 0/1] unpack-trees: skip stat on fsmonitor-valid files

[Utsav Shah via GitGitGadget]

The index might be aware that a file hasn't modified via fsmonitor, but
unpack-trees did not pay attention to it and checked via ie_match_stat which
can be inefficient on certain filesystems. This significantly slows down
commands that run oneway_merge, like checkout and reset --hard.

This patch makes oneway_merge check whether a file is considered unchanged
through fsmonitor and skips ie_match_stat on it. unpack-trees also now
correctly copies over fsmonitor validity state from the source index.
Finally, for correctness, we force a refresh of fsmonitor state in
tweak_fsmonitor.

After this change, commands like stash (that use reset --hard internally) go
from 8s or more to ~2s on a 250k file repository on a mac.

Changes since the last version are:

 * The sanity checks around accessing the fsmonitor_dirty bitmap have been
   moved to another patch, which is in message id [1]
 * Unintended indentation changes in fsmonitor have been removed
 * A comment explaining what untracked->use_fsmonitor means has been
   re-added (it was dropped in the previous version)
 * A few "helped-by" entries have been added to the patch

1: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com)

Helped-by: Junio C Hamano gitster@pobox.com [gitster@pobox.com]Helped-by:
Kevin Willford Kevin.Willford@microsoft.com [Kevin.Willford@microsoft.com]
Signed-off-by: Utsav Shah utsav@dropbox.com [utsav@dropbox.com]

Utsav Shah (1):
  unpack-trees: skip stat on fsmonitor-valid files

 fsmonitor.c                 | 23 +++++++++++++++++------
 t/t7519-status-fsmonitor.sh |  9 +++++++--
 unpack-trees.c              |  6 +++++-
 3 files changed, 29 insertions(+), 9 deletions(-)


base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/424

Range-diff vs v3:

 1:  4bea7075cf ! 1:  ea7880f2d0 unpack-trees: skip stat on fsmonitor-valid files
     @@ -17,52 +17,29 @@
          internally) go from 8s or more to ~2s on a 250k file repository on a
          mac.
      
     +    Changes since the last version are:
     +    * The sanity checks around accessing the fsmonitor_dirty bitmap have
     +    been moved to another patch, which is in message id [1]
     +    * Unintended indentation changes in fsmonitor have been removed
     +    * A comment explaining what untracked->use_fsmonitor means has been
     +    re-added (it was dropped in the previous version)
     +    * A few "helped-by" entries have been added to the patch
     +
     +    [1]: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com)
     +
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
     +    Helped-by: Kevin Willford <Kevin.Willford@microsoft.com>
          Signed-off-by: Utsav Shah <utsav@dropbox.com>
      
       diff --git a/fsmonitor.c b/fsmonitor.c
       --- a/fsmonitor.c
       +++ b/fsmonitor.c
      @@
     - 
     - 	if (pos >= istate->cache_nr)
     - 		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
     --		    (uintmax_t)pos, istate->cache_nr);
     -+			(uintmax_t)pos, istate->cache_nr);
     - 
     - 	ce = istate->cache[pos];
     - 	ce->ce_flags &= ~CE_FSMONITOR_VALID;
     -@@
     - 	}
     - 	istate->fsmonitor_dirty = fsmonitor_dirty;
     - 
     --	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
     --		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
     --		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
     -+	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
     -+		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
     -+			(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
     -+
     - 
     - 	trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful");
     - 	return 0;
     -@@
     - 	uint32_t ewah_size = 0;
     - 	int fixup = 0;
     - 
     --	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
     --		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
     --		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
     -+	if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
     -+		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
     -+			(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
     - 
     - 	put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
     - 	strbuf_add(sb, &hdr_version, sizeof(uint32_t));
     -@@
       		}
       		if (bol < query_result.len)
       			fsmonitor_refresh_callback(istate, buf + bol);
      +
     ++		/* Now mark the untracked cache for fsmonitor usage */
      +		if (istate->untracked)
      +			istate->untracked->use_fsmonitor = 1;
       	} else {
     @@ -88,11 +65,7 @@
       		if (istate->untracked)
       			istate->untracked->use_fsmonitor = 0;
      @@
     - 			/* Mark all previously saved entries as dirty */
     - 			if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
     - 				BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
     --				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
     -+					(uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
     + 				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
       			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
       
      -			/* Now mark the untracked cache for fsmonitor usage */

-- 
gitgitgadget

[PATCH v4 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Utsav Shah via GitGitGadget]

From: Utsav Shah <utsav@dropbox.com>

The index might be aware that a file hasn't modified via fsmonitor, but
unpack-trees did not pay attention to it and checked via ie_match_stat
which can be inefficient on certain filesystems. This significantly slows
down commands that run oneway_merge, like checkout and reset --hard.

This patch makes oneway_merge check whether a file is considered
unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees
also now correctly copies over fsmonitor validity state from the source
index. Finally, for correctness, we force a refresh of fsmonitor state in
tweak_fsmonitor.

After this change, commands like stash (that use reset --hard
internally) go from 8s or more to ~2s on a 250k file repository on a
mac.

Changes since the last version are:
* The sanity checks around accessing the fsmonitor_dirty bitmap have
been moved to another patch, which is in message id [1]
* Unintended indentation changes in fsmonitor have been removed
* A comment explaining what untracked->use_fsmonitor means has been
re-added (it was dropped in the previous version)
* A few "helped-by" entries have been added to the patch

[1]: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com)

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Kevin Willford <Kevin.Willford@microsoft.com>
Signed-off-by: Utsav Shah <utsav@dropbox.com>
---
 fsmonitor.c                 | 23 +++++++++++++++++------
 t/t7519-status-fsmonitor.sh |  9 +++++++--
 unpack-trees.c              |  6 +++++-
 3 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/fsmonitor.c b/fsmonitor.c
index 1f4aa1b150..0d270da80f 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -189,13 +189,26 @@ void refresh_fsmonitor(struct index_state *istate)
 		}
 		if (bol < query_result.len)
 			fsmonitor_refresh_callback(istate, buf + bol);
+
+		/* Now mark the untracked cache for fsmonitor usage */
+		if (istate->untracked)
+			istate->untracked->use_fsmonitor = 1;
 	} else {
+
+		/* We only want to run the post index changed hook if we've actually changed entries, so keep track
+		 * if we actually changed entries or not */
+		int is_cache_changed = 0;
 		/* Mark all entries invalid */
-		for (i = 0; i < istate->cache_nr; i++)
-			istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
+		for (i = 0; i < istate->cache_nr; i++) {
+			if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {
+				is_cache_changed = 1;
+				istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
+			}
+		}
 
 		/* If we're going to check every file, ensure we save the results */
-		istate->cache_changed |= FSMONITOR_CHANGED;
+		if (is_cache_changed)
+			istate->cache_changed |= FSMONITOR_CHANGED;
 
 		if (istate->untracked)
 			istate->untracked->use_fsmonitor = 0;
@@ -257,9 +270,7 @@ void tweak_fsmonitor(struct index_state *istate)
 				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
 			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
 
-			/* Now mark the untracked cache for fsmonitor usage */
-			if (istate->untracked)
-				istate->untracked->use_fsmonitor = 1;
+			refresh_fsmonitor(istate);
 		}
 
 		ewah_free(istate->fsmonitor_dirty);
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index d8df990972..69908b6a9b 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -106,6 +106,8 @@ EOF
 
 # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit
 test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor valid bit' '
+	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	EOF
 	git update-index --fsmonitor &&
 	git update-index --fsmonitor-valid dir1/modified &&
 	git update-index --fsmonitor-valid dir2/modified &&
@@ -164,6 +166,8 @@ EOF
 
 # test that newly added files are marked valid
 test_expect_success 'newly added files are marked valid' '
+	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	EOF
 	git add new &&
 	git add dir1/new &&
 	git add dir2/new &&
@@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged
 # Ensure commands that call refresh_index() to move the index back in time
 # properly invalidate the fsmonitor cache
 test_expect_success 'refresh_index() invalidates fsmonitor cache' '
-	write_script .git/hooks/fsmonitor-test<<-\EOF &&
-	EOF
 	clean_repo &&
 	dirty_repo &&
+	write_integration_script &&
 	git add . &&
+	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	EOF
 	git commit -m "to reset" &&
 	git reset HEAD~1 &&
 	git status >actual &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 33ea7810d8..fc5ceb932c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	o->merge_size = len;
 	mark_all_ce_unused(o->src_index);
 
+	if (o->src_index->fsmonitor_last_update)
+		o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;
+
 	/*
 	 * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries
 	 */
@@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,
 
 	if (old && same(old, a)) {
 		int update = 0;
-		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
+		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
+			!(old->ce_flags & CE_FSMONITOR_VALID)) {
 			struct stat st;
 			if (lstat(old->name, &st) ||
 			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
-- 
gitgitgadget

Re: [PATCH v4 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Junio C Hamano]

"Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Utsav Shah <utsav@dropbox.com>
>
> The index might be aware that a file hasn't modified via fsmonitor, but
> unpack-trees did not pay attention to it and checked via ie_match_stat
> which can be inefficient on certain filesystems. This significantly slows
> down commands that run oneway_merge, like checkout and reset --hard.
>
> This patch makes oneway_merge check whether a file is considered
> unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees
> also now correctly copies over fsmonitor validity state from the source
> index. Finally, for correctness, we force a refresh of fsmonitor state in
> tweak_fsmonitor.

Instead of saying "also now correctly copies..." as if it started
working correctly by accident, be more assertive and actively make
it so ;-)

    Check if a file is unchanged by fsmonitor in oneway_merge(), and
    avoid unnecessary calls to ie_match_stat().  Copy the fsmonitor
    validity state from the source index to the destination index in
    unpack_trees().  Force a refresh of the fsmonitor state in
    tweak_fsmonitor(), which is called after the index file is read
    from the disk, for correctness.

perhaps.

> After this change, commands like stash (that use reset --hard
> internally) go from 8s or more to ~2s on a 250k file repository on a
> mac.

Good.

>
> Changes since the last version are:
> * The sanity checks around accessing the fsmonitor_dirty bitmap have
> been moved to another patch, which is in message id [1]
> * Unintended indentation changes in fsmonitor have been removed
> * A comment explaining what untracked->use_fsmonitor means has been
> re-added (it was dropped in the previous version)
> * A few "helped-by" entries have been added to the patch
>
> [1]: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com)

The above is for the cover letter or after the three-dash lines, and
not for the log message.

> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Kevin Willford <Kevin.Willford@microsoft.com>
> Signed-off-by: Utsav Shah <utsav@dropbox.com>
> ---
>  fsmonitor.c                 | 23 +++++++++++++++++------
>  t/t7519-status-fsmonitor.sh |  9 +++++++--
>  unpack-trees.c              |  6 +++++-
>  3 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 1f4aa1b150..0d270da80f 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -189,13 +189,26 @@ void refresh_fsmonitor(struct index_state *istate)
>  		}
>  		if (bol < query_result.len)
>  			fsmonitor_refresh_callback(istate, buf + bol);
> +
> +		/* Now mark the untracked cache for fsmonitor usage */
> +		if (istate->untracked)
> +			istate->untracked->use_fsmonitor = 1;
>  	} else {
> +
> +		/* We only want to run the post index changed hook if we've actually changed entries, so keep track
> +		 * if we actually changed entries or not */

Multi-line comment style.

> +		int is_cache_changed = 0;
>  		/* Mark all entries invalid */
> -		for (i = 0; i < istate->cache_nr; i++)
> -			istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> +		for (i = 0; i < istate->cache_nr; i++) {

Lack of blank line between the last decl and the first stmt.
Probably the blank should go before "/* Mark all ...".

> @@ -257,9 +270,7 @@ void tweak_fsmonitor(struct index_state *istate)
>  				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
>  			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
>  
> -			/* Now mark the untracked cache for fsmonitor usage */
> -			if (istate->untracked)
> -				istate->untracked->use_fsmonitor = 1;
> +			refresh_fsmonitor(istate);
>  		}
>  
>  		ewah_free(istate->fsmonitor_dirty);

Looks good.

Thanks.