use fsmonitor data in git diff eliminating O(num_files) calls to lstat

Credit to alexmv who made this commit back in Dec, 2017 when he was at dbx. I've rebased it and am submitting it now.

With fsmonitor enabled, git diff currently lstats every file in the repo This makes use of the fsmonitor extension to skip lstat() calls on files that fsmonitor judged as unmodified.

I was able to do some testing with/without this change in a large in-house repo (~ 400k files)

-----------------------------------------
(1) With fsmonitor enabled - on master of git (2.29.0)
-----------------------------------------
../git/bin-wrappers/git checkout HEAD~200 strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
99.64 4.358994 10 446257 3 lstat
0.12 0.005353 7 764 360 open

(A subsequent call)
strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
99.84 4.380955 10 444904 3 lstat
0.06 0.002564 135 19 munmap
...

-----------------------------------------
(2) With fsmonitor enabled - with my patch
-----------------------------------------
../git/bin-wrappers/git checkout HEAD~200
strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------

 50.72    0.003090         163        19           munmap
 19.63    0.001196         598         2           futex

...
0.00 0.000000 0 4 3 lstat

-----------------------------------------
(3) With fsmonitor disabled entirely
-----------------------------------------

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
98.52 0.277085 92362 3 futex
0.27 0.000752 4 191 63 open
...
0.14 0.000397 3 158 3 lstat

I encoded this into a perf test with results as follow:

On master (2.29)

Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         2.52(1.59+1.56)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.18(0.12+0.06)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.36(0.73+0.62)
7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman)           0.85(0.30+0.54)
7519.7: status (fsmonitor=)                                      0.69(0.52+0.90)
7519.8: status -uno (fsmonitor=)                                 0.37(0.28+0.81)
7519.9: status -uall (fsmonitor=)                                1.53(0.93+1.32)
7519.10: diff (fsmonitor=)                                       0.34(0.26+0.81)

With this patch

Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         2.84(1.70+1.76)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.18(0.13+0.05)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.35(0.81+0.53)
7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman)           0.15(0.11+0.05)
7519.7: status (fsmonitor=)                                      0.71(0.54+0.90)
7519.8: status -uno (fsmonitor=)                                 0.38(0.30+0.81)
7519.9: status -uall (fsmonitor=)                                1.55(0.93+1.34)
7519.10: diff (fsmonitor=)                                       0.35(0.32+0.76)

Alex Vandiver (1):
fsmonitor: use fsmonitor data in git diff

Nipunn Koorapati (3):
t/perf/README: elaborate on output format t/perf/p7519-fsmonitor.sh: warm cache on first git status t/perf: add fsmonitor perf test for git diff

 diff-lib.c                | 17 +++++++++++++++--
 t/perf/README             |  2 ++
 t/perf/p7519-fsmonitor.sh | 19 ++++++++++++++++++-
 3 files changed, 35 insertions(+), 3 deletions(-)

base-commit: d4a392452e292ff924e79ec8458611c0f679d6d4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-756%2Fnipunn1313%2Fdiff_fsmon-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-756/nipunn1313/diff_fsmon-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/756

gitgitgadget

From: Alex Vandiver <[email protected]>

With fsmonitor enabled, the first call to match_stat_with_submodule calls refresh_fsmonitor, incurring the overhead of reading the list of updated files -- but run_diff_files does not respect the CE_FSMONITOR_VALID flag.

Make use of the fsmonitor extension to skip lstat() calls on files that fsmonitor judged as unmodified.

Notably, this change improves performance of the git shell prompt when GIT_PS1_SHOWDIRTYSTATE is set.

---
 diff-lib.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

index f95c6de75f..b7ee1b89ef 100644
@@ -97,6 +97,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
 
+	refresh_fsmonitor(istate);
+
 	if (diff_unmerged_stage < 0)
 		diff_unmerged_stage = 2;
 	entries = istate->cache_nr;

@@ -197,8 +199,19 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		if (ce_uptodate(ce) || ce_skip_worktree(ce))
 			continue;
 
-		/* If CE_VALID is set, don't look at workdir for file removal */
-		if (ce->ce_flags & CE_VALID) {
+		/*
+		 * If CE_VALID is set, the user has promised us that the workdir
+		 * hasn't changed compared to index, so don't stat workdir
+		 * for file removal
+		 *  eg - via git udpate-index --assume-unchanged
+		 *  eg - via core.ignorestat=true
+		 *
+		 * When using FSMONITOR:
+		 * If CE_FSMONITOR_VALID is set, then we know the metadata on disk
+		 * has not changed since the last refresh, and we can skip the
+		 * file-removal checks without doing the stat in check_removed.
+		 */
+		if (ce->ce_flags & CE_VALID || ce->ce_flags & CE_FSMONITOR_VALID) {
 			changed = 0;
 			newmode = ce->ce_mode;
 		} else {

gitgitgadget

From: Nipunn Koorapati <[email protected]>

---
 t/perf/README | 2 ++
 1 file changed, 2 insertions(+)

index bd649afa97..fb9127a66f 100644
@@ -28,6 +28,8 @@ the tests on the current git repository.
     7810.3: grep --cached, cheap regex       3.07(3.02+0.25)
     7810.4: grep --cached, expensive regex   9.39(30.57+0.24)
 
+Output format is in seconds "Elapsed(User + System)"
+
 You can compare multiple repositories and even git revisions with the
 'run' script:
 

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Results for the git-diff fsmonitor optimization
in patch in the parent-rev (using a 400k file repo to test)

As you can see here - git diff with fsmonitor running is significantly better with this patch series (80% faster on my workload)!

On master (2.29)

Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         0.39(0.33+0.06)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.17(0.13+0.05)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.34(0.77+0.56)
7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman)           0.82(0.24+0.58)
7519.7: status (fsmonitor=)                                      0.70(0.53+0.90)
7519.8: status -uno (fsmonitor=)                                 0.37(0.32+0.78)
7519.9: status -uall (fsmonitor=)                                1.55(1.01+1.25)
7519.10: diff (fsmonitor=)                                       0.34(0.35+0.72)

With this patch series

Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         0.39(0.33+0.07)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.17(0.12+0.05)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.35(0.73+0.61)
7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman)           0.14(0.10+0.05)
7519.7: status (fsmonitor=)                                      0.70(0.56+0.87)
7519.8: status -uno (fsmonitor=)                                 0.37(0.31+0.79)
7519.9: status -uall (fsmonitor=)                                1.54(0.97+1.29)
7519.10: diff (fsmonitor=)                                       0.34(0.28+0.79)

---
 t/perf/p7519-fsmonitor.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

index 9313d4a51d..80d0148557 100755
@@ -142,6 +142,14 @@ test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
 	git status -uall
 '
 
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff
+'
+
 test_expect_success "setup without fsmonitor" '
 	unset INTEGRATION_SCRIPT &&
 	git config --unset core.fsmonitor &&

@@ -172,6 +180,14 @@ test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
 	git status -uall
 '
 
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff
+'
+
 if test_have_prereq WATCHMAN
 then
 	watchman watch-del "$GIT_WORK_TREE" >/dev/null 2>&1 &&

gitgitgadget

From: Nipunn Koorapati <[email protected]>

The first git status would be inflated due to warming of filesystem cache. This makes the results comparable.

Before
Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         2.52(1.59+1.56)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.18(0.12+0.06)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.36(0.73+0.62)
7519.7: status (fsmonitor=)                                      0.69(0.52+0.90)
7519.8: status -uno (fsmonitor=)                                 0.37(0.28+0.81)
7519.9: status -uall (fsmonitor=)                                1.53(0.93+1.32)

After
Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         0.39(0.33+0.06)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.17(0.13+0.05)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.34(0.77+0.56)
7519.7: status (fsmonitor=)                                      0.70(0.53+0.90)
7519.8: status -uno (fsmonitor=)                                 0.37(0.32+0.78)
7519.9: status -uall (fsmonitor=)                                1.55(1.01+1.25)

---
 t/perf/p7519-fsmonitor.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index def7ecdbc7..9313d4a51d 100755
@@ -114,7 +114,8 @@ test_expect_success "setup for fsmonitor" '
 	fi &&
 
 	git config core.fsmonitor "$INTEGRATION_SCRIPT" &&
-	git update-index --fsmonitor
+	git update-index --fsmonitor &&
+	git status  # Warm caches
 '
 
 if test -n "$GIT_PERF_7519_DROP_CACHE"; then

gitgitgadget

run_diff_files() is used not just by "git diff" but other things like "git add", so if we get an overall speed-up without having to pay undue cost, that would be a very good news.

		if (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {

Agreed! I may be able to write perf benchmark tests to highlight benefits to git add as well.

My understanding is that refresh_fsmonitor is O(delta_since_last_refresh) - so for developers
with large repositories - this cost will amortize out over subsequent commands, so I don't
think it's worth investigating this tradeoff here.
As a user of large repositories, I expect that my major source of fsmonitor activity to be user
intent (eg git pull, or intentionally copying/editing a large number of files). After such a command,
I expect my next git command to be slower - that would be unsurprising.

I think the tradeoff could be made for small diff requests, but I don't think it's worth adding complexity here -
as that user will just have to pay the cost on their next git command.

Intended to indicate potential ways that CE_VALID might be set. When I was reading the source
here, it was pretty difficult to determine how this would be set. Agree that I picked unfortunate wording.
Thanks for the suggestions. Will update in the next iteration.

Sounds good. Will clarify. I like your comment better as well.

I personally find this more confusing because it involves multiple bitwise ops, but this
is potentially due to me having more mental practice thinking about boolean operators vs bitwise operators.
I'm more than happy to align with the common pattern of the repo. I'll change this.

Thank you for the thorough review!

Hmm. I do agree that I'd like to stay out of the business of trying to figure out exactly what that trade-off is (although I'm sure that it exists), only because it seems likely to vary to a large extent from repository to repository. (That is, 20% may be a good number for some repository, but a terrible choice for another).

But, I think that we can invoke watchman better here; the fsmonitor-watchman hook has no notion of a "pathspec", so every query just asks for everything that isn't in '$GIT_DIR'. Is there anything preventing us from taking an optional pathspec and building up a more targeted query?

There is some overhead to invoke the hook and talk to watchman, but I'd expect that to be dwarfed by not having to issue O(# files) syscalls.

I don't have an opinion, nor do I think that git.git has an established practice of doing one over the other. For what it's worth, my two-cents is that Junio's suggestion is easier to read.

Thanks,
Taylor

Note that you can directly compare results with the perf suite's "run" script by passing two revisions in addition to the test that you want to run and have the results aggregated side-by-side.

In your case, you'd want something like (within the t/perf directory):

  $ ./run HEAD . p7519-*.sh

where this patch is the uncommitted state (alternatively you could compare the two revisions directly in the case that you have already committed).

I think both of you misunderstood me.

My question was a simple yes/no "does there a trade off exist?" question and the sentences with 20% in it were mere example of possible trade-off I had in mind that _could_ exist. I wasn't even suggesting to figure out what the optimum cut-off heuristics would be (e.g. solving "when more than N% paths are subject to diff fsmonitor is faster" for N).

I was hoping that we can show that even having to lstat just a single path is expensive enough---IOW, "there is no trade-off worth worrying about, because talking to fsmonitor is so cheap compared to the cost of even a single lstst" would have been a valid and happy answer. With such a number, there is no risk of introducing an unwarranted performance regression to use cases that we did not anticipate by adding an unconditional call to refresh_fsmonitor().

But without any rationale, the performance implication of adding an unconditional call to refresh_fsmonitor() would become much muddier.

Yup, it is what I had in mind when I brought up the pathspec. It may be something worth pursuing longer term, but not within the scope of this patch.

"invoke the hook"---is that a pipe+fork+exec, or something else that is far lighter-weight?

n

Aha; thanks for clarifying. I'm glad we agree that finding 'N' would not be worth it, or at least that showing that talking to fsmonitor is cheaper than a single lstat would be more worthwhile.

Nipunn - I don't have fsmonitor/watchman setup on my workstation, but if you do, some numbers (or an interpretation of the numbers you already provided) on this would be really useful. If you don't have it set up, or don't have time to measure it, let me know, and I'd be happy to take a look.

The former; see 'fsmonitor.c:query_fsmonitor()'.

Thanks,
Taylor

It brings us back to the "overhead of how many lstat(2) takes us closer to the overhead of a single pipe+fork+exec plus reading from the pipe", doesn't it?

Somewhat unfortunately, yes. Hopefully any user that cares to use fsmonitor has enough files in their repository that a pipe+fork+exec is still faster than however many lstats they would have needed otherwise.

Of course, finding out what that number is is still interesting...

Thanks,
Taylor

I will add a benchmark for a git diff -- <pathspec>

I can try to do some manual testing to figure this out. Doesn't seem like the type of thing we'd want to add to the benchmark, as it would involve running git diff on a variety of pathspec workloads

--Nipunn

From: Alex Vandiver <[email protected]>

With fsmonitor enabled, the first call to match_stat_with_submodule calls refresh_fsmonitor, incurring the overhead of reading the list of updated files -- but run_diff_files does not respect the CE_FSMONITOR_VALID flag.

Make use of the fsmonitor extension to skip lstat() calls on files that fsmonitor judged as unmodified.

Notably, this change improves performance of the git shell prompt when GIT_PS1_SHOWDIRTYSTATE is set.

---
 diff-lib.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

index f95c6de75f..d2d31b9f82 100644
@@ -97,6 +97,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
 
+	refresh_fsmonitor(istate);
+
 	if (diff_unmerged_stage < 0)
 		diff_unmerged_stage = 2;
 	entries = istate->cache_nr;

@@ -197,8 +199,17 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		if (ce_uptodate(ce) || ce_skip_worktree(ce))
 			continue;
 
-		/* If CE_VALID is set, don't look at workdir for file removal */
-		if (ce->ce_flags & CE_VALID) {
+		/*
+		 * When CE_VALID is set (via "update-index --assume-unchanged"
+		 * or via adding paths while core.ignorestat is set to true),
+		 * the user has promised that the working tree file for that
+		 * path will not be modified.  When CE_FSMONITOR_VALID is true,
+		 * the fsmonitor knows that the path hasn't been modified since
+		 * we refreshed the cached stat information.  In either case,
+		 * we do not have to stat to see if the path has been removed
+		 * or modified.
+		 */
+		if (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {
 			changed = 0;
 			newmode = ce->ce_mode;
 		} else {

gitgitgadget

Credit to alexmv who made this commit back in Dec, 2017 when he was at dbx. I've rebased it and am submitting it now.

With fsmonitor enabled, git diff currently lstats every file in the repo This makes use of the fsmonitor extension to skip lstat() calls on files that fsmonitor judged as unmodified.

I was able to do some testing with/without this change in a large in-house repo (~ 400k files).

-----------------------------------------
(1) With fsmonitor enabled - on master of git (2.29.0)
-----------------------------------------
../git/bin-wrappers/git checkout HEAD~200 strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
99.64 4.358994 10 446257 3 lstat
0.12 0.005353 7 764 360 open

(A subsequent call)
strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
99.84 4.380955 10 444904 3 lstat
0.06 0.002564 135 19 munmap
...

-----------------------------------------
(2) With fsmonitor enabled - with my patch
-----------------------------------------
../git/bin-wrappers/git checkout HEAD~200
strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------

 50.72    0.003090         163        19           munmap
 19.63    0.001196         598         2           futex

...
0.00 0.000000 0 4 3 lstat

-----------------------------------------
(3) With fsmonitor disabled entirely
-----------------------------------------

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
98.52 0.277085 92362 3 futex
0.27 0.000752 4 191 63 open
...
0.14 0.000397 3 158 3 lstat

I was able to encode this into a perf test in one of the commits.

Changes since Patch Series V1

* Add git diff -- <pathspec> to perf tests
* improve readability of bitwise ops

Alex Vandiver (1):
fsmonitor: use fsmonitor data in git diff

Nipunn Koorapati (3):
t/perf/README: elaborate on output format t/perf/p7519-fsmonitor.sh: warm cache on first git status t/perf: add fsmonitor perf test for git diff

 diff-lib.c                | 15 ++++++--
 t/perf/README             |  2 ++
 t/perf/p7519-fsmonitor.sh | 74 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 88 insertions(+), 3 deletions(-)

base-commit: d4a392452e292ff924e79ec8458611c0f679d6d4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-756%2Fnipunn1313%2Fdiff_fsmon-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-756/nipunn1313/diff_fsmon-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/756

Range-diff vs v1:

1: 13fd992a37 ! 1: cba03dd40b fsmonitor: use fsmonitor data in git diff
@@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)
- /* If CE_VALID is set, don't look at workdir for file removal */
- if (ce->ce_flags & CE_VALID) {
+ /*
-+ * If CE_VALID is set, the user has promised us that the workdir
-+ * hasn't changed compared to index, so don't stat workdir
-+ * for file removal
-+ * eg - via git udpate-index --assume-unchanged
-+ * eg - via core.ignorestat=true
-+ *
-+ * When using FSMONITOR:
-+ * If CE_FSMONITOR_VALID is set, then we know the metadata on disk
-+ * has not changed since the last refresh, and we can skip the
-+ * file-removal checks without doing the stat in check_removed.
++ * When CE_VALID is set (via "update-index --assume-unchanged"
++ * or via adding paths while core.ignorestat is set to true),
++ * the user has promised that the working tree file for that
++ * path will not be modified. When CE_FSMONITOR_VALID is true,
++ * the fsmonitor knows that the path hasn't been modified since
++ * we refreshed the cached stat information. In either case,
++ * we do not have to stat to see if the path has been removed
++ * or modified.
+ */
-+ if (ce->ce_flags & CE_VALID || ce->ce_flags & CE_FSMONITOR_VALID) {
++ if (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {
changed = 0;
newmode = ce->ce_mode;
} else {

 2:  024cd07965 = 2:  1c7876166f t/perf/README: elaborate on output format
 3:  6482e372bc = 3:  401f696c81 t/perf/p7519-fsmonitor.sh: warm cache on first git status
 4:  0613b07676 ! 4:  f572e226bb t/perf: add fsmonitor perf test for git diff

@@ Commit message
significantly better with this patch series (80% faster on my workload)!

- On master (2.29)
+ GIT_PERF_LARGE_REPO=~/src/server ./run v2.29.0-rc1 . -- p7519-fsmonitor.sh

- Test this tree
- --------------------------------------------------------------------------------
- 7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 0.39(0.33+0.06)
- 7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.17(0.13+0.05)
- 7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.34(0.77+0.56)
- 7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.82(0.24+0.58)
- 7519.7: status (fsmonitor=) 0.70(0.53+0.90)
- 7519.8: status -uno (fsmonitor=) 0.37(0.32+0.78)
- 7519.9: status -uall (fsmonitor=) 1.55(1.01+1.25)
- 7519.10: diff (fsmonitor=) 0.34(0.35+0.72)
+ Test v2.29.0-rc1 this tree
+ -----------------------------------------------------------------------------------------------------------------
+ 7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 1.46(0.82+0.64) 1.47(0.83+0.62) +0.7%
+ 7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.16(0.12+0.04) 0.17(0.12+0.05) +6.3%
+ 7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62) 1.37(0.76+0.60) +0.7%
+ 7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.85(0.22+0.63) 0.14(0.10+0.05) -83.5%
+ 7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.05) 0.13(0.11+0.02) +8.3%
+ 7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.04) 0.13(0.09+0.04) +8.3%
+ 7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.07+0.05) 0.13(0.07+0.06) +8.3%
+ 7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.09+0.04) 0.13(0.08+0.05) +8.3%
+ 7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.14(0.09+0.05) 0.13(0.10+0.03) -7.1%
+ 7519.12: status (fsmonitor=) 1.67(0.93+1.49) 1.67(0.99+1.42) +0.0%
+ 7519.13: status -uno (fsmonitor=) 0.37(0.30+0.82) 0.37(0.33+0.79) +0.0%
+ 7519.14: status -uall (fsmonitor=) 1.58(0.97+1.35) 1.57(0.86+1.45) -0.6%
+ 7519.15: diff (fsmonitor=) 0.34(0.28+0.83) 0.34(0.27+0.83) +0.0%
+ 7519.16: diff -- 0_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.08+0.02) +0.0%
+ 7519.17: diff -- 10_files (fsmonitor=) 0.09(0.07+0.03) 0.09(0.06+0.05) +0.0%
+ 7519.18: diff -- 100_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.06+0.04) +0.0%
+ 7519.19: diff -- 1000_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.05+0.05) +0.0%
+ 7519.20: diff -- 10000_files (fsmonitor=) 0.10(0.08+0.04) 0.10(0.06+0.05) +0.0%

- With this patch series
+ I also added a benchmark for a tiny git diff workload w/ a pathspec.
+ I see an approximately .02 second overhead added w/ and w/o fsmonitor

- Test this tree
- --------------------------------------------------------------------------------
- 7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 0.39(0.33+0.07)
- 7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.17(0.12+0.05)
- 7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.35(0.73+0.61)
- 7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.14(0.10+0.05)
- 7519.7: status (fsmonitor=) 0.70(0.56+0.87)
- 7519.8: status -uno (fsmonitor=) 0.37(0.31+0.79)
- 7519.9: status -uall (fsmonitor=) 1.54(0.97+1.29)
- 7519.10: diff (fsmonitor=) 0.34(0.28+0.79)
+ From looking at these results, I suspected that refresh_fsmonitor
+ is already happening during git diff - independent of this patch
+ series' optimization. Confirmed that suspicion by breaking on
+ refresh_fsmonitor.
+
+ (gdb) bt [simplified]
+ 0 refresh_fsmonitor at fsmonitor.c:176
+ 1 ie_match_stat at read-cache.c:375
+ 2 match_stat_with_submodule at diff-lib.c:237
+ 4 builtin_diff_files at builtin/diff.c:260
+ 5 cmd_diff at builtin/diff.c:541
+ 6 run_builtin at git.c:450
+ 7 handle_builtin at git.c:700
+ 8 run_argv at git.c:767
+ 9 cmd_main at git.c:898
+ 10 main at common-main.c:52

## t/perf/p7519-fsmonitor.sh ##
+@@ t/perf/p7519-fsmonitor.sh: test_expect_success "setup for fsmonitor" ' +
+ git config core.fsmonitor "$INTEGRATION_SCRIPT" &&
+ git update-index --fsmonitor &&
++ mkdir 1_file 10_files 100_files 1000_files 10000_files &&
++ for i in seq 1 10; do touch 10_files/$i; done &&
++ for i in seq 1 100; do touch 100_files/$i; done &&
++ for i in seq 1 1000; do touch 1000_files/$i; done &&
++ for i in seq 1 10000; do touch 10000_files/$i; done &&
++ git add 1_file 10_files 100_files 1000_files 10000_files &&
++ git commit -m "Add files" &&
+ git status # Warm caches
+ '
+
@@ t/perf/p7519-fsmonitor.sh: test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
git status -uall
'
@@ t/perf/p7519-fsmonitor.sh: test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIP
+test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+ git diff
+'
++
++if test -n "$GIT_PERF_7519_DROP_CACHE"; then
++ test-tool drop-caches
++fi
++
++test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
++ git diff -- 1_file
++'
++
++test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
++ git diff -- 10_files
++'
++
++test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
++ git diff -- 100_files
++'
++
++test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
++ git diff -- 1000_files
++'
++
++test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
++ git diff -- 10000_files
++'
+
test_expect_success "setup without fsmonitor" '
unset INTEGRATION_SCRIPT &&
@@ t/perf/p7519-fsmonitor.sh: test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIP
+test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+ git diff
+'
++
++if test -n "$GIT_PERF_7519_DROP_CACHE"; then
++ test-tool drop-caches
++fi
++
++test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
++ git diff -- 1_file
++'
++
++test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
++ git diff -- 10_files
++'
++
++test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
++ git diff -- 100_files
++'
++
++test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
++ git diff -- 1000_files
++'
++
++test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
++ git diff -- 10000_files
++'
+
if test_have_prereq WATCHMAN
then

gitgitgadget

From: Nipunn Koorapati <[email protected]>

---
 t/perf/README | 2 ++
 1 file changed, 2 insertions(+)

index bd649afa97..fb9127a66f 100644
@@ -28,6 +28,8 @@ the tests on the current git repository.
     7810.3: grep --cached, cheap regex       3.07(3.02+0.25)
     7810.4: grep --cached, expensive regex   9.39(30.57+0.24)
 
+Output format is in seconds "Elapsed(User + System)"
+
 You can compare multiple repositories and even git revisions with the
 'run' script:
 

gitgitgadget

From: Nipunn Koorapati <[email protected]>

The first git status would be inflated due to warming of filesystem cache. This makes the results comparable.

Before
Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         2.52(1.59+1.56)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.18(0.12+0.06)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.36(0.73+0.62)
7519.7: status (fsmonitor=)                                      0.69(0.52+0.90)
7519.8: status -uno (fsmonitor=)                                 0.37(0.28+0.81)
7519.9: status -uall (fsmonitor=)                                1.53(0.93+1.32)

After
Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         0.39(0.33+0.06)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.17(0.13+0.05)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.34(0.77+0.56)
7519.7: status (fsmonitor=)                                      0.70(0.53+0.90)
7519.8: status -uno (fsmonitor=)                                 0.37(0.32+0.78)
7519.9: status -uall (fsmonitor=)                                1.55(1.01+1.25)

---
 t/perf/p7519-fsmonitor.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index def7ecdbc7..9313d4a51d 100755
@@ -114,7 +114,8 @@ test_expect_success "setup for fsmonitor" '
 	fi &&
 
 	git config core.fsmonitor "$INTEGRATION_SCRIPT" &&
-	git update-index --fsmonitor
+	git update-index --fsmonitor &&
+	git status  # Warm caches
 '
 
 if test -n "$GIT_PERF_7519_DROP_CACHE"; then

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Results for the git-diff fsmonitor optimization
in patch in the parent-rev (using a 400k file repo to test)

As you can see here - git diff with fsmonitor running is significantly better with this patch series (80% faster on my workload)!

GIT_PERF_LARGE_REPO=~/src/server ./run v2.29.0-rc1 . -- p7519-fsmonitor.sh

Test v2.29.0-rc1 this tree
-----------------------------------------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)                 1.46(0.82+0.64)   1.47(0.83+0.62) +0.7%
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)            0.16(0.12+0.04)   0.17(0.12+0.05) +6.3%
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)           1.36(0.73+0.62)   1.37(0.76+0.60) +0.7%
7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman)                   0.85(0.22+0.63)   0.14(0.10+0.05) -83.5%
7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman)        0.12(0.08+0.05)   0.13(0.11+0.02) +8.3%
7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman)       0.12(0.08+0.04)   0.13(0.09+0.04) +8.3%
7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman)      0.12(0.07+0.05)   0.13(0.07+0.06) +8.3%
7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman)     0.12(0.09+0.04)   0.13(0.08+0.05) +8.3%
7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman)   0.14(0.09+0.05)   0.13(0.10+0.03) -7.1%
7519.12: status (fsmonitor=)                                             1.67(0.93+1.49)   1.67(0.99+1.42) +0.0%
7519.13: status -uno (fsmonitor=)                                        0.37(0.30+0.82)   0.37(0.33+0.79) +0.0%
7519.14: status -uall (fsmonitor=)                                       1.58(0.97+1.35)   1.57(0.86+1.45) -0.6%
7519.15: diff (fsmonitor=)                                               0.34(0.28+0.83)   0.34(0.27+0.83) +0.0%
7519.16: diff -- 0_files (fsmonitor=)                                    0.09(0.06+0.04)   0.09(0.08+0.02) +0.0%
7519.17: diff -- 10_files (fsmonitor=)                                   0.09(0.07+0.03)   0.09(0.06+0.05) +0.0%
7519.18: diff -- 100_files (fsmonitor=)                                  0.09(0.06+0.04)   0.09(0.06+0.04) +0.0%
7519.19: diff -- 1000_files (fsmonitor=)                                 0.09(0.06+0.04)   0.09(0.05+0.05) +0.0%
7519.20: diff -- 10000_files (fsmonitor=)                                0.10(0.08+0.04)   0.10(0.06+0.05) +0.0%

I also added a benchmark for a tiny git diff workload w/ a pathspec. I see an approximately .02 second overhead added w/ and w/o fsmonitor

From looking at these results, I suspected that refresh_fsmonitor is already happening during git diff - independent of this patch series' optimization. Confirmed that suspicion by breaking on refresh_fsmonitor.

(gdb) bt [simplified]

0  refresh_fsmonitor  at fsmonitor.c:176
1  ie_match_stat  at read-cache.c:375
2  match_stat_with_submodule at diff-lib.c:237
4  builtin_diff_files  at builtin/diff.c:260
5  cmd_diff  at builtin/diff.c:541
6  run_builtin  at git.c:450
7  handle_builtin  at git.c:700
8  run_argv  at git.c:767
9  cmd_main  at git.c:898

10 main at common-main.c:52

---
 t/perf/p7519-fsmonitor.sh | 71 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

index 9313d4a51d..2b4803707f 100755
@@ -115,6 +115,13 @@ test_expect_success "setup for fsmonitor" '
 
 	git config core.fsmonitor "$INTEGRATION_SCRIPT" &&
 	git update-index --fsmonitor &&
+	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
+	for i in `seq 1 10`; do touch 10_files/$i; done &&
+	for i in `seq 1 100`; do touch 100_files/$i; done &&
+	for i in `seq 1 1000`; do touch 1000_files/$i; done &&
+	for i in `seq 1 10000`; do touch 10000_files/$i; done &&
+	git add 1_file 10_files 100_files 1000_files 10000_files &&
+	git commit -m "Add files" &&
 	git status  # Warm caches
 '
 

@@ -142,6 +149,38 @@ test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
 	git status -uall
 '
 
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff
+'
+
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1_file
+'
+
+test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10_files
+'
+
+test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 100_files
+'
+
+test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1000_files
+'
+
+test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10000_files
+'
+
 test_expect_success "setup without fsmonitor" '
 	unset INTEGRATION_SCRIPT &&
 	git config --unset core.fsmonitor &&

@@ -172,6 +211,38 @@ test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
 	git status -uall
 '
 
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff
+'
+
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1_file
+'
+
+test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10_files
+'
+
+test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 100_files
+'
+
+test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1000_files
+'
+
+test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10000_files
+'
+
 if test_have_prereq WATCHMAN
 then
 	watchman watch-del "$GIT_WORK_TREE" >/dev/null 2>&1 &&

gitgitgadget

These t/perf numbers are very helpful, at least to me.

Looks like about 0.01sec of overhead, which seems like an acceptable trade-off for when the user has at least 10,000 files.

This reminds me; did you look at the 'git add' performance change? I recall Junio mentioning that 'git add' takes the same paths in the code.

OK... so having fsmonitor turned on adds an imperceptible amount of slow-down to cases where there are [0, 10000) files. But, in exchange, you get much-improved whole-tree performance, as well as single-tree performance when that tree contains at least 10,000 files.

I was going to say that this has little downside, because turning on fsmonitor is probably a good indicator that you don't have any fewer than 10,000 files in your repository, but I think that's missing the point. Likely true, but that doesn't exclude the possibility of having sub-10,000 file directories, which users may very well still be diff-ing.

So, there's a slow-down, but it's hard to complain when you consider what we get in exchange.

Great! No slow-down without fsmonitor enabled, as expected. Fantastic.

So, the overhead that we're paying is purely the pipe+fork+exec? I.e., that watchman has already computed an answer in the earlier call, and we just have to read it again (or find out that the last results were unchanged)?

:-).

Makefile issue is easy enough to fix - will fix in another commit

Thanks; I don't think that you need to worry about touching up t/perf's Makefile (although I'd be very grateful if you did!), but rather just swapping new invocations of seq to use 'test_seq()' would be sufficient in and of itself.

Thanks,
Taylor

I did look into it and didn't see a big perf change - and didn't dig into why. I'll leave a perf test in the next roll of this patch series so you can see the numbers.

--Nipunn

Credit to alexmv who made this commit back in Dec, 2017 when he was at dbx. I've rebased it and am submitting it now.

With fsmonitor enabled, git diff currently lstats every file in the repo This makes use of the fsmonitor extension to skip lstat() calls on files that fsmonitor judged as unmodified.

I was able to do some testing with/without this change in a large in-house repo (~ 400k files).

-----------------------------------------
(1) With fsmonitor enabled - on master of git (2.29.0)
-----------------------------------------
../git/bin-wrappers/git checkout HEAD~200 strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
99.64 4.358994 10 446257 3 lstat
0.12 0.005353 7 764 360 open

(A subsequent call)
strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
99.84 4.380955 10 444904 3 lstat
0.06 0.002564 135 19 munmap
...

-----------------------------------------
(2) With fsmonitor enabled - with my patch
-----------------------------------------
../git/bin-wrappers/git checkout HEAD~200
strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------

 50.72    0.003090         163        19           munmap
 19.63    0.001196         598         2           futex

...
0.00 0.000000 0 4 3 lstat

-----------------------------------------
(3) With fsmonitor disabled entirely
-----------------------------------------

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
98.52 0.277085 92362 3 futex
0.27 0.000752 4 191 63 open
...
0.14 0.000397 3 158 3 lstat

I was able to encode this into a perf test in one of the commits.

Changes since Patch Series V1

* Add git diff -- <pathspec> to perf tests
* improve readability of bitwise ops

Alex Vandiver (1):
fsmonitor: use fsmonitor data in git diff

Nipunn Koorapati (6):
t/perf/README: elaborate on output format t/perf/p7519-fsmonitor.sh: warm cache on first git status t/perf: add fsmonitor perf test for git diff perf lint: check test-lint-shell-syntax in perf tests
p7519-fsmonitor: refactor to avoid code duplication
p7519-fsmonitor: add a git add benchmark

 diff-lib.c                | 15 +++++-
 t/Makefile                |  3 +-
 t/perf/README             |  2 +
 t/perf/p3400-rebase.sh    |  6 +--
 t/perf/p7519-fsmonitor.sh | 96 ++++++++++++++++++++++-----------------
 5 files changed, 75 insertions(+), 47 deletions(-)

base-commit: d4a392452e292ff924e79ec8458611c0f679d6d4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-756%2Fnipunn1313%2Fdiff_fsmon-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-756/nipunn1313/diff_fsmon-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/756

Range-diff vs v2:

 1:  cba03dd40b = 1:  cba03dd40b fsmonitor: use fsmonitor data in `git diff`
 2:  1c7876166f = 2:  1c7876166f t/perf/README: elaborate on output format
 3:  401f696c81 = 3:  401f696c81 t/perf/p7519-fsmonitor.sh: warm cache on first git status
 4:  f572e226bb ! 4:  b3ad8faac4 t/perf: add fsmonitor perf test for git diff
     @@ t/perf/p7519-fsmonitor.sh: test_expect_success "setup for fsmonitor" '
       	git config core.fsmonitor "$INTEGRATION_SCRIPT" &&
       	git update-index --fsmonitor &&
      +	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
     -+	for i in `seq 1 10`; do touch 10_files/$i; done &&
     -+	for i in `seq 1 100`; do touch 100_files/$i; done &&
     -+	for i in `seq 1 1000`; do touch 1000_files/$i; done &&
     -+	for i in `seq 1 10000`; do touch 10000_files/$i; done &&
     ++	for i in $(test_seq 1 10); do touch 10_files/$i; done &&
     ++	for i in $(test_seq 1 100); do touch 100_files/$i; done &&
     ++	for i in $(test_seq 1 1000); do touch 1000_files/$i; done &&
     ++	for i in $(test_seq 1 10000); do touch 10000_files/$i; done &&
      +	git add 1_file 10_files 100_files 1000_files 10000_files &&
      +	git commit -m "Add files" &&
       	git status  # Warm caches
 -:  ---------- > 5:  28c1e488bf perf lint: check test-lint-shell-syntax in perf tests
 -:  ---------- > 6:  b38f2984f9 p7519-fsmonitor: refactor to avoid code duplication
 -:  ---------- > 7:  d392a523f2 p7519-fsmonitor: add a git add benchmark

gitgitgadget

From: Alex Vandiver <[email protected]>

With fsmonitor enabled, the first call to match_stat_with_submodule calls refresh_fsmonitor, incurring the overhead of reading the list of updated files -- but run_diff_files does not respect the CE_FSMONITOR_VALID flag.

Make use of the fsmonitor extension to skip lstat() calls on files that fsmonitor judged as unmodified.

Notably, this change improves performance of the git shell prompt when GIT_PS1_SHOWDIRTYSTATE is set.

---
 diff-lib.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

index f95c6de75f..d2d31b9f82 100644
@@ -97,6 +97,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
 
+	refresh_fsmonitor(istate);
+
 	if (diff_unmerged_stage < 0)
 		diff_unmerged_stage = 2;
 	entries = istate->cache_nr;

@@ -197,8 +199,17 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		if (ce_uptodate(ce) || ce_skip_worktree(ce))
 			continue;
 
-		/* If CE_VALID is set, don't look at workdir for file removal */
-		if (ce->ce_flags & CE_VALID) {
+		/*
+		 * When CE_VALID is set (via "update-index --assume-unchanged"
+		 * or via adding paths while core.ignorestat is set to true),
+		 * the user has promised that the working tree file for that
+		 * path will not be modified.  When CE_FSMONITOR_VALID is true,
+		 * the fsmonitor knows that the path hasn't been modified since
+		 * we refreshed the cached stat information.  In either case,
+		 * we do not have to stat to see if the path has been removed
+		 * or modified.
+		 */
+		if (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {
 			changed = 0;
 			newmode = ce->ce_mode;
 		} else {

gitgitgadget

From: Nipunn Koorapati <[email protected]>

The first git status would be inflated due to warming of filesystem cache. This makes the results comparable.

Before
Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         2.52(1.59+1.56)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.18(0.12+0.06)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.36(0.73+0.62)
7519.7: status (fsmonitor=)                                      0.69(0.52+0.90)
7519.8: status -uno (fsmonitor=)                                 0.37(0.28+0.81)
7519.9: status -uall (fsmonitor=)                                1.53(0.93+1.32)

After
Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         0.39(0.33+0.06)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.17(0.13+0.05)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.34(0.77+0.56)
7519.7: status (fsmonitor=)                                      0.70(0.53+0.90)
7519.8: status -uno (fsmonitor=)                                 0.37(0.32+0.78)
7519.9: status -uall (fsmonitor=)                                1.55(1.01+1.25)

---
 t/perf/p7519-fsmonitor.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index def7ecdbc7..9313d4a51d 100755
@@ -114,7 +114,8 @@ test_expect_success "setup for fsmonitor" '
 	fi &&
 
 	git config core.fsmonitor "$INTEGRATION_SCRIPT" &&
-	git update-index --fsmonitor
+	git update-index --fsmonitor &&
+	git status  # Warm caches
 '
 
 if test -n "$GIT_PERF_7519_DROP_CACHE"; then

gitgitgadget

From: Nipunn Koorapati <[email protected]>

---
 t/perf/README | 2 ++
 1 file changed, 2 insertions(+)

index bd649afa97..fb9127a66f 100644
@@ -28,6 +28,8 @@ the tests on the current git repository.
     7810.3: grep --cached, cheap regex       3.07(3.02+0.25)
     7810.4: grep --cached, expensive regex   9.39(30.57+0.24)
 
+Output format is in seconds "Elapsed(User + System)"
+
 You can compare multiple repositories and even git revisions with the
 'run' script:
 

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Perf tests have some seq instead of test_seq. This runs the existing tests on the perf tests as well.

---
 t/Makefile             | 3 ++-
 t/perf/p3400-rebase.sh | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

index c83fd18861..74b53af2bd 100644
@@ -34,6 +34,7 @@ CHAINLINTTMP_SQ = $(subst ','\'',$(CHAINLINTTMP))
 T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
 TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
 THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))
+TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
 CHAINLINT = sed -f chainlint.sed
 

@@ -91,7 +92,7 @@ test-lint-executable:
 		echo >&2 "non-executable tests:" $$bad; exit 1; }
 
 test-lint-shell-syntax:
-	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
+	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)
 
 test-lint-filenames:
 	@# We do *not* pass a glob to ls-files but use grep instead, to catch

index d202aaed06..7a0bb29448 100755
@@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '
 	git checkout -f -B base &&
 	git checkout -B to-rebase &&
 	git checkout -B upstream &&
-	for i in $(seq 100)
+	for i in $(test_seq 100)
 	do
 		# simulate huge diffs
 		echo change$i >unrelated-file$i &&
-		seq 1000 >>unrelated-file$i &&
+		test_seq 1000 >>unrelated-file$i &&
 		git add unrelated-file$i &&
 		test_tick &&
 		git commit -m commit$i unrelated-file$i &&
 		echo change$i >unrelated-file$i &&
-		seq 1000 | tac >>unrelated-file$i &&
+		test_seq 1000 | tac >>unrelated-file$i &&
 		git add unrelated-file$i &&
 		test_tick &&
 		git commit -m commit$i-reverse unrelated-file$i ||

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Results for the git-diff fsmonitor optimization
in patch in the parent-rev (using a 400k file repo to test)

As you can see here - git diff with fsmonitor running is significantly better with this patch series (80% faster on my workload)!

GIT_PERF_LARGE_REPO=~/src/server ./run v2.29.0-rc1 . -- p7519-fsmonitor.sh

Test v2.29.0-rc1 this tree
-----------------------------------------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)                 1.46(0.82+0.64)   1.47(0.83+0.62) +0.7%
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)            0.16(0.12+0.04)   0.17(0.12+0.05) +6.3%
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)           1.36(0.73+0.62)   1.37(0.76+0.60) +0.7%
7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman)                   0.85(0.22+0.63)   0.14(0.10+0.05) -83.5%
7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman)        0.12(0.08+0.05)   0.13(0.11+0.02) +8.3%
7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman)       0.12(0.08+0.04)   0.13(0.09+0.04) +8.3%
7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman)      0.12(0.07+0.05)   0.13(0.07+0.06) +8.3%
7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman)     0.12(0.09+0.04)   0.13(0.08+0.05) +8.3%
7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman)   0.14(0.09+0.05)   0.13(0.10+0.03) -7.1%
7519.12: status (fsmonitor=)                                             1.67(0.93+1.49)   1.67(0.99+1.42) +0.0%
7519.13: status -uno (fsmonitor=)                                        0.37(0.30+0.82)   0.37(0.33+0.79) +0.0%
7519.14: status -uall (fsmonitor=)                                       1.58(0.97+1.35)   1.57(0.86+1.45) -0.6%
7519.15: diff (fsmonitor=)                                               0.34(0.28+0.83)   0.34(0.27+0.83) +0.0%
7519.16: diff -- 0_files (fsmonitor=)                                    0.09(0.06+0.04)   0.09(0.08+0.02) +0.0%
7519.17: diff -- 10_files (fsmonitor=)                                   0.09(0.07+0.03)   0.09(0.06+0.05) +0.0%
7519.18: diff -- 100_files (fsmonitor=)                                  0.09(0.06+0.04)   0.09(0.06+0.04) +0.0%
7519.19: diff -- 1000_files (fsmonitor=)                                 0.09(0.06+0.04)   0.09(0.05+0.05) +0.0%
7519.20: diff -- 10000_files (fsmonitor=)                                0.10(0.08+0.04)   0.10(0.06+0.05) +0.0%

I also added a benchmark for a tiny git diff workload w/ a pathspec. I see an approximately .02 second overhead added w/ and w/o fsmonitor

From looking at these results, I suspected that refresh_fsmonitor is already happening during git diff - independent of this patch series' optimization. Confirmed that suspicion by breaking on refresh_fsmonitor.

(gdb) bt [simplified]

0  refresh_fsmonitor  at fsmonitor.c:176
1  ie_match_stat  at read-cache.c:375
2  match_stat_with_submodule at diff-lib.c:237
4  builtin_diff_files  at builtin/diff.c:260
5  cmd_diff  at builtin/diff.c:541
6  run_builtin  at git.c:450
7  handle_builtin  at git.c:700
8  run_argv  at git.c:767
9  cmd_main  at git.c:898

10 main at common-main.c:52

---
 t/perf/p7519-fsmonitor.sh | 71 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

index 9313d4a51d..ef4c3c8c5c 100755
@@ -115,6 +115,13 @@ test_expect_success "setup for fsmonitor" '
 
 	git config core.fsmonitor "$INTEGRATION_SCRIPT" &&
 	git update-index --fsmonitor &&
+	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
+	for i in $(test_seq 1 10); do touch 10_files/$i; done &&
+	for i in $(test_seq 1 100); do touch 100_files/$i; done &&
+	for i in $(test_seq 1 1000); do touch 1000_files/$i; done &&
+	for i in $(test_seq 1 10000); do touch 10000_files/$i; done &&
+	git add 1_file 10_files 100_files 1000_files 10000_files &&
+	git commit -m "Add files" &&
 	git status  # Warm caches
 '
 

@@ -142,6 +149,38 @@ test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
 	git status -uall
 '
 
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff
+'
+
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1_file
+'
+
+test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10_files
+'
+
+test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 100_files
+'
+
+test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1000_files
+'
+
+test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10000_files
+'
+
 test_expect_success "setup without fsmonitor" '
 	unset INTEGRATION_SCRIPT &&
 	git config --unset core.fsmonitor &&

@@ -172,6 +211,38 @@ test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
 	git status -uall
 '
 
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff
+'
+
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1_file
+'
+
+test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10_files
+'
+
+test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 100_files
+'
+
+test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1000_files
+'
+
+test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10000_files
+'
+
 if test_have_prereq WATCHMAN
 then
 	watchman watch-del "$GIT_WORK_TREE" >/dev/null 2>&1 &&

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Test v2.29.0-rc1 this tree
-----------------------------------------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)                 1.48(0.79+0.67)   1.48(0.79+0.67) +0.0%
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)            0.16(0.11+0.05)   0.17(0.13+0.04) +6.3%
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)           1.36(0.77+0.58)   1.37(0.72+0.63) +0.7%
7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman)                   0.84(0.21+0.63)   0.14(0.11+0.03) -83.3%
7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman)        0.12(0.07+0.05)   0.13(0.09+0.04) +8.3%
7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman)       0.12(0.09+0.04)   0.13(0.07+0.06) +8.3%
7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman)      0.12(0.08+0.05)   0.12(0.08+0.05) +0.0%
7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman)     0.12(0.08+0.05)   0.13(0.09+0.04) +8.3%
7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman)   0.14(0.08+0.06)   0.13(0.07+0.06) -7.1%
7519.11: add (fsmonitor=.git/hooks/fsmonitor-watchman)                   2.75(1.41+1.27)   2.03(1.26+0.70) -26.2%
7519.13: status (fsmonitor=)                                             1.38(1.03+1.04)   1.37(1.04+1.04) -0.7%
7519.14: status -uno (fsmonitor=)                                        1.11(0.83+0.98)   1.10(0.89+0.90) -0.9%
7519.15: status -uall (fsmonitor=)                                       2.30(1.57+1.42)   2.31(1.49+1.50) +0.4%
7519.16: diff (fsmonitor=)                                               1.43(1.13+1.76)   1.46(1.19+1.72) +2.1%
7519.17: diff -- 0_files (fsmonitor=)                                    0.10(0.08+0.04)   0.11(0.08+0.04) +10.0%
7519.18: diff -- 10_files (fsmonitor=)                                   0.10(0.07+0.05)   0.11(0.08+0.04) +10.0%
7519.19: diff -- 100_files (fsmonitor=)                                  0.10(0.07+0.04)   0.11(0.07+0.05) +10.0%
7519.20: diff -- 1000_files (fsmonitor=)                                 0.10(0.08+0.03)   0.11(0.08+0.04) +10.0%
7519.21: diff -- 10000_files (fsmonitor=)                                0.11(0.08+0.05)   0.12(0.07+0.06) +9.1%
7519.22: add (fsmonitor=)                                                2.26(1.46+1.49)   2.27(1.42+1.55) +0.4%

---
 t/perf/p7519-fsmonitor.sh | 4 ++++
 1 file changed, 4 insertions(+)

index 75a0cef01d..fb20fe0937 100755
@@ -169,6 +169,10 @@ test_fsmonitor_suite() {
 	test_perf_w_drop_caches "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
 		git diff -- 10000_files
 	'
+
+	test_perf_w_drop_caches "add (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git add  --all
+	'
 }
 
 test_fsmonitor_suite

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Much of the benchmark code is redundant. This is
easier to understand and edit.

---
 t/perf/p7519-fsmonitor.sh | 136 +++++++++++---------------------------
 1 file changed, 37 insertions(+), 99 deletions(-)

index ef4c3c8c5c..75a0cef01d 100755
@@ -125,61 +125,53 @@ test_expect_success "setup for fsmonitor" '
 	git status  # Warm caches
 '
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
+test_perf_w_drop_caches () {
+	if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+		test-tool drop-caches
+	fi
 
-test_perf "status (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status
-'
+	test_perf "$@"
+}
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
+test_fsmonitor_suite() {
+	test_perf_w_drop_caches "status (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git status
+	'
 
-test_perf "status -uno (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status -uno
-'
+	test_perf_w_drop_caches "status -uno (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git status -uno
+	'
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
+	test_perf_w_drop_caches "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git status -uall
+	'
 
-test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status -uall
-'
-
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff
-'
+	test_perf_w_drop_caches "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff
+	'
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
+	test_perf_w_drop_caches "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff -- 1_file
+	'
 
-test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 1_file
-'
+	test_perf_w_drop_caches "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff -- 10_files
+	'
 
-test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 10_files
-'
+	test_perf_w_drop_caches "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff -- 100_files
+	'
 
-test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 100_files
-'
+	test_perf_w_drop_caches "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff -- 1000_files
+	'
 
-test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 1000_files
-'
+	test_perf_w_drop_caches "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff -- 10000_files
+	'
+}
 
-test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 10000_files
-'
+test_fsmonitor_suite
 
 test_expect_success "setup without fsmonitor" '
 	unset INTEGRATION_SCRIPT &&

@@ -187,61 +179,7 @@ test_expect_success "setup without fsmonitor" '
 	git update-index --no-fsmonitor
 '
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "status (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status
-'
-
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "status -uno (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status -uno
-'
-
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status -uall
-'
-
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff
-'
-
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 1_file
-'
-
-test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 10_files
-'
-
-test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 100_files
-'
-
-test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 1000_files
-'
-
-test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 10000_files
-'
+test_fsmonitor_suite
 
 if test_have_prereq WATCHMAN
 then

gitgitgadget

Actually found a 25% improvement here on git add with this patch series

Good; no huge slow-down here. Thanks for checking!

Thanks,
Taylor

Much easier to read, thank you for taking the time to simplify the code.

I know that this is maybe more review than you were hoping for, but I think it's been worth it and the series is in an even better state than when you started.

Thanks,
Taylor

test_seq is fine, but I do not think tac is portable (only saved by the fact that not many people, especially on exotic platforms, run perf scripts).

Serves me right for reading while I'm tired! I glazed right over 'tac'. If you need a truly unrelated file, you could write random data into it (there are some examples in t/test-lib-functions.sh), but I'd just write 'test_seq 1001'.

Thanks,
Taylor

Looked at doing this and noticed that there are several targets in test-lint in t/Makefile. This would involve duplicating them into t/perf/Makefile which seems like it would be poor form, especially given their complexity. Perhaps t/perf/Makefile could have a target which calls t/Makefile's test-lint target instead. Will play around with it.

Yeah - I see the point, but I agree that since the patch is small, it's ok this way.
If the patch grows significantly, I can make it into two patches

--Nipunn

Seems like there might be some value to adding tac to the perl script check-non-portable-shell.pl - though I'm not sure what we'd use as an alternative. I'll leave this here for now for someone else to handle in a follow up patch series

Credit to alexmv who made this commit back in Dec, 2017 when he was at dbx. I've rebased it and am submitting it now.

With fsmonitor enabled, git diff currently lstats every file in the repo This makes use of the fsmonitor extension to skip lstat() calls on files that fsmonitor judged as unmodified.

I was able to do some testing with/without this change in a large in-house repo (~ 400k files).

-----------------------------------------
(1) With fsmonitor enabled - on master of git (2.29.0)
-----------------------------------------
../git/bin-wrappers/git checkout HEAD~200 strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
99.64 4.358994 10 446257 3 lstat
0.12 0.005353 7 764 360 open

(A subsequent call)
strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
99.84 4.380955 10 444904 3 lstat
0.06 0.002564 135 19 munmap
...

-----------------------------------------
(2) With fsmonitor enabled - with my patch
-----------------------------------------
../git/bin-wrappers/git checkout HEAD~200
strace -c ../git/bin-wrappers/git diff

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------

 50.72    0.003090         163        19           munmap
 19.63    0.001196         598         2           futex

...
0.00 0.000000 0 4 3 lstat

-----------------------------------------
(3) With fsmonitor disabled entirely
-----------------------------------------

% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
98.52 0.277085 92362 3 futex
0.27 0.000752 4 191 63 open
...
0.14 0.000397 3 158 3 lstat

I was able to encode this into a perf test in one of the commits.

Changes since Patch Series V1

* Add git diff -- <pathspec> to perf tests
* improve readability of bitwise ops

Changes since Patch Series V2

* Add git add to perf tests
* Refactor perf fsmonitor to simplify / remove redundancy
* Add linting to perf tests
* Added git diff -- <pathspec> for various sized pathspecs
* Confirmed that refresh_fsmonitor was always being called / added to commit message

Changes since Patch Series V3

* Move perf test linting to Makefile in perf/ directory

Alex Vandiver (1):
fsmonitor: use fsmonitor data in git diff

Nipunn Koorapati (6):
t/perf/README: elaborate on output format t/perf/p7519-fsmonitor.sh: warm cache on first git status t/perf: add fsmonitor perf test for git diff perf lint: add make test-lint to perf tests
p7519-fsmonitor: refactor to avoid code duplication
p7519-fsmonitor: add a git add benchmark

 diff-lib.c                | 15 +++++-
 t/Makefile                |  7 +--
 t/perf/Makefile           |  5 +-
 t/perf/README             |  2 +
 t/perf/p3400-rebase.sh    |  6 +--
 t/perf/p7519-fsmonitor.sh | 96 ++++++++++++++++++++++-----------------
 6 files changed, 81 insertions(+), 50 deletions(-)

base-commit: d4a392452e292ff924e79ec8458611c0f679d6d4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-756%2Fnipunn1313%2Fdiff_fsmon-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-756/nipunn1313/diff_fsmon-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/756

Range-diff vs v3:

 1:  cba03dd40b = 1:  cba03dd40b fsmonitor: use fsmonitor data in `git diff`
 2:  1c7876166f = 2:  1c7876166f t/perf/README: elaborate on output format
 3:  401f696c81 = 3:  401f696c81 t/perf/p7519-fsmonitor.sh: warm cache on first git status
 4:  b3ad8faac4 = 4:  b3ad8faac4 t/perf: add fsmonitor perf test for git diff
 5:  28c1e488bf ! 5:  b534cd137a perf lint: check test-lint-shell-syntax in perf tests

@@ Metadata
Author: Nipunn Koorapati <[email protected]>

## Commit message ##
- perf lint: check test-lint-shell-syntax in perf tests
+ perf lint: add make test-lint to perf tests

- Perf tests have some seq instead of test_seq. This
- runs the existing tests on the perf tests as well.
+ Perf tests have not been linted for some time.
+ They've grown some seq instead of test_seq. This
+ runs the existing lints on the perf tests as well.

@@ t/Makefile: CHAINLINTTMP_SQ = $(subst ','\'',$(CHAINLINTTMP))
CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test))) CHAINLINT = sed -f chainlint.sed

-@@ t/Makefile: test-lint-executable:
+@@ t/Makefile: test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
+ test-lint-filenames
+
+ test-lint-duplicates:
+- @dups=echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d && \
++ @dups=echo $(T) $(TPERF) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d && \
+ test -z "$$dups" || { \
+ echo >&2 "duplicate test numbers:" $$dups; exit 1; }
+
+ test-lint-executable:
+- @bad=for i in $(T); do test -x "$$i" || echo $$i; done && \
++ @bad=for i in $(T) $(TPERF); do test -x "$$i" || echo $$i; done && \
+ test -z "$$bad" || { \
echo >&2 "non-executable tests:" $$bad; exit 1; }

test-lint-shell-syntax:
@@ t/Makefile: test-lint-executable:
test-lint-filenames:
@# We do *not* pass a glob to ls-files but use grep instead, to catch

+ ## t/perf/Makefile ##
+@@
+ -include ../../config.mak
+ export GIT_TEST_OPTIONS
+
+-all: perf
++all: test-lint perf
+
+ perf: pre-clean
+ ./run
+@@ t/perf/Makefile: pre-clean:
+ clean:
+ rm -rf build "trash directory".* test-results
+
++test-lint:
++ $(MAKE) -C .. test-lint
++
+ .PHONY: all perf pre-clean clean
+
## t/perf/p3400-rebase.sh ##
@@ t/perf/p3400-rebase.sh: test_expect_success 'setup rebasing on top of a lot of changes' '
git checkout -f -B base &&

 6:  b38f2984f9 = 6:  3b20f4c76e p7519-fsmonitor: refactor to avoid code duplication
 7:  d392a523f2 = 7:  6f97439936 p7519-fsmonitor: add a git add benchmark

gitgitgadget

From: Alex Vandiver <[email protected]>

With fsmonitor enabled, the first call to match_stat_with_submodule calls refresh_fsmonitor, incurring the overhead of reading the list of updated files -- but run_diff_files does not respect the CE_FSMONITOR_VALID flag.

Make use of the fsmonitor extension to skip lstat() calls on files that fsmonitor judged as unmodified.

Notably, this change improves performance of the git shell prompt when GIT_PS1_SHOWDIRTYSTATE is set.

---
 diff-lib.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

index f95c6de75f..d2d31b9f82 100644
@@ -97,6 +97,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
 
+	refresh_fsmonitor(istate);
+
 	if (diff_unmerged_stage < 0)
 		diff_unmerged_stage = 2;
 	entries = istate->cache_nr;

@@ -197,8 +199,17 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		if (ce_uptodate(ce) || ce_skip_worktree(ce))
 			continue;
 
-		/* If CE_VALID is set, don't look at workdir for file removal */
-		if (ce->ce_flags & CE_VALID) {
+		/*
+		 * When CE_VALID is set (via "update-index --assume-unchanged"
+		 * or via adding paths while core.ignorestat is set to true),
+		 * the user has promised that the working tree file for that
+		 * path will not be modified.  When CE_FSMONITOR_VALID is true,
+		 * the fsmonitor knows that the path hasn't been modified since
+		 * we refreshed the cached stat information.  In either case,
+		 * we do not have to stat to see if the path has been removed
+		 * or modified.
+		 */
+		if (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {
 			changed = 0;
 			newmode = ce->ce_mode;
 		} else {

gitgitgadget

From: Nipunn Koorapati <[email protected]>

---
 t/perf/README | 2 ++
 1 file changed, 2 insertions(+)

index bd649afa97..fb9127a66f 100644
@@ -28,6 +28,8 @@ the tests on the current git repository.
     7810.3: grep --cached, cheap regex       3.07(3.02+0.25)
     7810.4: grep --cached, expensive regex   9.39(30.57+0.24)
 
+Output format is in seconds "Elapsed(User + System)"
+
 You can compare multiple repositories and even git revisions with the
 'run' script:
 

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Much of the benchmark code is redundant. This is
easier to understand and edit.

---
 t/perf/p7519-fsmonitor.sh | 136 +++++++++++---------------------------
 1 file changed, 37 insertions(+), 99 deletions(-)

index ef4c3c8c5c..75a0cef01d 100755
@@ -125,61 +125,53 @@ test_expect_success "setup for fsmonitor" '
 	git status  # Warm caches
 '
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
+test_perf_w_drop_caches () {
+	if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+		test-tool drop-caches
+	fi
 
-test_perf "status (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status
-'
+	test_perf "$@"
+}
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
+test_fsmonitor_suite() {
+	test_perf_w_drop_caches "status (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git status
+	'
 
-test_perf "status -uno (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status -uno
-'
+	test_perf_w_drop_caches "status -uno (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git status -uno
+	'
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
+	test_perf_w_drop_caches "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git status -uall
+	'
 
-test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status -uall
-'
-
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff
-'
+	test_perf_w_drop_caches "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff
+	'
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
+	test_perf_w_drop_caches "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff -- 1_file
+	'
 
-test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 1_file
-'
+	test_perf_w_drop_caches "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff -- 10_files
+	'
 
-test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 10_files
-'
+	test_perf_w_drop_caches "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff -- 100_files
+	'
 
-test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 100_files
-'
+	test_perf_w_drop_caches "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff -- 1000_files
+	'
 
-test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 1000_files
-'
+	test_perf_w_drop_caches "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git diff -- 10000_files
+	'
+}
 
-test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 10000_files
-'
+test_fsmonitor_suite
 
 test_expect_success "setup without fsmonitor" '
 	unset INTEGRATION_SCRIPT &&

@@ -187,61 +179,7 @@ test_expect_success "setup without fsmonitor" '
 	git update-index --no-fsmonitor
 '
 
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "status (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status
-'
-
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "status -uno (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status -uno
-'
-
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git status -uall
-'
-
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff
-'
-
-if test -n "$GIT_PERF_7519_DROP_CACHE"; then
-	test-tool drop-caches
-fi
-
-test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 1_file
-'
-
-test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 10_files
-'
-
-test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 100_files
-'
-
-test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 1000_files
-'
-
-test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
-	git diff -- 10000_files
-'
+test_fsmonitor_suite
 
 if test_have_prereq WATCHMAN
 then

gitgitgadget

From: Nipunn Koorapati <[email protected]>

The first git status would be inflated due to warming of filesystem cache. This makes the results comparable.

Before
Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         2.52(1.59+1.56)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.18(0.12+0.06)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.36(0.73+0.62)
7519.7: status (fsmonitor=)                                      0.69(0.52+0.90)
7519.8: status -uno (fsmonitor=)                                 0.37(0.28+0.81)
7519.9: status -uall (fsmonitor=)                                1.53(0.93+1.32)

After
Test this tree
--------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)         0.39(0.33+0.06)
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)    0.17(0.13+0.05)
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)   1.34(0.77+0.56)
7519.7: status (fsmonitor=)                                      0.70(0.53+0.90)
7519.8: status -uno (fsmonitor=)                                 0.37(0.32+0.78)
7519.9: status -uall (fsmonitor=)                                1.55(1.01+1.25)

---
 t/perf/p7519-fsmonitor.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index def7ecdbc7..9313d4a51d 100755
@@ -114,7 +114,8 @@ test_expect_success "setup for fsmonitor" '
 	fi &&
 
 	git config core.fsmonitor "$INTEGRATION_SCRIPT" &&
-	git update-index --fsmonitor
+	git update-index --fsmonitor &&
+	git status  # Warm caches
 '
 
 if test -n "$GIT_PERF_7519_DROP_CACHE"; then

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Perf tests have not been linted for some time. They've grown some seq instead of test_seq. This runs the existing lints on the perf tests as well.

---
 t/Makefile             | 7 ++++---
 t/perf/Makefile        | 5 ++++-
 t/perf/p3400-rebase.sh | 6 +++---
 3 files changed, 11 insertions(+), 7 deletions(-)

index c83fd18861..882d26eee3 100644
@@ -34,6 +34,7 @@ CHAINLINTTMP_SQ = $(subst ','\'',$(CHAINLINTTMP))
 T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
 TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
 THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))
+TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
 CHAINLINT = sed -f chainlint.sed
 

@@ -81,17 +82,17 @@ test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
 	test-lint-filenames
 
 test-lint-duplicates:
-	@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
+	@dups=`echo $(T) $(TPERF) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
 		test -z "$$dups" || { \
 		echo >&2 "duplicate test numbers:" $$dups; exit 1; }
 
 test-lint-executable:
-	@bad=`for i in $(T); do test -x "$$i" || echo $$i; done` && \
+	@bad=`for i in $(T) $(TPERF); do test -x "$$i" || echo $$i; done` && \
 		test -z "$$bad" || { \
 		echo >&2 "non-executable tests:" $$bad; exit 1; }
 
 test-lint-shell-syntax:
-	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
+	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)
 
 test-lint-filenames:
 	@# We do *not* pass a glob to ls-files but use grep instead, to catch

index 8c47155a7c..fcb0e8865e 100644
@@ -1,7 +1,7 @@
 -include ../../config.mak
 export GIT_TEST_OPTIONS
 
-all: perf
+all: test-lint perf
 
 perf: pre-clean
 	./run

@@ -12,4 +12,7 @@ pre-clean:
 clean:
 	rm -rf build "trash directory".* test-results
 
+test-lint:
+	$(MAKE) -C .. test-lint
+
 .PHONY: all perf pre-clean clean

index d202aaed06..7a0bb29448 100755
@@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '
 	git checkout -f -B base &&
 	git checkout -B to-rebase &&
 	git checkout -B upstream &&
-	for i in $(seq 100)
+	for i in $(test_seq 100)
 	do
 		# simulate huge diffs
 		echo change$i >unrelated-file$i &&
-		seq 1000 >>unrelated-file$i &&
+		test_seq 1000 >>unrelated-file$i &&
 		git add unrelated-file$i &&
 		test_tick &&
 		git commit -m commit$i unrelated-file$i &&
 		echo change$i >unrelated-file$i &&
-		seq 1000 | tac >>unrelated-file$i &&
+		test_seq 1000 | tac >>unrelated-file$i &&
 		git add unrelated-file$i &&
 		test_tick &&
 		git commit -m commit$i-reverse unrelated-file$i ||

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Results for the git-diff fsmonitor optimization
in patch in the parent-rev (using a 400k file repo to test)

As you can see here - git diff with fsmonitor running is significantly better with this patch series (80% faster on my workload)!

GIT_PERF_LARGE_REPO=~/src/server ./run v2.29.0-rc1 . -- p7519-fsmonitor.sh

Test v2.29.0-rc1 this tree
-----------------------------------------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)                 1.46(0.82+0.64)   1.47(0.83+0.62) +0.7%
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)            0.16(0.12+0.04)   0.17(0.12+0.05) +6.3%
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)           1.36(0.73+0.62)   1.37(0.76+0.60) +0.7%
7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman)                   0.85(0.22+0.63)   0.14(0.10+0.05) -83.5%
7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman)        0.12(0.08+0.05)   0.13(0.11+0.02) +8.3%
7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman)       0.12(0.08+0.04)   0.13(0.09+0.04) +8.3%
7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman)      0.12(0.07+0.05)   0.13(0.07+0.06) +8.3%
7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman)     0.12(0.09+0.04)   0.13(0.08+0.05) +8.3%
7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman)   0.14(0.09+0.05)   0.13(0.10+0.03) -7.1%
7519.12: status (fsmonitor=)                                             1.67(0.93+1.49)   1.67(0.99+1.42) +0.0%
7519.13: status -uno (fsmonitor=)                                        0.37(0.30+0.82)   0.37(0.33+0.79) +0.0%
7519.14: status -uall (fsmonitor=)                                       1.58(0.97+1.35)   1.57(0.86+1.45) -0.6%
7519.15: diff (fsmonitor=)                                               0.34(0.28+0.83)   0.34(0.27+0.83) +0.0%
7519.16: diff -- 0_files (fsmonitor=)                                    0.09(0.06+0.04)   0.09(0.08+0.02) +0.0%
7519.17: diff -- 10_files (fsmonitor=)                                   0.09(0.07+0.03)   0.09(0.06+0.05) +0.0%
7519.18: diff -- 100_files (fsmonitor=)                                  0.09(0.06+0.04)   0.09(0.06+0.04) +0.0%
7519.19: diff -- 1000_files (fsmonitor=)                                 0.09(0.06+0.04)   0.09(0.05+0.05) +0.0%
7519.20: diff -- 10000_files (fsmonitor=)                                0.10(0.08+0.04)   0.10(0.06+0.05) +0.0%

I also added a benchmark for a tiny git diff workload w/ a pathspec. I see an approximately .02 second overhead added w/ and w/o fsmonitor

From looking at these results, I suspected that refresh_fsmonitor is already happening during git diff - independent of this patch series' optimization. Confirmed that suspicion by breaking on refresh_fsmonitor.

(gdb) bt [simplified]

0  refresh_fsmonitor  at fsmonitor.c:176
1  ie_match_stat  at read-cache.c:375
2  match_stat_with_submodule at diff-lib.c:237
4  builtin_diff_files  at builtin/diff.c:260
5  cmd_diff  at builtin/diff.c:541
6  run_builtin  at git.c:450
7  handle_builtin  at git.c:700
8  run_argv  at git.c:767
9  cmd_main  at git.c:898

10 main at common-main.c:52

---
 t/perf/p7519-fsmonitor.sh | 71 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

index 9313d4a51d..ef4c3c8c5c 100755
@@ -115,6 +115,13 @@ test_expect_success "setup for fsmonitor" '
 
 	git config core.fsmonitor "$INTEGRATION_SCRIPT" &&
 	git update-index --fsmonitor &&
+	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
+	for i in $(test_seq 1 10); do touch 10_files/$i; done &&
+	for i in $(test_seq 1 100); do touch 100_files/$i; done &&
+	for i in $(test_seq 1 1000); do touch 1000_files/$i; done &&
+	for i in $(test_seq 1 10000); do touch 10000_files/$i; done &&
+	git add 1_file 10_files 100_files 1000_files 10000_files &&
+	git commit -m "Add files" &&
 	git status  # Warm caches
 '
 

@@ -142,6 +149,38 @@ test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
 	git status -uall
 '
 
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff
+'
+
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1_file
+'
+
+test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10_files
+'
+
+test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 100_files
+'
+
+test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1000_files
+'
+
+test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10000_files
+'
+
 test_expect_success "setup without fsmonitor" '
 	unset INTEGRATION_SCRIPT &&
 	git config --unset core.fsmonitor &&

@@ -172,6 +211,38 @@ test_perf "status -uall (fsmonitor=$INTEGRATION_SCRIPT)" '
 	git status -uall
 '
 
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff
+'
+
+if test -n "$GIT_PERF_7519_DROP_CACHE"; then
+	test-tool drop-caches
+fi
+
+test_perf "diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1_file
+'
+
+test_perf "diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10_files
+'
+
+test_perf "diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 100_files
+'
+
+test_perf "diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 1000_files
+'
+
+test_perf "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
+	git diff -- 10000_files
+'
+
 if test_have_prereq WATCHMAN
 then
 	watchman watch-del "$GIT_WORK_TREE" >/dev/null 2>&1 &&

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Test v2.29.0-rc1 this tree
-----------------------------------------------------------------------------------------------------------------

7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman)                 1.48(0.79+0.67)   1.48(0.79+0.67) +0.0%
7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman)            0.16(0.11+0.05)   0.17(0.13+0.04) +6.3%
7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman)           1.36(0.77+0.58)   1.37(0.72+0.63) +0.7%
7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman)                   0.84(0.21+0.63)   0.14(0.11+0.03) -83.3%
7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman)        0.12(0.07+0.05)   0.13(0.09+0.04) +8.3%
7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman)       0.12(0.09+0.04)   0.13(0.07+0.06) +8.3%
7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman)      0.12(0.08+0.05)   0.12(0.08+0.05) +0.0%
7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman)     0.12(0.08+0.05)   0.13(0.09+0.04) +8.3%
7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman)   0.14(0.08+0.06)   0.13(0.07+0.06) -7.1%
7519.11: add (fsmonitor=.git/hooks/fsmonitor-watchman)                   2.75(1.41+1.27)   2.03(1.26+0.70) -26.2%
7519.13: status (fsmonitor=)                                             1.38(1.03+1.04)   1.37(1.04+1.04) -0.7%
7519.14: status -uno (fsmonitor=)                                        1.11(0.83+0.98)   1.10(0.89+0.90) -0.9%
7519.15: status -uall (fsmonitor=)                                       2.30(1.57+1.42)   2.31(1.49+1.50) +0.4%
7519.16: diff (fsmonitor=)                                               1.43(1.13+1.76)   1.46(1.19+1.72) +2.1%
7519.17: diff -- 0_files (fsmonitor=)                                    0.10(0.08+0.04)   0.11(0.08+0.04) +10.0%
7519.18: diff -- 10_files (fsmonitor=)                                   0.10(0.07+0.05)   0.11(0.08+0.04) +10.0%
7519.19: diff -- 100_files (fsmonitor=)                                  0.10(0.07+0.04)   0.11(0.07+0.05) +10.0%
7519.20: diff -- 1000_files (fsmonitor=)                                 0.10(0.08+0.03)   0.11(0.08+0.04) +10.0%
7519.21: diff -- 10000_files (fsmonitor=)                                0.11(0.08+0.05)   0.12(0.07+0.06) +9.1%
7519.22: add (fsmonitor=)                                                2.26(1.46+1.49)   2.27(1.42+1.55) +0.4%

---
 t/perf/p7519-fsmonitor.sh | 4 ++++
 1 file changed, 4 insertions(+)

index 75a0cef01d..fb20fe0937 100755
@@ -169,6 +169,10 @@ test_fsmonitor_suite() {
 	test_perf_w_drop_caches "diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)" '
 		git diff -- 10000_files
 	'
+
+	test_perf_w_drop_caches "add (fsmonitor=$INTEGRATION_SCRIPT)" '
+		git add  --all
+	'
 }
 
 test_fsmonitor_suite

gitgitgadget

Hey. I think there's some confusion. I didn't add tac. It was already here. I didn't even notice it until Junio mentioned it.

--Nipunn

You're right, sorry; I just saw a line beginning with '+' that contained 'tac' and thought that it was new in this patch. What you have is OK, then, since it's not a new problem with your patch.

It couldn't hurt to have the linting phase catch that, but let's leave that for another day, since I think what you have in this version looks good to me.

Thanks for listening to all of my feedback :).

Thanks,
Taylor