{"thread":{"id":"54452","subject":"[PATCH 0/4] use fsmonitor data in git diff eliminating O(num_files) calls to lstat","startedAt":"2020-10-17T21:04:42Z","lastAt":"2020-10-20T22:19:41Z","messageCount":52,"participants":["Nipunn Koorapati via GitGitGadget","Alex Vandiver via GitGitGadget","Junio C Hamano","Nipunn Koorapati","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"407809","messageId":"pull.756.git.1602968677.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":null,"subject":"[PATCH 0/4] use fsmonitor data in git diff eliminating O(num_files) calls to lstat","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-17T21:04:32Z","receivedAt":"2020-10-17T21:04:42Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Credit to alexmv who made this commit back in Dec, 2017 when he was at dbx.\nI've rebased it and am submitting it now.\n\nWith fsmonitor enabled, git diff currently lstats every file in the repo\nThis makes use of the fsmonitor extension to skip lstat() calls on files\nthat fsmonitor judged as unmodified.\n\nI was able to do some testing with/without this change in a large in-house\nrepo (~ 400k files)\n\n-----------------------------------------\n(1) With fsmonitor enabled - on master of git (2.29.0)\n-----------------------------------------\n../git/bin-wrappers/git checkout HEAD~200\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 99.64 4.358994 10 446257 3 lstat\n 0.12 0.005353 7 764 360 open\n\n(A subsequent call)\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 99.84 4.380955 10 444904 3 lstat\n 0.06 0.002564 135 19 munmap\n...\n\n-----------------------------------------\n(2) With fsmonitor enabled - with my patch\n-----------------------------------------\n../git/bin-wrappers/git checkout HEAD~200\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 50.72 0.003090 163 19 munmap\n 19.63 0.001196 598 2 futex\n...\n 0.00 0.000000 0 4 3 lstat\n\n\n-----------------------------------------\n(3) With fsmonitor disabled entirely\n-----------------------------------------\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 98.52 0.277085 92362 3 futex\n 0.27 0.000752 4 191 63 open\n...\n 0.14 0.000397 3 158 3 lstat\n\nI encoded this into a perf test with results as follow:\n\nOn master (2.29)\n\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 2.52(1.59+1.56)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.18(0.12+0.06)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62)\n7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.85(0.30+0.54)\n7519.7: status (fsmonitor=) 0.69(0.52+0.90)\n7519.8: status -uno (fsmonitor=) 0.37(0.28+0.81)\n7519.9: status -uall (fsmonitor=) 1.53(0.93+1.32)\n7519.10: diff (fsmonitor=) 0.34(0.26+0.81)\n\nWith this patch\n\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 2.84(1.70+1.76)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.18(0.13+0.05)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.35(0.81+0.53)\n7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.15(0.11+0.05)\n7519.7: status (fsmonitor=) 0.71(0.54+0.90)\n7519.8: status -uno (fsmonitor=) 0.38(0.30+0.81)\n7519.9: status -uall (fsmonitor=) 1.55(0.93+1.34)\n7519.10: diff (fsmonitor=) 0.35(0.32+0.76)\n\nAlex Vandiver (1):\n fsmonitor: use fsmonitor data in `git diff`\n\nNipunn Koorapati (3):\n t/perf/README: elaborate on output format\n t/perf/p7519-fsmonitor.sh: warm cache on first git status\n t/perf: add fsmonitor perf test for git diff\n\n diff-lib.c | 17 +++++++++++++++--\n t/perf/README | 2 ++\n t/perf/p7519-fsmonitor.sh | 19 ++++++++++++++++++-\n 3 files changed, 35 insertions(+), 3 deletions(-)\n\n\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-756%2Fnipunn1313%2Fdiff_fsmon-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-756/nipunn1313/diff_fsmon-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/756\n-- \ngitgitgadget\n"},{"id":"407810","messageId":"13fd992a375e30e8c7b0953a128e149951dee0ea.1602968677.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.git.1602968677.gitgitgadget@gmail.com","subject":"[PATCH 1/4] fsmonitor: use fsmonitor data in `git diff`","fromName":"Alex Vandiver via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-17T21:04:33Z","receivedAt":"2020-10-17T21:04:42Z","isPatch":true,"sender":{"key":"alex@chmrr.net","avatar":"https://avatars.githubusercontent.com/u/28347?v=4"},"body":"From: Alex Vandiver \n\nWith fsmonitor enabled, the first call to match_stat_with_submodule\ncalls refresh_fsmonitor, incurring the overhead of reading the list of\nupdated files -- but run_diff_files does not respect the\nCE_FSMONITOR_VALID flag.\n\nMake use of the fsmonitor extension to skip lstat() calls on files\nthat fsmonitor judged as unmodified.\n\nNotably, this change improves performance of the git shell prompt when\nGIT_PS1_SHOWDIRTYSTATE is set.\n\nSigned-off-by: Alex Vandiver \nSigned-off-by: Nipunn Koorapati \n---\n diff-lib.c | 17 +++++++++++++++--\n 1 file changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f95c6de75f..b7ee1b89ef 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -97,6 +97,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n \n+\trefresh_fsmonitor(istate);\n+\n \tif (diff_unmerged_stage < 0)\n \t\tdiff_unmerged_stage = 2;\n \tentries = istate->cache_nr;\n@@ -197,8 +199,19 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (ce_uptodate(ce) || ce_skip_worktree(ce))\n \t\t\tcontinue;\n \n-\t\t/* If CE_VALID is set, don't look at workdir for file removal */\n-\t\tif (ce->ce_flags & CE_VALID) {\n+\t\t/*\n+\t\t * If CE_VALID is set, the user has promised us that the workdir\n+\t\t * hasn't changed compared to index, so don't stat workdir\n+\t\t * for file removal\n+\t\t * eg - via git udpate-index --assume-unchanged\n+\t\t * eg - via core.ignorestat=true\n+\t\t *\n+\t\t * When using FSMONITOR:\n+\t\t * If CE_FSMONITOR_VALID is set, then we know the metadata on disk\n+\t\t * has not changed since the last refresh, and we can skip the\n+\t\t * file-removal checks without doing the stat in check_removed.\n+\t\t */\n+\t\tif (ce->ce_flags & CE_VALID || ce->ce_flags & CE_FSMONITOR_VALID) {\n \t\t\tchanged = 0;\n \t\t\tnewmode = ce->ce_mode;\n \t\t} else {\n-- \ngitgitgadget\n\n"},{"id":"407811","messageId":"024cd079654a0f0ecf6fe29976f8274efff58e49.1602968677.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.git.1602968677.gitgitgadget@gmail.com","subject":"[PATCH 2/4] t/perf/README: elaborate on output format","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-17T21:04:34Z","receivedAt":"2020-10-17T21:04:44Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/README | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/perf/README b/t/perf/README\nindex bd649afa97..fb9127a66f 100644\n--- a/t/perf/README\n+++ b/t/perf/README\n@@ -28,6 +28,8 @@ the tests on the current git repository.\n 7810.3: grep --cached, cheap regex 3.07(3.02+0.25)\n 7810.4: grep --cached, expensive regex 9.39(30.57+0.24)\n \n+Output format is in seconds \"Elapsed(User + System)\"\n+\n You can compare multiple repositories and even git revisions with the\n 'run' script:\n \n-- \ngitgitgadget\n\n"},{"id":"407812","messageId":"0613b07676e8abd0b4f342784b94d11174981537.1602968677.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.git.1602968677.gitgitgadget@gmail.com","subject":"[PATCH 4/4] t/perf: add fsmonitor perf test for git diff","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-17T21:04:36Z","receivedAt":"2020-10-17T21:04:46Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nResults for the git-diff fsmonitor optimization\nin patch in the parent-rev (using a 400k file repo to test)\n\nAs you can see here - git diff with fsmonitor running is\nsignificantly better with this patch series (80% faster on my\nworkload)!\n\nOn master (2.29)\n\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 0.39(0.33+0.06)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.17(0.13+0.05)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.34(0.77+0.56)\n7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.82(0.24+0.58)\n7519.7: status (fsmonitor=) 0.70(0.53+0.90)\n7519.8: status -uno (fsmonitor=) 0.37(0.32+0.78)\n7519.9: status -uall (fsmonitor=) 1.55(1.01+1.25)\n7519.10: diff (fsmonitor=) 0.34(0.35+0.72)\n\nWith this patch series\n\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 0.39(0.33+0.07)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.17(0.12+0.05)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.35(0.73+0.61)\n7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.14(0.10+0.05)\n7519.7: status (fsmonitor=) 0.70(0.56+0.87)\n7519.8: status -uno (fsmonitor=) 0.37(0.31+0.79)\n7519.9: status -uall (fsmonitor=) 1.54(0.97+1.29)\n7519.10: diff (fsmonitor=) 0.34(0.28+0.79)\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex 9313d4a51d..80d0148557 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -142,6 +142,14 @@ test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n \tgit status -uall\n '\n \n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff\n+'\n+\n test_expect_success \"setup without fsmonitor\" '\n \tunset INTEGRATION_SCRIPT &&\n \tgit config --unset core.fsmonitor &&\n@@ -172,6 +180,14 @@ test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n \tgit status -uall\n '\n \n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff\n+'\n+\n if test_have_prereq WATCHMAN\n then\n \twatchman watch-del \"$GIT_WORK_TREE\" >/dev/null 2>&1 &&\n-- \ngitgitgadget\n"},{"id":"407813","messageId":"6482e372bc0dca08efe9686b5b9e06a27a1d7a70.1602968677.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.git.1602968677.gitgitgadget@gmail.com","subject":"[PATCH 3/4] t/perf/p7519-fsmonitor.sh: warm cache on first git status","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-17T21:04:35Z","receivedAt":"2020-10-17T21:04:47Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nThe first git status would be inflated due to warming of\nfilesystem cache. This makes the results comparable.\n\nBefore\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 2.52(1.59+1.56)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.18(0.12+0.06)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62)\n7519.7: status (fsmonitor=) 0.69(0.52+0.90)\n7519.8: status -uno (fsmonitor=) 0.37(0.28+0.81)\n7519.9: status -uall (fsmonitor=) 1.53(0.93+1.32)\n\nAfter\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 0.39(0.33+0.06)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.17(0.13+0.05)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.34(0.77+0.56)\n7519.7: status (fsmonitor=) 0.70(0.53+0.90)\n7519.8: status -uno (fsmonitor=) 0.37(0.32+0.78)\n7519.9: status -uall (fsmonitor=) 1.55(1.01+1.25)\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex def7ecdbc7..9313d4a51d 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -114,7 +114,8 @@ test_expect_success \"setup for fsmonitor\" '\n \tfi &&\n \n \tgit config core.fsmonitor \"$INTEGRATION_SCRIPT\" &&\n-\tgit update-index --fsmonitor\n+\tgit update-index --fsmonitor &&\n+\tgit status # Warm caches\n '\n \n if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-- \ngitgitgadget\n\n"},{"id":"407815","messageId":"xmqqeelw8p8i.fsf@gitster.c.googlers.com","threadId":"54452","inReplyTo":"13fd992a375e30e8c7b0953a128e149951dee0ea.1602968677.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] fsmonitor: use fsmonitor data in `git diff`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-17T22:25:01Z","receivedAt":"2020-10-17T22:25:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Vandiver via GitGitGadget\" writes:\n\n> From: Alex Vandiver \n>\n> With fsmonitor enabled, the first call to match_stat_with_submodule\n> calls refresh_fsmonitor, incurring the overhead of reading the list of\n> updated files -- but run_diff_files does not respect the\n> CE_FSMONITOR_VALID flag.\n\nrun_diff_files() is used not just by \"git diff\" but other things\nlike \"git add\", so if we get an overall speed-up without having to\npay undue cost, that would be a very good news.\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index f95c6de75f..b7ee1b89ef 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -97,6 +97,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> \n> \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n> \n> +\trefresh_fsmonitor(istate);\n> +\n\n\"git diff\" and friends are often run with pathspec, but the API into\nthe fsmonitor, refresh_fsmonitor() call, has no way to say \"I only\nam interested in the status of this directory and everything else\ndoes not matter\". How expensive would this call to accept fsmonitor\ndata for the entire tree be, and would there eventually be a point\nwhere the number of paths we are interested in checking (i.e. the\npaths that would match the pathspec) is so small that we would be\nbetter off not making this call? E.g. if we are checking more than\n20% of the working tree, running refresh_fsmonitor() for the entire\nworking tree is still a win, but if we are only checking less than\nthat, we are better off without fsmonitor, or does a tradeoff like\nthat exist?\n\n> @@ -197,8 +199,19 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\tif (ce_uptodate(ce) || ce_skip_worktree(ce))\n> \t\t\tcontinue;\n> \n> -\t\t/* If CE_VALID is set, don't look at workdir for file removal */\n> -\t\tif (ce->ce_flags & CE_VALID) {\n> +\t\t/*\n> +\t\t * If CE_VALID is set, the user has promised us that the workdir\n> +\t\t * hasn't changed compared to index, so don't stat workdir\n> +\t\t * for file removal\n\nThe above seems to be an attempt to elaborate on the existing\ncomment, but ...\n\n> +\t\t * eg - via git udpate-index --assume-unchanged\n> +\t\t * eg - via core.ignorestat=true\n\n... what are these two lines doing here? It makes no sense to say\n\"Don't stat workdir for file removal by doing 'git update-index' or\nby seetting core.ignorestat\", but the placement of these two lines\nmakes it look as if that is what you are saying. Perhaps\n\n\tWhen CE_VALID is set (via \"update-index --assume-unchanged\"\n\tor via adding paths while core.ignorestat is set to true),\n\tthe user has promised ..., so don't stat workdir for removed\n\tfiles.\n\nwould probably be what you meant bo say.\n\n> +\t\t * When using FSMONITOR:\n> +\t\t * If CE_FSMONITOR_VALID is set, then we know the metadata on disk\n> +\t\t * has not changed since the last refresh, and we can skip the\n> +\t\t * file-removal checks without doing the stat in check_removed.\n\nAn iffy description. You skip all the file-removal check by not\ncalling check_removed() as a whole.\n\nThis is not the fault of this patch, but in any case, the\ndescription places too much stress on \"removal\" when in reality,\nremoval is not all that special in this codepath. The check_removed\ncall also contributes to noticiing modified (not removed) files. If\nwe are updating the comment here, we should correct that too,\nperhaps\n\n\tWhen CE_VALID is set (via \"update-index --assume-unchanged\"\n\tor via adding paths while core.ignorestat is set to true),\n\tthe user has promised that the working tree file for that\n\tpath will not be modified. When CE_FSMONITOR_VALID is true,\n\tthe fsmonitor knows that the path hasn't been modified since\n\twe refreshed the cached stat information. In either case,\n\twe do not have to stat to see if the path has been removed\n\tor modified.\n\nor something like that, perhaps.\n\n> +\t\t */\n> +\t\tif (ce->ce_flags & CE_VALID || ce->ce_flags & CE_FSMONITOR_VALID) {\n\nWould it become easier to read, if written like this instead?\n\n\t\tif (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {\n\nThat reflects what the suggested comment says better.\n\n> \t\t\tchanged = 0;\n> \t\t\tnewmode = ce->ce_mode;\n> \t\t} else {\n\nThanks.\n"},{"id":"407816","messageId":"xmqqa6wk8p3i.fsf@gitster.c.googlers.com","threadId":"54452","inReplyTo":"0613b07676e8abd0b4f342784b94d11174981537.1602968677.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] t/perf: add fsmonitor perf test for git diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-17T22:28:01Z","receivedAt":"2020-10-17T22:28:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nipunn Koorapati via GitGitGadget\" writes:\n\n> +test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n> +\tgit diff\n\nThis is a whole-tree diff. It would be interesting to also see if a\nmeaningful tradeoff exists if a test is run with a tree with say 100\ntop-level subdirectories but with just one of them covered by a\npathspec, with many modified paths sprinkled all over.\n\n> +'\n> +\n> if test_have_prereq WATCHMAN\n> then\n> \twatchman watch-del \"$GIT_WORK_TREE\" >/dev/null 2>&1 &&\n"},{"id":"407829","messageId":"CAN8Z4-W=+D-P_qCYijGMnStY-EGwKFx-+AYzjACDPAXnLRAA8A@mail.gmail.com","threadId":"54452","inReplyTo":"xmqqeelw8p8i.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/4] fsmonitor: use fsmonitor data in `git diff`","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-18T00:54:44Z","receivedAt":"2020-10-18T00:55:14Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> run_diff_files() is used not just by \"git diff\" but other things\n> like \"git add\", so if we get an overall speed-up without having to\n> pay undue cost, that would be a very good news.\n\nAgreed! I may be able to write perf benchmark tests to highlight\nbenefits to git add as well.\n\n> 20% of the working tree, running refresh_fsmonitor() for the entire\n> working tree is still a win, but if we are only checking less than\n> that, we are better off without fsmonitor, or does a tradeoff like\n> that exist?\n\nMy understanding is that refresh_fsmonitor is\nO(delta_since_last_refresh) - so for developers\nwith large repositories - this cost will amortize out over subsequent\ncommands, so I don't\nthink it's worth investigating this tradeoff here.\nAs a user of large repositories, I expect that my major source of\nfsmonitor activity to be user\nintent (eg git pull, or intentionally copying/editing a large number\nof files). After such a command,\nI expect my next git command to be slower - that would be unsurprising.\n\nI think the tradeoff could be made for small diff requests, but I\ndon't think it's worth adding complexity here -\nas that user will just have to pay the cost on their next git command.\n\n> > + * eg - via git udpate-index --assume-unchanged\n> > + * eg - via core.ignorestat=true\n>\n> ... what are these two lines doing here?\n\nIntended to indicate potential ways that CE_VALID might be set. When I\nwas reading the source\nhere, it was pretty difficult to determine how this would be set.\nAgree that I picked unfortunate wording.\nThanks for the suggestions. Will update in the next iteration.\n\n>\n> would probably be what you meant bo say.\n>\n> When CE_VALID is set (via \"update-index --assume-unchanged\"\n> or via adding paths while core.ignorestat is set to true),\n> the user has promised that the working tree file for that\n> path will not be modified. When CE_FSMONITOR_VALID is true,\n> the fsmonitor knows that the path hasn't been modified since\n> we refreshed the cached stat information. In either case,\n> we do not have to stat to see if the path has been removed\n> or modified.\n>\n> or something like that, perhaps.\n\nSounds good. Will clarify. I like your comment better as well.\n\n>\n> > + */\n> > + if (ce->ce_flags & CE_VALID || ce->ce_flags & CE_FSMONITOR_VALID) {\n>\n> Would it become easier to read, if written like this instead?\n>\n> if (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {\n\nI personally find this more confusing because it involves multiple\nbitwise ops, but this\nis potentially due to me having more mental practice thinking about\nboolean operators vs bitwise operators.\nI'm more than happy to align with the common pattern of the repo. I'll\nchange this.\n\n>\n> Thanks.\n\nThank you for the thorough review!\n"},{"id":"407833","messageId":"20201018041642.GB2262492@nand.local","threadId":"54452","inReplyTo":"CAN8Z4-W=+D-P_qCYijGMnStY-EGwKFx-+AYzjACDPAXnLRAA8A@mail.gmail.com","subject":"Re: [PATCH 1/4] fsmonitor: use fsmonitor data in `git diff`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-18T04:17:47Z","receivedAt":"2020-10-18T04:22:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Oct 18, 2020 at 01:54:44AM +0100, Nipunn Koorapati wrote:\n> > 20% of the working tree, running refresh_fsmonitor() for the entire\n> > working tree is still a win, but if we are only checking less than\n> > that, we are better off without fsmonitor, or does a tradeoff like\n> > that exist?\n>\n> My understanding is that refresh_fsmonitor is\n> O(delta_since_last_refresh) - so for developers\n> with large repositories - this cost will amortize out over subsequent\n> commands, so I don't\n> think it's worth investigating this tradeoff here.\n> As a user of large repositories, I expect that my major source of\n> fsmonitor activity to be user\n> intent (eg git pull, or intentionally copying/editing a large number\n> of files). After such a command,\n> I expect my next git command to be slower - that would be unsurprising.\n>\n> I think the tradeoff could be made for small diff requests, but I\n> don't think it's worth adding complexity here -\n> as that user will just have to pay the cost on their next git command.\n\nHmm. I do agree that I'd like to stay out of the business of trying to\nfigure out exactly what that trade-off is (although I'm sure that it\nexists), only because it seems likely to vary to a large extent from\nrepository to repository. (That is, 20% may be a good number for some\nrepository, but a terrible choice for another).\n\nBut, I think that we can invoke watchman better here; the\nfsmonitor-watchman hook has no notion of a \"pathspec\", so every query\njust asks for everything that isn't in '$GIT_DIR'. Is there anything\npreventing us from taking an optional pathspec and building up a more\ntargeted query?\n\nThere is some overhead to invoke the hook and talk to watchman, but\nI'd expect that to be dwarfed by not having to issue O(# files)\nsyscalls.\n\n> >\n> > > + */\n> > > + if (ce->ce_flags & CE_VALID || ce->ce_flags & CE_FSMONITOR_VALID) {\n> >\n> > Would it become easier to read, if written like this instead?\n> >\n> > if (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {\n>\n> I personally find this more confusing because it involves multiple\n> bitwise ops, but this\n> is potentially due to me having more mental practice thinking about\n> boolean operators vs bitwise operators.\n> I'm more than happy to align with the common pattern of the repo. I'll\n> change this.\n\nI don't have an opinion, nor do I think that git.git has an established\npractice of doing one over the other. For what it's worth, my two-cents\nis that Junio's suggestion is easier to read.\n\nThanks,\nTaylor\n"},{"id":"407832","messageId":"20201018042244.GA2263679@nand.local","threadId":"54452","inReplyTo":"6482e372bc0dca08efe9686b5b9e06a27a1d7a70.1602968677.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] t/perf/p7519-fsmonitor.sh: warm cache on first git status","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-18T04:22:44Z","receivedAt":"2020-10-18T04:22:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Oct 17, 2020 at 09:04:35PM +0000, Nipunn Koorapati via GitGitGadget wrote:\n> From: Nipunn Koorapati \n>\n> The first git status would be inflated due to warming of\n> filesystem cache. This makes the results comparable.\n>\n> Before\n> Test this tree\n> --------------------------------------------------------------------------------\n> 7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 2.52(1.59+1.56)\n> 7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.18(0.12+0.06)\n> 7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62)\n> 7519.7: status (fsmonitor=) 0.69(0.52+0.90)\n> 7519.8: status -uno (fsmonitor=) 0.37(0.28+0.81)\n> 7519.9: status -uall (fsmonitor=) 1.53(0.93+1.32)\n>\n> After\n> Test this tree\n> --------------------------------------------------------------------------------\n> 7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 0.39(0.33+0.06)\n> 7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.17(0.13+0.05)\n> 7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.34(0.77+0.56)\n> 7519.7: status (fsmonitor=) 0.70(0.53+0.90)\n> 7519.8: status -uno (fsmonitor=) 0.37(0.32+0.78)\n> 7519.9: status -uall (fsmonitor=) 1.55(1.01+1.25)\n\nNote that you can directly compare results with the perf suite's \"run\"\nscript by passing two revisions in addition to the test that you want to\nrun and have the results aggregated side-by-side.\n\nIn your case, you'd want something like (within the t/perf directory):\n\n $ ./run HEAD . p7519-*.sh\n\nwhere this patch is the uncommitted state (alternatively you could\ncompare the two revisions directly in the case that you have already\ncommitted).\n\n> diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\n> index def7ecdbc7..9313d4a51d 100755\n> --- a/t/perf/p7519-fsmonitor.sh\n> +++ b/t/perf/p7519-fsmonitor.sh\n> @@ -114,7 +114,8 @@ test_expect_success \"setup for fsmonitor\" '\n> \tfi &&\n>\n> \tgit config core.fsmonitor \"$INTEGRATION_SCRIPT\" &&\n> -\tgit update-index --fsmonitor\n> +\tgit update-index --fsmonitor &&\n> +\tgit status # Warm caches\n\nSeems reasonable, and the comment is much appreciated :-).\n\nThanks,\nTaylor\n"},{"id":"407834","messageId":"xmqq1rhw86ur.fsf@gitster.c.googlers.com","threadId":"54452","inReplyTo":"20201018041642.GB2262492@nand.local","subject":"Re: [PATCH 1/4] fsmonitor: use fsmonitor data in `git diff`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-18T05:02:04Z","receivedAt":"2020-10-18T05:03:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Hmm. I do agree that I'd like to stay out of the business of trying to\n> figure out exactly what that trade-off is (although I'm sure that it\n> exists), only because it seems likely to vary to a large extent from\n> repository to repository. (That is, 20% may be a good number for some\n> repository, but a terrible choice for another).\n\nI think both of you misunderstood me. \n\nMy question was a simple yes/no \"does there a trade off exist?\"\nquestion and the sentences with 20% in it were mere example of\npossible trade-off I had in mind that _could_ exist. I wasn't even\nsuggesting to figure out what the optimum cut-off heuristics would\nbe (e.g. solving \"when more than N% paths are subject to diff\nfsmonitor is faster\" for N).\n\nI was hoping that we can show that even having to lstat just a\nsingle path is expensive enough---IOW, \"there is no trade-off worth\nworrying about, because talking to fsmonitor is so cheap compared to\nthe cost of even a single lstst\" would have been a valid and happy\nanswer. With such a number, there is no risk of introducing an\nunwarranted performance regression to use cases that we did not\nanticipate by adding an unconditional call to refresh_fsmonitor().\n\nBut without any rationale, the performance implication of adding an\nunconditional call to refresh_fsmonitor() would become much muddier.\n\n> But, I think that we can invoke watchman better here; the\n> fsmonitor-watchman hook has no notion of a \"pathspec\", so every query\n> just asks for everything that isn't in '$GIT_DIR'. Is there anything\n> preventing us from taking an optional pathspec and building up a more\n> targeted query?\n\nYup, it is what I had in mind when I brought up the pathspec. It\nmay be something worth pursuing longer term, but not within the\nscope of this patch.\n\n> There is some overhead to invoke the hook and talk to watchman, but\n> I'd expect that to be dwarfed by not having to issue O(# files)\n> syscalls.\n\n\"invoke the hook\"---is that a pipe+fork+exec, or something else that\nis far lighter-weight?\n\nn\n"},{"id":"407869","messageId":"20201018234344.GC4204@nand.local","threadId":"54452","inReplyTo":"xmqq1rhw86ur.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/4] fsmonitor: use fsmonitor data in `git diff`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-18T23:43:44Z","receivedAt":"2020-10-18T23:44:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Oct 17, 2020 at 10:02:04PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Hmm. I do agree that I'd like to stay out of the business of trying to\n> > figure out exactly what that trade-off is (although I'm sure that it\n> > exists), only because it seems likely to vary to a large extent from\n> > repository to repository. (That is, 20% may be a good number for some\n> > repository, but a terrible choice for another).\n>\n> I think both of you misunderstood me.\n>\n> My question was a simple yes/no \"does there a trade off exist?\"\n> question and the sentences with 20% in it were mere example of\n> possible trade-off I had in mind that _could_ exist. I wasn't even\n> suggesting to figure out what the optimum cut-off heuristics would\n> be (e.g. solving \"when more than N% paths are subject to diff\n> fsmonitor is faster\" for N).\n>\n> I was hoping that we can show that even having to lstat just a\n> single path is expensive enough---IOW, \"there is no trade-off worth\n> worrying about, because talking to fsmonitor is so cheap compared to\n> the cost of even a single lstst\" would have been a valid and happy\n> answer. With such a number, there is no risk of introducing an\n> unwarranted performance regression to use cases that we did not\n> anticipate by adding an unconditional call to refresh_fsmonitor().\n>\n> But without any rationale, the performance implication of adding an\n> unconditional call to refresh_fsmonitor() would become much muddier.\n\nAha; thanks for clarifying. I'm glad we agree that finding 'N' would not\nbe worth it, or at least that showing that talking to fsmonitor is\ncheaper than a single lstat would be more worthwhile.\n\nNipunn - I don't have fsmonitor/watchman setup on my workstation, but if\nyou do, some numbers (or an interpretation of the numbers you already\nprovided) on this would be really useful. If you don't have it set up,\nor don't have time to measure it, let me know, and I'd be happy to take\na look.\n\n> > But, I think that we can invoke watchman better here; the\n> > fsmonitor-watchman hook has no notion of a \"pathspec\", so every query\n> > just asks for everything that isn't in '$GIT_DIR'. Is there anything\n> > preventing us from taking an optional pathspec and building up a more\n> > targeted query?\n>\n> Yup, it is what I had in mind when I brought up the pathspec. It\n> may be something worth pursuing longer term, but not within the\n> scope of this patch.\n>\n> > There is some overhead to invoke the hook and talk to watchman, but\n> > I'd expect that to be dwarfed by not having to issue O(# files)\n> > syscalls.\n>\n> \"invoke the hook\"---is that a pipe+fork+exec, or something else that\n> is far lighter-weight?\n\nThe former; see 'fsmonitor.c:query_fsmonitor()'.\n\nThanks,\nTaylor\n"},{"id":"407880","messageId":"xmqqr1puuo35.fsf@gitster.c.googlers.com","threadId":"54452","inReplyTo":"20201018234344.GC4204@nand.local","subject":"Re: [PATCH 1/4] fsmonitor: use fsmonitor data in `git diff`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-19T17:23:26Z","receivedAt":"2020-10-19T17:23:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> > There is some overhead to invoke the hook and talk to watchman, but\n>> > I'd expect that to be dwarfed by not having to issue O(# files)\n>> > syscalls.\n>>\n>> \"invoke the hook\"---is that a pipe+fork+exec, or something else that\n>> is far lighter-weight?\n>\n> The former; see 'fsmonitor.c:query_fsmonitor()'.\n\nIt brings us back to the \"overhead of how many lstat(2) takes us\ncloser to the overhead of a single pipe+fork+exec plus reading from\nthe pipe\", doesn't it?\n\n"},{"id":"407884","messageId":"20201019173724.GA42706@nand.local","threadId":"54452","inReplyTo":"xmqqr1puuo35.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/4] fsmonitor: use fsmonitor data in `git diff`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-19T17:37:24Z","receivedAt":"2020-10-19T17:37:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 19, 2020 at 10:23:26AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> >> > There is some overhead to invoke the hook and talk to watchman, but\n> >> > I'd expect that to be dwarfed by not having to issue O(# files)\n> >> > syscalls.\n> >>\n> >> \"invoke the hook\"---is that a pipe+fork+exec, or something else that\n> >> is far lighter-weight?\n> >\n> > The former; see 'fsmonitor.c:query_fsmonitor()'.\n>\n> It brings us back to the \"overhead of how many lstat(2) takes us\n> closer to the overhead of a single pipe+fork+exec plus reading from\n> the pipe\", doesn't it?\n\nSomewhat unfortunately, yes. Hopefully any user that cares to use\nfsmonitor has enough files in their repository that a pipe+fork+exec is\nstill faster than however many lstats they would have needed otherwise.\n\nOf course, finding out what that number is is still interesting...\n\nThanks,\nTaylor\n"},{"id":"407888","messageId":"CAN8Z4-WyrhzeQGzNpQvCYYBSif_1NghFWwJvS1gXppQna6eC0A@mail.gmail.com","threadId":"54452","inReplyTo":"20201019173724.GA42706@nand.local","subject":"Re: [PATCH 1/4] fsmonitor: use fsmonitor data in `git diff`","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-19T18:07:03Z","receivedAt":"2020-10-19T18:07:17Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> It brings us back to the \"overhead of how many lstat(2) takes us\n> closer to the overhead of a single pipe+fork+exec plus reading from\n> the pipe\", doesn't it?\n>\n\nI will add a benchmark for a `git diff -- `\n\n> Somewhat unfortunately, yes. Hopefully any user that cares to use\n> fsmonitor has enough files in their repository that a pipe+fork+exec is\n> still faster than however many lstats they would have needed otherwise.\n>\n> Of course, finding out what that number is is still interesting...\n\nI can try to do some manual testing to figure this out. Doesn't seem like the\ntype of thing we'd want to add to the benchmark, as it would involve running\ngit diff on a variety of pathspec workloads\n\n--Nipunn\n"},{"id":"407916","messageId":"cba03dd40bc6af965eb33eba87ea490588dc6bcc.1603143316.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v2.git.1603143316.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] fsmonitor: use fsmonitor data in `git diff`","fromName":"Alex Vandiver via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T21:35:12Z","receivedAt":"2020-10-19T21:35:22Z","isPatch":true,"sender":{"key":"alex@chmrr.net","avatar":"https://avatars.githubusercontent.com/u/28347?v=4"},"body":"From: Alex Vandiver \n\nWith fsmonitor enabled, the first call to match_stat_with_submodule\ncalls refresh_fsmonitor, incurring the overhead of reading the list of\nupdated files -- but run_diff_files does not respect the\nCE_FSMONITOR_VALID flag.\n\nMake use of the fsmonitor extension to skip lstat() calls on files\nthat fsmonitor judged as unmodified.\n\nNotably, this change improves performance of the git shell prompt when\nGIT_PS1_SHOWDIRTYSTATE is set.\n\nSigned-off-by: Alex Vandiver \nSigned-off-by: Nipunn Koorapati \n---\n diff-lib.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f95c6de75f..d2d31b9f82 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -97,6 +97,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n \n+\trefresh_fsmonitor(istate);\n+\n \tif (diff_unmerged_stage < 0)\n \t\tdiff_unmerged_stage = 2;\n \tentries = istate->cache_nr;\n@@ -197,8 +199,17 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (ce_uptodate(ce) || ce_skip_worktree(ce))\n \t\t\tcontinue;\n \n-\t\t/* If CE_VALID is set, don't look at workdir for file removal */\n-\t\tif (ce->ce_flags & CE_VALID) {\n+\t\t/*\n+\t\t * When CE_VALID is set (via \"update-index --assume-unchanged\"\n+\t\t * or via adding paths while core.ignorestat is set to true),\n+\t\t * the user has promised that the working tree file for that\n+\t\t * path will not be modified. When CE_FSMONITOR_VALID is true,\n+\t\t * the fsmonitor knows that the path hasn't been modified since\n+\t\t * we refreshed the cached stat information. In either case,\n+\t\t * we do not have to stat to see if the path has been removed\n+\t\t * or modified.\n+\t\t */\n+\t\tif (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {\n \t\t\tchanged = 0;\n \t\t\tnewmode = ce->ce_mode;\n \t\t} else {\n-- \ngitgitgadget\n\n"},{"id":"407917","messageId":"pull.756.v2.git.1603143316.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.git.1602968677.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] use fsmonitor data in git diff eliminating O(num_files) calls to lstat","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T21:35:11Z","receivedAt":"2020-10-19T21:35:23Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Credit to alexmv who made this commit back in Dec, 2017 when he was at dbx.\nI've rebased it and am submitting it now.\n\nWith fsmonitor enabled, git diff currently lstats every file in the repo\nThis makes use of the fsmonitor extension to skip lstat() calls on files\nthat fsmonitor judged as unmodified.\n\nI was able to do some testing with/without this change in a large in-house\nrepo (~ 400k files).\n\n-----------------------------------------\n(1) With fsmonitor enabled - on master of git (2.29.0)\n-----------------------------------------\n../git/bin-wrappers/git checkout HEAD~200\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 99.64 4.358994 10 446257 3 lstat\n 0.12 0.005353 7 764 360 open\n\n(A subsequent call)\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 99.84 4.380955 10 444904 3 lstat\n 0.06 0.002564 135 19 munmap\n...\n\n-----------------------------------------\n(2) With fsmonitor enabled - with my patch\n-----------------------------------------\n../git/bin-wrappers/git checkout HEAD~200\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 50.72 0.003090 163 19 munmap\n 19.63 0.001196 598 2 futex\n...\n 0.00 0.000000 0 4 3 lstat\n\n\n-----------------------------------------\n(3) With fsmonitor disabled entirely\n-----------------------------------------\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 98.52 0.277085 92362 3 futex\n 0.27 0.000752 4 191 63 open\n...\n 0.14 0.000397 3 158 3 lstat\n\nI was able to encode this into a perf test in one of the commits.\n\nChanges since Patch Series V1\n\n * Add git diff -- to perf tests\n * improve readability of bitwise ops\n\nAlex Vandiver (1):\n fsmonitor: use fsmonitor data in `git diff`\n\nNipunn Koorapati (3):\n t/perf/README: elaborate on output format\n t/perf/p7519-fsmonitor.sh: warm cache on first git status\n t/perf: add fsmonitor perf test for git diff\n\n diff-lib.c | 15 ++++++--\n t/perf/README | 2 ++\n t/perf/p7519-fsmonitor.sh | 74 ++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 88 insertions(+), 3 deletions(-)\n\n\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-756%2Fnipunn1313%2Fdiff_fsmon-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-756/nipunn1313/diff_fsmon-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/756\n\nRange-diff vs v1:\n\n 1: 13fd992a37 ! 1: cba03dd40b fsmonitor: use fsmonitor data in `git diff`\n @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)\n -\t\t/* If CE_VALID is set, don't look at workdir for file removal */\n -\t\tif (ce->ce_flags & CE_VALID) {\n +\t\t/*\n -+\t\t * If CE_VALID is set, the user has promised us that the workdir\n -+\t\t * hasn't changed compared to index, so don't stat workdir\n -+\t\t * for file removal\n -+\t\t * eg - via git udpate-index --assume-unchanged\n -+\t\t * eg - via core.ignorestat=true\n -+\t\t *\n -+\t\t * When using FSMONITOR:\n -+\t\t * If CE_FSMONITOR_VALID is set, then we know the metadata on disk\n -+\t\t * has not changed since the last refresh, and we can skip the\n -+\t\t * file-removal checks without doing the stat in check_removed.\n ++\t\t * When CE_VALID is set (via \"update-index --assume-unchanged\"\n ++\t\t * or via adding paths while core.ignorestat is set to true),\n ++\t\t * the user has promised that the working tree file for that\n ++\t\t * path will not be modified. When CE_FSMONITOR_VALID is true,\n ++\t\t * the fsmonitor knows that the path hasn't been modified since\n ++\t\t * we refreshed the cached stat information. In either case,\n ++\t\t * we do not have to stat to see if the path has been removed\n ++\t\t * or modified.\n +\t\t */\n -+\t\tif (ce->ce_flags & CE_VALID || ce->ce_flags & CE_FSMONITOR_VALID) {\n ++\t\tif (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {\n \t\t\tchanged = 0;\n \t\t\tnewmode = ce->ce_mode;\n \t\t} else {\n 2: 024cd07965 = 2: 1c7876166f t/perf/README: elaborate on output format\n 3: 6482e372bc = 3: 401f696c81 t/perf/p7519-fsmonitor.sh: warm cache on first git status\n 4: 0613b07676 ! 4: f572e226bb t/perf: add fsmonitor perf test for git diff\n @@ Commit message\n significantly better with this patch series (80% faster on my\n workload)!\n \n - On master (2.29)\n + GIT_PERF_LARGE_REPO=~/src/server ./run v2.29.0-rc1 . -- p7519-fsmonitor.sh\n \n - Test this tree\n - --------------------------------------------------------------------------------\n - 7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 0.39(0.33+0.06)\n - 7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.17(0.13+0.05)\n - 7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.34(0.77+0.56)\n - 7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.82(0.24+0.58)\n - 7519.7: status (fsmonitor=) 0.70(0.53+0.90)\n - 7519.8: status -uno (fsmonitor=) 0.37(0.32+0.78)\n - 7519.9: status -uall (fsmonitor=) 1.55(1.01+1.25)\n - 7519.10: diff (fsmonitor=) 0.34(0.35+0.72)\n + Test v2.29.0-rc1 this tree\n + -----------------------------------------------------------------------------------------------------------------\n + 7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 1.46(0.82+0.64) 1.47(0.83+0.62) +0.7%\n + 7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.16(0.12+0.04) 0.17(0.12+0.05) +6.3%\n + 7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62) 1.37(0.76+0.60) +0.7%\n + 7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.85(0.22+0.63) 0.14(0.10+0.05) -83.5%\n + 7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.05) 0.13(0.11+0.02) +8.3%\n + 7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.04) 0.13(0.09+0.04) +8.3%\n + 7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.07+0.05) 0.13(0.07+0.06) +8.3%\n + 7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.09+0.04) 0.13(0.08+0.05) +8.3%\n + 7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.14(0.09+0.05) 0.13(0.10+0.03) -7.1%\n + 7519.12: status (fsmonitor=) 1.67(0.93+1.49) 1.67(0.99+1.42) +0.0%\n + 7519.13: status -uno (fsmonitor=) 0.37(0.30+0.82) 0.37(0.33+0.79) +0.0%\n + 7519.14: status -uall (fsmonitor=) 1.58(0.97+1.35) 1.57(0.86+1.45) -0.6%\n + 7519.15: diff (fsmonitor=) 0.34(0.28+0.83) 0.34(0.27+0.83) +0.0%\n + 7519.16: diff -- 0_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.08+0.02) +0.0%\n + 7519.17: diff -- 10_files (fsmonitor=) 0.09(0.07+0.03) 0.09(0.06+0.05) +0.0%\n + 7519.18: diff -- 100_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.06+0.04) +0.0%\n + 7519.19: diff -- 1000_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.05+0.05) +0.0%\n + 7519.20: diff -- 10000_files (fsmonitor=) 0.10(0.08+0.04) 0.10(0.06+0.05) +0.0%\n \n - With this patch series\n + I also added a benchmark for a tiny git diff workload w/ a pathspec.\n + I see an approximately .02 second overhead added w/ and w/o fsmonitor\n \n - Test this tree\n - --------------------------------------------------------------------------------\n - 7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 0.39(0.33+0.07)\n - 7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.17(0.12+0.05)\n - 7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.35(0.73+0.61)\n - 7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.14(0.10+0.05)\n - 7519.7: status (fsmonitor=) 0.70(0.56+0.87)\n - 7519.8: status -uno (fsmonitor=) 0.37(0.31+0.79)\n - 7519.9: status -uall (fsmonitor=) 1.54(0.97+1.29)\n - 7519.10: diff (fsmonitor=) 0.34(0.28+0.79)\n + From looking at these results, I suspected that refresh_fsmonitor\n + is already happening during git diff - independent of this patch\n + series' optimization. Confirmed that suspicion by breaking on\n + refresh_fsmonitor.\n +\n + (gdb) bt [simplified]\n + 0 refresh_fsmonitor at fsmonitor.c:176\n + 1 ie_match_stat at read-cache.c:375\n + 2 match_stat_with_submodule at diff-lib.c:237\n + 4 builtin_diff_files at builtin/diff.c:260\n + 5 cmd_diff at builtin/diff.c:541\n + 6 run_builtin at git.c:450\n + 7 handle_builtin at git.c:700\n + 8 run_argv at git.c:767\n + 9 cmd_main at git.c:898\n + 10 main at common-main.c:52\n \n Signed-off-by: Nipunn Koorapati \n \n ## t/perf/p7519-fsmonitor.sh ##\n +@@ t/perf/p7519-fsmonitor.sh: test_expect_success \"setup for fsmonitor\" '\n + \n + \tgit config core.fsmonitor \"$INTEGRATION_SCRIPT\" &&\n + \tgit update-index --fsmonitor &&\n ++\tmkdir 1_file 10_files 100_files 1000_files 10000_files &&\n ++\tfor i in `seq 1 10`; do touch 10_files/$i; done &&\n ++\tfor i in `seq 1 100`; do touch 100_files/$i; done &&\n ++\tfor i in `seq 1 1000`; do touch 1000_files/$i; done &&\n ++\tfor i in `seq 1 10000`; do touch 10000_files/$i; done &&\n ++\tgit add 1_file 10_files 100_files 1000_files 10000_files &&\n ++\tgit commit -m \"Add files\" &&\n + \tgit status # Warm caches\n + '\n + \n @@ t/perf/p7519-fsmonitor.sh: test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n \tgit status -uall\n '\n @@ t/perf/p7519-fsmonitor.sh: test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIP\n +test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n +\tgit diff\n +'\n ++\n ++if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n ++\ttest-tool drop-caches\n ++fi\n ++\n ++test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n ++\tgit diff -- 1_file\n ++'\n ++\n ++test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n ++\tgit diff -- 10_files\n ++'\n ++\n ++test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n ++\tgit diff -- 100_files\n ++'\n ++\n ++test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n ++\tgit diff -- 1000_files\n ++'\n ++\n ++test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n ++\tgit diff -- 10000_files\n ++'\n +\n test_expect_success \"setup without fsmonitor\" '\n \tunset INTEGRATION_SCRIPT &&\n @@ t/perf/p7519-fsmonitor.sh: test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIP\n +test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n +\tgit diff\n +'\n ++\n ++if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n ++\ttest-tool drop-caches\n ++fi\n ++\n ++test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n ++\tgit diff -- 1_file\n ++'\n ++\n ++test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n ++\tgit diff -- 10_files\n ++'\n ++\n ++test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n ++\tgit diff -- 100_files\n ++'\n ++\n ++test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n ++\tgit diff -- 1000_files\n ++'\n ++\n ++test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n ++\tgit diff -- 10000_files\n ++'\n +\n if test_have_prereq WATCHMAN\n then\n\n-- \ngitgitgadget\n"},{"id":"407919","messageId":"1c7876166f5d9262c44c9df0f613e7d0beb98722.1603143316.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v2.git.1603143316.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] t/perf/README: elaborate on output format","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T21:35:13Z","receivedAt":"2020-10-19T21:35:23Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/README | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/perf/README b/t/perf/README\nindex bd649afa97..fb9127a66f 100644\n--- a/t/perf/README\n+++ b/t/perf/README\n@@ -28,6 +28,8 @@ the tests on the current git repository.\n 7810.3: grep --cached, cheap regex 3.07(3.02+0.25)\n 7810.4: grep --cached, expensive regex 9.39(30.57+0.24)\n \n+Output format is in seconds \"Elapsed(User + System)\"\n+\n You can compare multiple repositories and even git revisions with the\n 'run' script:\n \n-- \ngitgitgadget\n\n"},{"id":"407918","messageId":"401f696c8156acafd1bf91511fde7ae099ff9052.1603143316.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v2.git.1603143316.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] t/perf/p7519-fsmonitor.sh: warm cache on first git status","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T21:35:14Z","receivedAt":"2020-10-19T21:35:24Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nThe first git status would be inflated due to warming of\nfilesystem cache. This makes the results comparable.\n\nBefore\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 2.52(1.59+1.56)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.18(0.12+0.06)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62)\n7519.7: status (fsmonitor=) 0.69(0.52+0.90)\n7519.8: status -uno (fsmonitor=) 0.37(0.28+0.81)\n7519.9: status -uall (fsmonitor=) 1.53(0.93+1.32)\n\nAfter\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 0.39(0.33+0.06)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.17(0.13+0.05)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.34(0.77+0.56)\n7519.7: status (fsmonitor=) 0.70(0.53+0.90)\n7519.8: status -uno (fsmonitor=) 0.37(0.32+0.78)\n7519.9: status -uall (fsmonitor=) 1.55(1.01+1.25)\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex def7ecdbc7..9313d4a51d 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -114,7 +114,8 @@ test_expect_success \"setup for fsmonitor\" '\n \tfi &&\n \n \tgit config core.fsmonitor \"$INTEGRATION_SCRIPT\" &&\n-\tgit update-index --fsmonitor\n+\tgit update-index --fsmonitor &&\n+\tgit status # Warm caches\n '\n \n if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-- \ngitgitgadget\n\n"},{"id":"407920","messageId":"f572e226bb5e4b67cc57f8d9d4732086f01190a2.1603143316.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v2.git.1603143316.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] t/perf: add fsmonitor perf test for git diff","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T21:35:15Z","receivedAt":"2020-10-19T21:35:29Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nResults for the git-diff fsmonitor optimization\nin patch in the parent-rev (using a 400k file repo to test)\n\nAs you can see here - git diff with fsmonitor running is\nsignificantly better with this patch series (80% faster on my\nworkload)!\n\nGIT_PERF_LARGE_REPO=~/src/server ./run v2.29.0-rc1 . -- p7519-fsmonitor.sh\n\nTest v2.29.0-rc1 this tree\n-----------------------------------------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 1.46(0.82+0.64) 1.47(0.83+0.62) +0.7%\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.16(0.12+0.04) 0.17(0.12+0.05) +6.3%\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62) 1.37(0.76+0.60) +0.7%\n7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.85(0.22+0.63) 0.14(0.10+0.05) -83.5%\n7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.05) 0.13(0.11+0.02) +8.3%\n7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.04) 0.13(0.09+0.04) +8.3%\n7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.07+0.05) 0.13(0.07+0.06) +8.3%\n7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.09+0.04) 0.13(0.08+0.05) +8.3%\n7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.14(0.09+0.05) 0.13(0.10+0.03) -7.1%\n7519.12: status (fsmonitor=) 1.67(0.93+1.49) 1.67(0.99+1.42) +0.0%\n7519.13: status -uno (fsmonitor=) 0.37(0.30+0.82) 0.37(0.33+0.79) +0.0%\n7519.14: status -uall (fsmonitor=) 1.58(0.97+1.35) 1.57(0.86+1.45) -0.6%\n7519.15: diff (fsmonitor=) 0.34(0.28+0.83) 0.34(0.27+0.83) +0.0%\n7519.16: diff -- 0_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.08+0.02) +0.0%\n7519.17: diff -- 10_files (fsmonitor=) 0.09(0.07+0.03) 0.09(0.06+0.05) +0.0%\n7519.18: diff -- 100_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.06+0.04) +0.0%\n7519.19: diff -- 1000_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.05+0.05) +0.0%\n7519.20: diff -- 10000_files (fsmonitor=) 0.10(0.08+0.04) 0.10(0.06+0.05) +0.0%\n\nI also added a benchmark for a tiny git diff workload w/ a pathspec.\nI see an approximately .02 second overhead added w/ and w/o fsmonitor\n\nFrom looking at these results, I suspected that refresh_fsmonitor\nis already happening during git diff - independent of this patch\nseries' optimization. Confirmed that suspicion by breaking on\nrefresh_fsmonitor.\n\n(gdb) bt [simplified]\n0 refresh_fsmonitor at fsmonitor.c:176\n1 ie_match_stat at read-cache.c:375\n2 match_stat_with_submodule at diff-lib.c:237\n4 builtin_diff_files at builtin/diff.c:260\n5 cmd_diff at builtin/diff.c:541\n6 run_builtin at git.c:450\n7 handle_builtin at git.c:700\n8 run_argv at git.c:767\n9 cmd_main at git.c:898\n10 main at common-main.c:52\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 71 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 71 insertions(+)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex 9313d4a51d..2b4803707f 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -115,6 +115,13 @@ test_expect_success \"setup for fsmonitor\" '\n \n \tgit config core.fsmonitor \"$INTEGRATION_SCRIPT\" &&\n \tgit update-index --fsmonitor &&\n+\tmkdir 1_file 10_files 100_files 1000_files 10000_files &&\n+\tfor i in `seq 1 10`; do touch 10_files/$i; done &&\n+\tfor i in `seq 1 100`; do touch 100_files/$i; done &&\n+\tfor i in `seq 1 1000`; do touch 1000_files/$i; done &&\n+\tfor i in `seq 1 10000`; do touch 10000_files/$i; done &&\n+\tgit add 1_file 10_files 100_files 1000_files 10000_files &&\n+\tgit commit -m \"Add files\" &&\n \tgit status # Warm caches\n '\n \n@@ -142,6 +149,38 @@ test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n \tgit status -uall\n '\n \n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff\n+'\n+\n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1_file\n+'\n+\n+test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10_files\n+'\n+\n+test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 100_files\n+'\n+\n+test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1000_files\n+'\n+\n+test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10000_files\n+'\n+\n test_expect_success \"setup without fsmonitor\" '\n \tunset INTEGRATION_SCRIPT &&\n \tgit config --unset core.fsmonitor &&\n@@ -172,6 +211,38 @@ test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n \tgit status -uall\n '\n \n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff\n+'\n+\n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1_file\n+'\n+\n+test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10_files\n+'\n+\n+test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 100_files\n+'\n+\n+test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1000_files\n+'\n+\n+test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10000_files\n+'\n+\n if test_have_prereq WATCHMAN\n then\n \twatchman watch-del \"$GIT_WORK_TREE\" >/dev/null 2>&1 &&\n-- \ngitgitgadget\n"},{"id":"407921","messageId":"20201019214314.GA47659@nand.local","threadId":"54452","inReplyTo":"f572e226bb5e4b67cc57f8d9d4732086f01190a2.1603143316.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/4] t/perf: add fsmonitor perf test for git diff","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-19T21:43:39Z","receivedAt":"2020-10-19T21:43:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 19, 2020 at 09:35:15PM +0000, Nipunn Koorapati via GitGitGadget wrote:\n> diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\n> index 9313d4a51d..2b4803707f 100755\n> --- a/t/perf/p7519-fsmonitor.sh\n> +++ b/t/perf/p7519-fsmonitor.sh\n> @@ -115,6 +115,13 @@ test_expect_success \"setup for fsmonitor\" '\n>\n> \tgit config core.fsmonitor \"$INTEGRATION_SCRIPT\" &&\n> \tgit update-index --fsmonitor &&\n> +\tmkdir 1_file 10_files 100_files 1000_files 10000_files &&\n> +\tfor i in `seq 1 10`; do touch 10_files/$i; done &&\n> +\tfor i in `seq 1 100`; do touch 100_files/$i; done &&\n> +\tfor i in `seq 1 1000`; do touch 1000_files/$i; done &&\n> +\tfor i in `seq 1 10000`; do touch 10000_files/$i; done &&\n\nI just happened to notice these while reading your range diff; git\ndiscourages the use of seq in test, instead preferring our own\nworks-everywhere 'test_seq()'.\n\nI was wondering how this slipped through since it should be checked\nautomatically by t/check-non-portable-shell.pl, but that is only run\nfrom t/Makefile, not t/perf/Makefile. That probably explains how a few\nraw `seq`'s made it into t/perf.\n\nIn either case, test_seq() is preferred here.\n\nThanks,\nTaylor\n"},{"id":"407922","messageId":"20201019215438.GA49623@nand.local","threadId":"54452","inReplyTo":"f572e226bb5e4b67cc57f8d9d4732086f01190a2.1603143316.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/4] t/perf: add fsmonitor perf test for git diff","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-19T21:54:38Z","receivedAt":"2020-10-19T21:54:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 19, 2020 at 09:35:15PM +0000, Nipunn Koorapati via GitGitGadget wrote:\n> From: Nipunn Koorapati \n>\n> Results for the git-diff fsmonitor optimization\n> in patch in the parent-rev (using a 400k file repo to test)\n>\n> As you can see here - git diff with fsmonitor running is\n> significantly better with this patch series (80% faster on my\n> workload)!\n\nThese t/perf numbers are very helpful, at least to me.\n\n> GIT_PERF_LARGE_REPO=~/src/server ./run v2.29.0-rc1 . -- p7519-fsmonitor.sh\n>\n> Test v2.29.0-rc1 this tree\n> -----------------------------------------------------------------------------------------------------------------\n> 7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 1.46(0.82+0.64) 1.47(0.83+0.62) +0.7%\n> 7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.16(0.12+0.04) 0.17(0.12+0.05) +6.3%\n> 7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62) 1.37(0.76+0.60) +0.7%\n\nLooks like about 0.01sec of overhead, which seems like an acceptable\ntrade-off for when the user has at least 10,000 files.\n\nThis reminds me; did you look at the 'git add' performance change? I\nrecall Junio mentioning that 'git add' takes the same paths in the code.\n\n> 7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.85(0.22+0.63) 0.14(0.10+0.05) -83.5%\n> 7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.05) 0.13(0.11+0.02) +8.3%\n> 7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.04) 0.13(0.09+0.04) +8.3%\n> 7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.07+0.05) 0.13(0.07+0.06) +8.3%\n> 7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.09+0.04) 0.13(0.08+0.05) +8.3%\n> 7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.14(0.09+0.05) 0.13(0.10+0.03) -7.1%\n\nOK... so having fsmonitor turned on adds an imperceptible amount of\nslow-down to cases where there are [0, 10000) files. But, in exchange,\nyou get much-improved whole-tree performance, as well as single-tree\nperformance when that tree contains at least 10,000 files.\n\nI was going to say that this has little downside, because turning on\nfsmonitor is probably a good indicator that you don't have any fewer\nthan 10,000 files in your repository, but I think that's missing the\npoint. Likely true, but that doesn't exclude the possibility of having\nsub-10,000 file directories, which users may very well still be\ndiff-ing.\n\nSo, there's a slow-down, but it's hard to complain when you consider\nwhat we get in exchange.\n\n> 7519.12: status (fsmonitor=) 1.67(0.93+1.49) 1.67(0.99+1.42) +0.0%\n> 7519.13: status -uno (fsmonitor=) 0.37(0.30+0.82) 0.37(0.33+0.79) +0.0%\n> 7519.14: status -uall (fsmonitor=) 1.58(0.97+1.35) 1.57(0.86+1.45) -0.6%\n> 7519.15: diff (fsmonitor=) 0.34(0.28+0.83) 0.34(0.27+0.83) +0.0%\n> 7519.16: diff -- 0_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.08+0.02) +0.0%\n> 7519.17: diff -- 10_files (fsmonitor=) 0.09(0.07+0.03) 0.09(0.06+0.05) +0.0%\n> 7519.18: diff -- 100_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.06+0.04) +0.0%\n> 7519.19: diff -- 1000_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.05+0.05) +0.0%\n> 7519.20: diff -- 10000_files (fsmonitor=) 0.10(0.08+0.04) 0.10(0.06+0.05) +0.0%\n\nGreat! No slow-down without fsmonitor enabled, as expected. Fantastic.\n\n> I also added a benchmark for a tiny git diff workload w/ a pathspec.\n> I see an approximately .02 second overhead added w/ and w/o fsmonitor\n>\n> From looking at these results, I suspected that refresh_fsmonitor\n> is already happening during git diff - independent of this patch\n> series' optimization. Confirmed that suspicion by breaking on\n> refresh_fsmonitor.\n\nSo, the overhead that we're paying is purely the pipe+fork+exec? I.e.,\nthat watchman has already computed an answer in the earlier call, and we\njust have to read it again (or find out that the last results were\nunchanged)?\n\n> (gdb) bt [simplified]\n> 0 refresh_fsmonitor at fsmonitor.c:176\n> 1 ie_match_stat at read-cache.c:375\n> 2 match_stat_with_submodule at diff-lib.c:237\n> 4 builtin_diff_files at builtin/diff.c:260\n> 5 cmd_diff at builtin/diff.c:541\n> 6 run_builtin at git.c:450\n> 7 handle_builtin at git.c:700\n> 8 run_argv at git.c:767\n> 9 cmd_main at git.c:898\n> 10 main at common-main.c:52\n\n:-).\n\n> Signed-off-by: Nipunn Koorapati \n> ---\n> t/perf/p7519-fsmonitor.sh | 71 +++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 71 insertions(+)\n>\n> diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\n> index 9313d4a51d..2b4803707f 100755\n> --- a/t/perf/p7519-fsmonitor.sh\n> +++ b/t/perf/p7519-fsmonitor.sh\n> @@ -115,6 +115,13 @@ test_expect_success \"setup for fsmonitor\" '\n\nEverything in here looks very reasonable to me, except for the seq vs.\ntest_seq() issue that I pointed out in another email in this thread.\n\nIt's too bad that we have to write these twice, but that's not the fault\nof your patch.\n\nThanks,\nTaylor\n"},{"id":"407923","messageId":"CAN8Z4-VxEFvPJwA987xWV9d54geyTJJgaqc0FRL-0MuQgVS5ew@mail.gmail.com","threadId":"54452","inReplyTo":"20201019215438.GA49623@nand.local","subject":"Re: [PATCH v2 4/4] t/perf: add fsmonitor perf test for git diff","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-19T22:00:05Z","receivedAt":"2020-10-19T22:00:21Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> I was wondering how this slipped through since it should be checked\n> automatically by t/check-non-portable-shell.pl, but that is only run\n> from t/Makefile, not t/perf/Makefile. That probably explains how a few\n> raw `seq`'s made it into t/perf.\n\nMakefile issue is easy enough to fix - will fix in another commit\n"},{"id":"407925","messageId":"20201019220258.GC49623@nand.local","threadId":"54452","inReplyTo":"CAN8Z4-VxEFvPJwA987xWV9d54geyTJJgaqc0FRL-0MuQgVS5ew@mail.gmail.com","subject":"Re: [PATCH v2 4/4] t/perf: add fsmonitor perf test for git diff","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-19T22:02:58Z","receivedAt":"2020-10-19T22:03:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 19, 2020 at 11:00:05PM +0100, Nipunn Koorapati wrote:\n> > I was wondering how this slipped through since it should be checked\n> > automatically by t/check-non-portable-shell.pl, but that is only run\n> > from t/Makefile, not t/perf/Makefile. That probably explains how a few\n> > raw `seq`'s made it into t/perf.\n>\n> Makefile issue is easy enough to fix - will fix in another commit\n\nThanks; I don't think that you need to worry about touching up t/perf's\nMakefile (although I'd be very grateful if you did!), but rather just\nswapping new invocations of seq to use 'test_seq()' would be sufficient\nin and of itself.\n\nThanks,\nTaylor\n"},{"id":"407927","messageId":"CAN8Z4-VVx70ZDk8_pDH7cjoOZwVTCefEP1QCYCYpqRYhAn9SXQ@mail.gmail.com","threadId":"54452","inReplyTo":"20201019215438.GA49623@nand.local","subject":"Re: [PATCH v2 4/4] t/perf: add fsmonitor perf test for git diff","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-19T22:25:19Z","receivedAt":"2020-10-19T22:25:34Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> This reminds me; did you look at the 'git add' performance change? I\n> recall Junio mentioning that 'git add' takes the same paths in the code.\n\nI did look into it and didn't see a big perf change - and didn't dig into why.\nI'll leave a perf test in the next roll of this patch series so you can see the\nnumbers.\n\n--Nipunn\n"},{"id":"407928","messageId":"pull.756.v3.git.1603147657.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v2.git.1603143316.gitgitgadget@gmail.com","subject":"[PATCH v3 0/7] use fsmonitor data in git diff eliminating O(num_files) calls to lstat","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T22:47:30Z","receivedAt":"2020-10-19T22:47:42Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Credit to alexmv who made this commit back in Dec, 2017 when he was at dbx.\nI've rebased it and am submitting it now.\n\nWith fsmonitor enabled, git diff currently lstats every file in the repo\nThis makes use of the fsmonitor extension to skip lstat() calls on files\nthat fsmonitor judged as unmodified.\n\nI was able to do some testing with/without this change in a large in-house\nrepo (~ 400k files).\n\n-----------------------------------------\n(1) With fsmonitor enabled - on master of git (2.29.0)\n-----------------------------------------\n../git/bin-wrappers/git checkout HEAD~200\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 99.64 4.358994 10 446257 3 lstat\n 0.12 0.005353 7 764 360 open\n\n(A subsequent call)\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 99.84 4.380955 10 444904 3 lstat\n 0.06 0.002564 135 19 munmap\n...\n\n-----------------------------------------\n(2) With fsmonitor enabled - with my patch\n-----------------------------------------\n../git/bin-wrappers/git checkout HEAD~200\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 50.72 0.003090 163 19 munmap\n 19.63 0.001196 598 2 futex\n...\n 0.00 0.000000 0 4 3 lstat\n\n\n-----------------------------------------\n(3) With fsmonitor disabled entirely\n-----------------------------------------\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 98.52 0.277085 92362 3 futex\n 0.27 0.000752 4 191 63 open\n...\n 0.14 0.000397 3 158 3 lstat\n\nI was able to encode this into a perf test in one of the commits.\n\nChanges since Patch Series V1\n\n * Add git diff -- to perf tests\n * improve readability of bitwise ops\n\nAlex Vandiver (1):\n fsmonitor: use fsmonitor data in `git diff`\n\nNipunn Koorapati (6):\n t/perf/README: elaborate on output format\n t/perf/p7519-fsmonitor.sh: warm cache on first git status\n t/perf: add fsmonitor perf test for git diff\n perf lint: check test-lint-shell-syntax in perf tests\n p7519-fsmonitor: refactor to avoid code duplication\n p7519-fsmonitor: add a git add benchmark\n\n diff-lib.c | 15 +++++-\n t/Makefile | 3 +-\n t/perf/README | 2 +\n t/perf/p3400-rebase.sh | 6 +--\n t/perf/p7519-fsmonitor.sh | 96 ++++++++++++++++++++++-----------------\n 5 files changed, 75 insertions(+), 47 deletions(-)\n\n\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-756%2Fnipunn1313%2Fdiff_fsmon-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-756/nipunn1313/diff_fsmon-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/756\n\nRange-diff vs v2:\n\n 1: cba03dd40b = 1: cba03dd40b fsmonitor: use fsmonitor data in `git diff`\n 2: 1c7876166f = 2: 1c7876166f t/perf/README: elaborate on output format\n 3: 401f696c81 = 3: 401f696c81 t/perf/p7519-fsmonitor.sh: warm cache on first git status\n 4: f572e226bb ! 4: b3ad8faac4 t/perf: add fsmonitor perf test for git diff\n @@ t/perf/p7519-fsmonitor.sh: test_expect_success \"setup for fsmonitor\" '\n \tgit config core.fsmonitor \"$INTEGRATION_SCRIPT\" &&\n \tgit update-index --fsmonitor &&\n +\tmkdir 1_file 10_files 100_files 1000_files 10000_files &&\n -+\tfor i in `seq 1 10`; do touch 10_files/$i; done &&\n -+\tfor i in `seq 1 100`; do touch 100_files/$i; done &&\n -+\tfor i in `seq 1 1000`; do touch 1000_files/$i; done &&\n -+\tfor i in `seq 1 10000`; do touch 10000_files/$i; done &&\n ++\tfor i in $(test_seq 1 10); do touch 10_files/$i; done &&\n ++\tfor i in $(test_seq 1 100); do touch 100_files/$i; done &&\n ++\tfor i in $(test_seq 1 1000); do touch 1000_files/$i; done &&\n ++\tfor i in $(test_seq 1 10000); do touch 10000_files/$i; done &&\n +\tgit add 1_file 10_files 100_files 1000_files 10000_files &&\n +\tgit commit -m \"Add files\" &&\n \tgit status # Warm caches\n -: ---------- > 5: 28c1e488bf perf lint: check test-lint-shell-syntax in perf tests\n -: ---------- > 6: b38f2984f9 p7519-fsmonitor: refactor to avoid code duplication\n -: ---------- > 7: d392a523f2 p7519-fsmonitor: add a git add benchmark\n\n-- \ngitgitgadget\n"},{"id":"407929","messageId":"401f696c8156acafd1bf91511fde7ae099ff9052.1603147657.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v3.git.1603147657.gitgitgadget@gmail.com","subject":"[PATCH v3 3/7] t/perf/p7519-fsmonitor.sh: warm cache on first git status","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T22:47:33Z","receivedAt":"2020-10-19T22:47:44Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nThe first git status would be inflated due to warming of\nfilesystem cache. This makes the results comparable.\n\nBefore\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 2.52(1.59+1.56)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.18(0.12+0.06)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62)\n7519.7: status (fsmonitor=) 0.69(0.52+0.90)\n7519.8: status -uno (fsmonitor=) 0.37(0.28+0.81)\n7519.9: status -uall (fsmonitor=) 1.53(0.93+1.32)\n\nAfter\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 0.39(0.33+0.06)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.17(0.13+0.05)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.34(0.77+0.56)\n7519.7: status (fsmonitor=) 0.70(0.53+0.90)\n7519.8: status -uno (fsmonitor=) 0.37(0.32+0.78)\n7519.9: status -uall (fsmonitor=) 1.55(1.01+1.25)\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex def7ecdbc7..9313d4a51d 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -114,7 +114,8 @@ test_expect_success \"setup for fsmonitor\" '\n \tfi &&\n \n \tgit config core.fsmonitor \"$INTEGRATION_SCRIPT\" &&\n-\tgit update-index --fsmonitor\n+\tgit update-index --fsmonitor &&\n+\tgit status # Warm caches\n '\n \n if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-- \ngitgitgadget\n\n"},{"id":"407930","messageId":"cba03dd40bc6af965eb33eba87ea490588dc6bcc.1603147657.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v3.git.1603147657.gitgitgadget@gmail.com","subject":"[PATCH v3 1/7] fsmonitor: use fsmonitor data in `git diff`","fromName":"Alex Vandiver via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T22:47:31Z","receivedAt":"2020-10-19T22:47:44Z","isPatch":true,"sender":{"key":"alex@chmrr.net","avatar":"https://avatars.githubusercontent.com/u/28347?v=4"},"body":"From: Alex Vandiver \n\nWith fsmonitor enabled, the first call to match_stat_with_submodule\ncalls refresh_fsmonitor, incurring the overhead of reading the list of\nupdated files -- but run_diff_files does not respect the\nCE_FSMONITOR_VALID flag.\n\nMake use of the fsmonitor extension to skip lstat() calls on files\nthat fsmonitor judged as unmodified.\n\nNotably, this change improves performance of the git shell prompt when\nGIT_PS1_SHOWDIRTYSTATE is set.\n\nSigned-off-by: Alex Vandiver \nSigned-off-by: Nipunn Koorapati \n---\n diff-lib.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f95c6de75f..d2d31b9f82 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -97,6 +97,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n \n+\trefresh_fsmonitor(istate);\n+\n \tif (diff_unmerged_stage < 0)\n \t\tdiff_unmerged_stage = 2;\n \tentries = istate->cache_nr;\n@@ -197,8 +199,17 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (ce_uptodate(ce) || ce_skip_worktree(ce))\n \t\t\tcontinue;\n \n-\t\t/* If CE_VALID is set, don't look at workdir for file removal */\n-\t\tif (ce->ce_flags & CE_VALID) {\n+\t\t/*\n+\t\t * When CE_VALID is set (via \"update-index --assume-unchanged\"\n+\t\t * or via adding paths while core.ignorestat is set to true),\n+\t\t * the user has promised that the working tree file for that\n+\t\t * path will not be modified. When CE_FSMONITOR_VALID is true,\n+\t\t * the fsmonitor knows that the path hasn't been modified since\n+\t\t * we refreshed the cached stat information. In either case,\n+\t\t * we do not have to stat to see if the path has been removed\n+\t\t * or modified.\n+\t\t */\n+\t\tif (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {\n \t\t\tchanged = 0;\n \t\t\tnewmode = ce->ce_mode;\n \t\t} else {\n-- \ngitgitgadget\n\n"},{"id":"407931","messageId":"1c7876166f5d9262c44c9df0f613e7d0beb98722.1603147657.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v3.git.1603147657.gitgitgadget@gmail.com","subject":"[PATCH v3 2/7] t/perf/README: elaborate on output format","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T22:47:32Z","receivedAt":"2020-10-19T22:47:45Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/README | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/perf/README b/t/perf/README\nindex bd649afa97..fb9127a66f 100644\n--- a/t/perf/README\n+++ b/t/perf/README\n@@ -28,6 +28,8 @@ the tests on the current git repository.\n 7810.3: grep --cached, cheap regex 3.07(3.02+0.25)\n 7810.4: grep --cached, expensive regex 9.39(30.57+0.24)\n \n+Output format is in seconds \"Elapsed(User + System)\"\n+\n You can compare multiple repositories and even git revisions with the\n 'run' script:\n \n-- \ngitgitgadget\n\n"},{"id":"407932","messageId":"28c1e488bf644786af071e66b73450baa47ccc44.1603147657.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v3.git.1603147657.gitgitgadget@gmail.com","subject":"[PATCH v3 5/7] perf lint: check test-lint-shell-syntax in perf tests","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T22:47:35Z","receivedAt":"2020-10-19T22:47:48Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nPerf tests have some seq instead of test_seq. This\nruns the existing tests on the perf tests as well.\n\nSigned-off-by: Nipunn Koorapati \n---\n t/Makefile | 3 ++-\n t/perf/p3400-rebase.sh | 6 +++---\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex c83fd18861..74b53af2bd 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -34,6 +34,7 @@ CHAINLINTTMP_SQ = $(subst ','\\'',$(CHAINLINTTMP))\n T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))\n TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))\n THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))\n+TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))\n CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n CHAINLINT = sed -f chainlint.sed\n \n@@ -91,7 +92,7 @@ test-lint-executable:\n \t\techo >&2 \"non-executable tests:\" $$bad; exit 1; }\n \n test-lint-shell-syntax:\n-\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)\n+\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)\n \n test-lint-filenames:\n \t@# We do *not* pass a glob to ls-files but use grep instead, to catch\ndiff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh\nindex d202aaed06..7a0bb29448 100755\n--- a/t/perf/p3400-rebase.sh\n+++ b/t/perf/p3400-rebase.sh\n@@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '\n \tgit checkout -f -B base &&\n \tgit checkout -B to-rebase &&\n \tgit checkout -B upstream &&\n-\tfor i in $(seq 100)\n+\tfor i in $(test_seq 100)\n \tdo\n \t\t# simulate huge diffs\n \t\techo change$i >unrelated-file$i &&\n-\t\tseq 1000 >>unrelated-file$i &&\n+\t\ttest_seq 1000 >>unrelated-file$i &&\n \t\tgit add unrelated-file$i &&\n \t\ttest_tick &&\n \t\tgit commit -m commit$i unrelated-file$i &&\n \t\techo change$i >unrelated-file$i &&\n-\t\tseq 1000 | tac >>unrelated-file$i &&\n+\t\ttest_seq 1000 | tac >>unrelated-file$i &&\n \t\tgit add unrelated-file$i &&\n \t\ttest_tick &&\n \t\tgit commit -m commit$i-reverse unrelated-file$i ||\n-- \ngitgitgadget\n\n"},{"id":"407933","messageId":"d392a523f239f92f1b47ec1a96885f69fdc6d8b4.1603147657.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v3.git.1603147657.gitgitgadget@gmail.com","subject":"[PATCH v3 7/7] p7519-fsmonitor: add a git add benchmark","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T22:47:37Z","receivedAt":"2020-10-19T22:47:49Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nTest v2.29.0-rc1 this tree\n-----------------------------------------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 1.48(0.79+0.67) 1.48(0.79+0.67) +0.0%\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.16(0.11+0.05) 0.17(0.13+0.04) +6.3%\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.77+0.58) 1.37(0.72+0.63) +0.7%\n7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.84(0.21+0.63) 0.14(0.11+0.03) -83.3%\n7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.07+0.05) 0.13(0.09+0.04) +8.3%\n7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.09+0.04) 0.13(0.07+0.06) +8.3%\n7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.05) 0.12(0.08+0.05) +0.0%\n7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.05) 0.13(0.09+0.04) +8.3%\n7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.14(0.08+0.06) 0.13(0.07+0.06) -7.1%\n7519.11: add (fsmonitor=.git/hooks/fsmonitor-watchman) 2.75(1.41+1.27) 2.03(1.26+0.70) -26.2%\n7519.13: status (fsmonitor=) 1.38(1.03+1.04) 1.37(1.04+1.04) -0.7%\n7519.14: status -uno (fsmonitor=) 1.11(0.83+0.98) 1.10(0.89+0.90) -0.9%\n7519.15: status -uall (fsmonitor=) 2.30(1.57+1.42) 2.31(1.49+1.50) +0.4%\n7519.16: diff (fsmonitor=) 1.43(1.13+1.76) 1.46(1.19+1.72) +2.1%\n7519.17: diff -- 0_files (fsmonitor=) 0.10(0.08+0.04) 0.11(0.08+0.04) +10.0%\n7519.18: diff -- 10_files (fsmonitor=) 0.10(0.07+0.05) 0.11(0.08+0.04) +10.0%\n7519.19: diff -- 100_files (fsmonitor=) 0.10(0.07+0.04) 0.11(0.07+0.05) +10.0%\n7519.20: diff -- 1000_files (fsmonitor=) 0.10(0.08+0.03) 0.11(0.08+0.04) +10.0%\n7519.21: diff -- 10000_files (fsmonitor=) 0.11(0.08+0.05) 0.12(0.07+0.06) +9.1%\n7519.22: add (fsmonitor=) 2.26(1.46+1.49) 2.27(1.42+1.55) +0.4%\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex 75a0cef01d..fb20fe0937 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -169,6 +169,10 @@ test_fsmonitor_suite() {\n \ttest_perf_w_drop_caches \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n \t\tgit diff -- 10000_files\n \t'\n+\n+\ttest_perf_w_drop_caches \"add (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit add --all\n+\t'\n }\n \n test_fsmonitor_suite\n-- \ngitgitgadget\n"},{"id":"407934","messageId":"b3ad8faac43f7e499c794b4a9c106f9fcc121281.1603147657.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v3.git.1603147657.gitgitgadget@gmail.com","subject":"[PATCH v3 4/7] t/perf: add fsmonitor perf test for git diff","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T22:47:34Z","receivedAt":"2020-10-19T22:47:49Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nResults for the git-diff fsmonitor optimization\nin patch in the parent-rev (using a 400k file repo to test)\n\nAs you can see here - git diff with fsmonitor running is\nsignificantly better with this patch series (80% faster on my\nworkload)!\n\nGIT_PERF_LARGE_REPO=~/src/server ./run v2.29.0-rc1 . -- p7519-fsmonitor.sh\n\nTest v2.29.0-rc1 this tree\n-----------------------------------------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 1.46(0.82+0.64) 1.47(0.83+0.62) +0.7%\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.16(0.12+0.04) 0.17(0.12+0.05) +6.3%\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62) 1.37(0.76+0.60) +0.7%\n7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.85(0.22+0.63) 0.14(0.10+0.05) -83.5%\n7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.05) 0.13(0.11+0.02) +8.3%\n7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.04) 0.13(0.09+0.04) +8.3%\n7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.07+0.05) 0.13(0.07+0.06) +8.3%\n7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.09+0.04) 0.13(0.08+0.05) +8.3%\n7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.14(0.09+0.05) 0.13(0.10+0.03) -7.1%\n7519.12: status (fsmonitor=) 1.67(0.93+1.49) 1.67(0.99+1.42) +0.0%\n7519.13: status -uno (fsmonitor=) 0.37(0.30+0.82) 0.37(0.33+0.79) +0.0%\n7519.14: status -uall (fsmonitor=) 1.58(0.97+1.35) 1.57(0.86+1.45) -0.6%\n7519.15: diff (fsmonitor=) 0.34(0.28+0.83) 0.34(0.27+0.83) +0.0%\n7519.16: diff -- 0_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.08+0.02) +0.0%\n7519.17: diff -- 10_files (fsmonitor=) 0.09(0.07+0.03) 0.09(0.06+0.05) +0.0%\n7519.18: diff -- 100_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.06+0.04) +0.0%\n7519.19: diff -- 1000_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.05+0.05) +0.0%\n7519.20: diff -- 10000_files (fsmonitor=) 0.10(0.08+0.04) 0.10(0.06+0.05) +0.0%\n\nI also added a benchmark for a tiny git diff workload w/ a pathspec.\nI see an approximately .02 second overhead added w/ and w/o fsmonitor\n\nFrom looking at these results, I suspected that refresh_fsmonitor\nis already happening during git diff - independent of this patch\nseries' optimization. Confirmed that suspicion by breaking on\nrefresh_fsmonitor.\n\n(gdb) bt [simplified]\n0 refresh_fsmonitor at fsmonitor.c:176\n1 ie_match_stat at read-cache.c:375\n2 match_stat_with_submodule at diff-lib.c:237\n4 builtin_diff_files at builtin/diff.c:260\n5 cmd_diff at builtin/diff.c:541\n6 run_builtin at git.c:450\n7 handle_builtin at git.c:700\n8 run_argv at git.c:767\n9 cmd_main at git.c:898\n10 main at common-main.c:52\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 71 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 71 insertions(+)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex 9313d4a51d..ef4c3c8c5c 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -115,6 +115,13 @@ test_expect_success \"setup for fsmonitor\" '\n \n \tgit config core.fsmonitor \"$INTEGRATION_SCRIPT\" &&\n \tgit update-index --fsmonitor &&\n+\tmkdir 1_file 10_files 100_files 1000_files 10000_files &&\n+\tfor i in $(test_seq 1 10); do touch 10_files/$i; done &&\n+\tfor i in $(test_seq 1 100); do touch 100_files/$i; done &&\n+\tfor i in $(test_seq 1 1000); do touch 1000_files/$i; done &&\n+\tfor i in $(test_seq 1 10000); do touch 10000_files/$i; done &&\n+\tgit add 1_file 10_files 100_files 1000_files 10000_files &&\n+\tgit commit -m \"Add files\" &&\n \tgit status # Warm caches\n '\n \n@@ -142,6 +149,38 @@ test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n \tgit status -uall\n '\n \n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff\n+'\n+\n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1_file\n+'\n+\n+test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10_files\n+'\n+\n+test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 100_files\n+'\n+\n+test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1000_files\n+'\n+\n+test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10000_files\n+'\n+\n test_expect_success \"setup without fsmonitor\" '\n \tunset INTEGRATION_SCRIPT &&\n \tgit config --unset core.fsmonitor &&\n@@ -172,6 +211,38 @@ test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n \tgit status -uall\n '\n \n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff\n+'\n+\n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1_file\n+'\n+\n+test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10_files\n+'\n+\n+test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 100_files\n+'\n+\n+test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1000_files\n+'\n+\n+test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10000_files\n+'\n+\n if test_have_prereq WATCHMAN\n then\n \twatchman watch-del \"$GIT_WORK_TREE\" >/dev/null 2>&1 &&\n-- \ngitgitgadget\n\n"},{"id":"407935","messageId":"b38f2984f93488d6582eff4865d6d5293491ce60.1603147657.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v3.git.1603147657.gitgitgadget@gmail.com","subject":"[PATCH v3 6/7] p7519-fsmonitor: refactor to avoid code duplication","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-19T22:47:36Z","receivedAt":"2020-10-19T22:47:51Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nMuch of the benchmark code is redundant. This is\neasier to understand and edit.\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 136 +++++++++++---------------------------\n 1 file changed, 37 insertions(+), 99 deletions(-)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex ef4c3c8c5c..75a0cef01d 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -125,61 +125,53 @@ test_expect_success \"setup for fsmonitor\" '\n \tgit status # Warm caches\n '\n \n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n+test_perf_w_drop_caches () {\n+\tif test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\t\ttest-tool drop-caches\n+\tfi\n \n-test_perf \"status (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status\n-'\n+\ttest_perf \"$@\"\n+}\n \n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n+test_fsmonitor_suite() {\n+\ttest_perf_w_drop_caches \"status (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit status\n+\t'\n \n-test_perf \"status -uno (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status -uno\n-'\n+\ttest_perf_w_drop_caches \"status -uno (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit status -uno\n+\t'\n \n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n+\ttest_perf_w_drop_caches \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit status -uall\n+\t'\n \n-test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status -uall\n-'\n-\n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff\n-'\n+\ttest_perf_w_drop_caches \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff\n+\t'\n \n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n+\ttest_perf_w_drop_caches \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff -- 1_file\n+\t'\n \n-test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 1_file\n-'\n+\ttest_perf_w_drop_caches \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff -- 10_files\n+\t'\n \n-test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 10_files\n-'\n+\ttest_perf_w_drop_caches \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff -- 100_files\n+\t'\n \n-test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 100_files\n-'\n+\ttest_perf_w_drop_caches \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff -- 1000_files\n+\t'\n \n-test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 1000_files\n-'\n+\ttest_perf_w_drop_caches \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff -- 10000_files\n+\t'\n+}\n \n-test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 10000_files\n-'\n+test_fsmonitor_suite\n \n test_expect_success \"setup without fsmonitor\" '\n \tunset INTEGRATION_SCRIPT &&\n@@ -187,61 +179,7 @@ test_expect_success \"setup without fsmonitor\" '\n \tgit update-index --no-fsmonitor\n '\n \n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"status (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status\n-'\n-\n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"status -uno (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status -uno\n-'\n-\n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status -uall\n-'\n-\n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff\n-'\n-\n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 1_file\n-'\n-\n-test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 10_files\n-'\n-\n-test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 100_files\n-'\n-\n-test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 1000_files\n-'\n-\n-test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 10000_files\n-'\n+test_fsmonitor_suite\n \n if test_have_prereq WATCHMAN\n then\n-- \ngitgitgadget\n\n"},{"id":"407936","messageId":"CAN8Z4-UaVgU59uqyWvwG-+U5TKyhGn800gaayCpzM3kukTYBSQ@mail.gmail.com","threadId":"54452","inReplyTo":"d392a523f239f92f1b47ec1a96885f69fdc6d8b4.1603147657.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 7/7] p7519-fsmonitor: add a git add benchmark","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-19T23:02:15Z","receivedAt":"2020-10-19T23:02:28Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Actually found a 25% improvement here on git add with this patch series\n"},{"id":"407949","messageId":"20201020023857.GC54484@nand.local","threadId":"54452","inReplyTo":"28c1e488bf644786af071e66b73450baa47ccc44.1603147657.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 5/7] perf lint: check test-lint-shell-syntax in perf tests","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-20T02:38:57Z","receivedAt":"2020-10-20T02:39:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 19, 2020 at 10:47:35PM +0000, Nipunn Koorapati via GitGitGadget wrote:\n> test-lint-shell-syntax:\n> -\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)\n> +\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)\n\nI really appreciate your initiative to modify t/Makefile to start\nlinting t/perf/p????-*.sh files, too. Could I bother you to elaborate a\nlittle bit on why you chose to modify a recipe in t/Makefile instead of\nt/perf/Makefile?\n\nI'm not necessarily opposed, but having this in t/perf/Makefile would\nallow me to just run 'make' in 't/perf' and still have the scripts\nlinted there without having to involve a 'make' in 't'.\n\nFor what it's worth, I suspect that this is because 't/Makefile' already\nhas a 'test-lint-shell-syntax' target, and 't/perf/Makefile' does not. I\nthink it would be OK to add it there, too, and move this change into\nt/perf.\n\n> diff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh\n> index d202aaed06..7a0bb29448 100755\n> --- a/t/perf/p3400-rebase.sh\n> +++ b/t/perf/p3400-rebase.sh\n> @@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '\n> \tgit checkout -f -B base &&\n> \tgit checkout -B to-rebase &&\n> \tgit checkout -B upstream &&\n> -\tfor i in $(seq 100)\n> +\tfor i in $(test_seq 100)\n> \tdo\n> \t\t# simulate huge diffs\n> \t\techo change$i >unrelated-file$i &&\n> -\t\tseq 1000 >>unrelated-file$i &&\n> +\t\ttest_seq 1000 >>unrelated-file$i &&\n> \t\tgit add unrelated-file$i &&\n> \t\ttest_tick &&\n> \t\tgit commit -m commit$i unrelated-file$i &&\n> \t\techo change$i >unrelated-file$i &&\n> -\t\tseq 1000 | tac >>unrelated-file$i &&\n> +\t\ttest_seq 1000 | tac >>unrelated-file$i &&\n\nMakes sense. I wouldn't be opposed to breaking this out into an earlier\nchange (e.g., \"it's about to become not OK to use seq in t/perf, so\nprepare for that by replacing any invocations with test_seq()\"), but I\nthink it's probably not worth it, since this patch is small as it is.\n\nThanks,\nTaylor\n"},{"id":"407950","messageId":"20201020024018.GD54484@nand.local","threadId":"54452","inReplyTo":"d392a523f239f92f1b47ec1a96885f69fdc6d8b4.1603147657.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 7/7] p7519-fsmonitor: add a git add benchmark","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-20T02:40:18Z","receivedAt":"2020-10-20T02:40:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 19, 2020 at 10:47:37PM +0000, Nipunn Koorapati via GitGitGadget wrote:\n> From: Nipunn Koorapati \n>\n> Test v2.29.0-rc1 this tree\n> -----------------------------------------------------------------------------------------------------------------\n> [...]\n> 7519.22: add (fsmonitor=) 2.26(1.46+1.49) 2.27(1.42+1.55) +0.4%\n\nGood; no huge slow-down here. Thanks for checking!\n\n> Signed-off-by: Nipunn Koorapati \n> ---\n> t/perf/p7519-fsmonitor.sh | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\n> index 75a0cef01d..fb20fe0937 100755\n> --- a/t/perf/p7519-fsmonitor.sh\n> +++ b/t/perf/p7519-fsmonitor.sh\n> @@ -169,6 +169,10 @@ test_fsmonitor_suite() {\n> \ttest_perf_w_drop_caches \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n> \t\tgit diff -- 10000_files\n> \t'\n> +\n> +\ttest_perf_w_drop_caches \"add (fsmonitor=$INTEGRATION_SCRIPT)\" '\n> +\t\tgit add --all\n> +\t'\n> }\n>\n> test_fsmonitor_suite\n> --\n> gitgitgadget\n\n Acked-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"407951","messageId":"20201020024329.GE54484@nand.local","threadId":"54452","inReplyTo":"b38f2984f93488d6582eff4865d6d5293491ce60.1603147657.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 6/7] p7519-fsmonitor: refactor to avoid code duplication","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-20T02:43:29Z","receivedAt":"2020-10-20T02:43:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 19, 2020 at 10:47:36PM +0000, Nipunn Koorapati via GitGitGadget wrote:\n> From: Nipunn Koorapati \n>\n> Much of the benchmark code is redundant. This is\n> easier to understand and edit.\n>\n> Signed-off-by: Nipunn Koorapati \n\nMuch easier to read, thank you for taking the time to simplify the code.\n\nI know that this is maybe more review than you were hoping for, but I\nthink it's been worth it and the series is in an even better state than\nwhen you started.\n\n Acked-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"407954","messageId":"xmqqlfg1d22n.fsf@gitster.c.googlers.com","threadId":"54452","inReplyTo":"20201020023857.GC54484@nand.local","subject":"Re: [PATCH v3 5/7] perf lint: check test-lint-shell-syntax in perf tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-20T03:10:56Z","receivedAt":"2020-10-20T03:11:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> \t\techo change$i >unrelated-file$i &&\n>> -\t\tseq 1000 | tac >>unrelated-file$i &&\n>> +\t\ttest_seq 1000 | tac >>unrelated-file$i &&\n>\n> Makes sense. I wouldn't be opposed to breaking this out into an earlier\n> change (e.g., \"it's about to become not OK to use seq in t/perf, so\n> prepare for that by replacing any invocations with test_seq()\"), but I\n> think it's probably not worth it, since this patch is small as it is.\n\ntest_seq is fine, but I do not think tac is portable (only saved by\nthe fact that not many people, especially on exotic platforms, run\nperf scripts).\n"},{"id":"407955","messageId":"20201020031509.GA56322@nand.local","threadId":"54452","inReplyTo":"xmqqlfg1d22n.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 5/7] perf lint: check test-lint-shell-syntax in perf tests","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-20T03:15:09Z","receivedAt":"2020-10-20T03:15:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 19, 2020 at 08:10:56PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> >> \t\techo change$i >unrelated-file$i &&\n> >> -\t\tseq 1000 | tac >>unrelated-file$i &&\n> >> +\t\ttest_seq 1000 | tac >>unrelated-file$i &&\n> >\n> > Makes sense. I wouldn't be opposed to breaking this out into an earlier\n> > change (e.g., \"it's about to become not OK to use seq in t/perf, so\n> > prepare for that by replacing any invocations with test_seq()\"), but I\n> > think it's probably not worth it, since this patch is small as it is.\n>\n> test_seq is fine, but I do not think tac is portable (only saved by\n> the fact that not many people, especially on exotic platforms, run\n> perf scripts).\n\nServes me right for reading while I'm tired! I glazed right over 'tac'.\nIf you need a truly unrelated file, you could write random data into it\n(there are some examples in t/test-lib-functions.sh), but I'd just write\n'test_seq 1001'.\n\nThanks,\nTaylor\n"},{"id":"407967","messageId":"CAN8Z4-WqMSynUNZpyevq09cMqE0dcxY5RwXbs-i8zjqzvRjo3Q@mail.gmail.com","threadId":"54452","inReplyTo":"20201020023857.GC54484@nand.local","subject":"Re: [PATCH v3 5/7] perf lint: check test-lint-shell-syntax in perf tests","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-20T10:09:49Z","receivedAt":"2020-10-20T10:10:03Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"On Tue, Oct 20, 2020 at 3:39 AM Taylor Blau wrote:\n>\n> I'm not necessarily opposed, but having this in t/perf/Makefile would\n> allow me to just run 'make' in 't/perf' and still have the scripts\n> linted there without having to involve a 'make' in 't'.\n>\n> For what it's worth, I suspect that this is because 't/Makefile' already\n> has a 'test-lint-shell-syntax' target, and 't/perf/Makefile' does not. I\n> think it would be OK to add it there, too, and move this change into\n> t/perf.\n\nLooked at doing this and noticed that there are several targets in test-lint\nin t/Makefile. This would involve duplicating them into t/perf/Makefile which\nseems like it would be poor form, especially given their complexity.\nPerhaps t/perf/Makefile could have a target which calls t/Makefile's test-lint\ntarget instead. Will play around with it.\n\n>\n> Makes sense. I wouldn't be opposed to breaking this out into an earlier\n> change (e.g., \"it's about to become not OK to use seq in t/perf, so\n> prepare for that by replacing any invocations with test_seq()\"), but I\n> think it's probably not worth it, since this patch is small as it is.\n>\n\nYeah - I see the point, but I agree that since the patch is small,\nit's ok this way.\nIf the patch grows significantly, I can make it into two patches\n\n--Nipunn\n"},{"id":"407968","messageId":"CAN8Z4-V05AXvxBktMim=m8v3CyL5_HmcuicAb8jY+UuqkWL4Tw@mail.gmail.com","threadId":"54452","inReplyTo":"20201020031509.GA56322@nand.local","subject":"Re: [PATCH v3 5/7] perf lint: check test-lint-shell-syntax in perf tests","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-20T10:16:20Z","receivedAt":"2020-10-20T10:16:34Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> > test_seq is fine, but I do not think tac is portable (only saved by\n> > the fact that not many people, especially on exotic platforms, run\n> > perf scripts).\n>\n> Serves me right for reading while I'm tired! I glazed right over 'tac'.\n> If you need a truly unrelated file, you could write random data into it\n> (there are some examples in t/test-lib-functions.sh), but I'd just write\n> 'test_seq 1001'.\n\nSeems like there might be some value to adding `tac` to the perl script\ncheck-non-portable-shell.pl - though I'm not sure what we'd use as an\nalternative. I'll leave this here for now for someone else to handle in\na follow up patch series\n"},{"id":"407982","messageId":"pull.756.v4.git.1603201264.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v3.git.1603147657.gitgitgadget@gmail.com","subject":"[PATCH v4 0/7] use fsmonitor data in git diff eliminating O(num_files) calls to lstat","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-20T13:40:57Z","receivedAt":"2020-10-20T13:41:10Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Credit to alexmv who made this commit back in Dec, 2017 when he was at dbx.\nI've rebased it and am submitting it now.\n\nWith fsmonitor enabled, git diff currently lstats every file in the repo\nThis makes use of the fsmonitor extension to skip lstat() calls on files\nthat fsmonitor judged as unmodified.\n\nI was able to do some testing with/without this change in a large in-house\nrepo (~ 400k files).\n\n-----------------------------------------\n(1) With fsmonitor enabled - on master of git (2.29.0)\n-----------------------------------------\n../git/bin-wrappers/git checkout HEAD~200\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 99.64 4.358994 10 446257 3 lstat\n 0.12 0.005353 7 764 360 open\n\n(A subsequent call)\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 99.84 4.380955 10 444904 3 lstat\n 0.06 0.002564 135 19 munmap\n...\n\n-----------------------------------------\n(2) With fsmonitor enabled - with my patch\n-----------------------------------------\n../git/bin-wrappers/git checkout HEAD~200\nstrace -c ../git/bin-wrappers/git diff\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 50.72 0.003090 163 19 munmap\n 19.63 0.001196 598 2 futex\n...\n 0.00 0.000000 0 4 3 lstat\n\n\n-----------------------------------------\n(3) With fsmonitor disabled entirely\n-----------------------------------------\n\n% time seconds usecs/call calls errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 98.52 0.277085 92362 3 futex\n 0.27 0.000752 4 191 63 open\n...\n 0.14 0.000397 3 158 3 lstat\n\nI was able to encode this into a perf test in one of the commits.\n\nChanges since Patch Series V1\n\n * Add git diff -- to perf tests\n * improve readability of bitwise ops\n\nChanges since Patch Series V2\n\n * Add git add to perf tests\n * Refactor perf fsmonitor to simplify / remove redundancy\n * Add linting to perf tests\n * Added git diff -- for various sized pathspecs\n * Confirmed that refresh_fsmonitor was always being called / added to\n commit message\n\nChanges since Patch Series V3\n\n * Move perf test linting to Makefile in perf/ directory\n\nAlex Vandiver (1):\n fsmonitor: use fsmonitor data in `git diff`\n\nNipunn Koorapati (6):\n t/perf/README: elaborate on output format\n t/perf/p7519-fsmonitor.sh: warm cache on first git status\n t/perf: add fsmonitor perf test for git diff\n perf lint: add make test-lint to perf tests\n p7519-fsmonitor: refactor to avoid code duplication\n p7519-fsmonitor: add a git add benchmark\n\n diff-lib.c | 15 +++++-\n t/Makefile | 7 +--\n t/perf/Makefile | 5 +-\n t/perf/README | 2 +\n t/perf/p3400-rebase.sh | 6 +--\n t/perf/p7519-fsmonitor.sh | 96 ++++++++++++++++++++++-----------------\n 6 files changed, 81 insertions(+), 50 deletions(-)\n\n\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-756%2Fnipunn1313%2Fdiff_fsmon-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-756/nipunn1313/diff_fsmon-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/756\n\nRange-diff vs v3:\n\n 1: cba03dd40b = 1: cba03dd40b fsmonitor: use fsmonitor data in `git diff`\n 2: 1c7876166f = 2: 1c7876166f t/perf/README: elaborate on output format\n 3: 401f696c81 = 3: 401f696c81 t/perf/p7519-fsmonitor.sh: warm cache on first git status\n 4: b3ad8faac4 = 4: b3ad8faac4 t/perf: add fsmonitor perf test for git diff\n 5: 28c1e488bf ! 5: b534cd137a perf lint: check test-lint-shell-syntax in perf tests\n @@ Metadata\n Author: Nipunn Koorapati \n \n ## Commit message ##\n - perf lint: check test-lint-shell-syntax in perf tests\n + perf lint: add make test-lint to perf tests\n \n - Perf tests have some seq instead of test_seq. This\n - runs the existing tests on the perf tests as well.\n + Perf tests have not been linted for some time.\n + They've grown some seq instead of test_seq. This\n + runs the existing lints on the perf tests as well.\n \n Signed-off-by: Nipunn Koorapati \n \n @@ t/Makefile: CHAINLINTTMP_SQ = $(subst ','\\'',$(CHAINLINTTMP))\n CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n CHAINLINT = sed -f chainlint.sed\n \n -@@ t/Makefile: test-lint-executable:\n +@@ t/Makefile: test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \\\n + \ttest-lint-filenames\n + \n + test-lint-duplicates:\n +-\t@dups=`echo $(T) | tr ' ' '\\n' | sed 's/-.*//' | sort | uniq -d` && \\\n ++\t@dups=`echo $(T) $(TPERF) | tr ' ' '\\n' | sed 's/-.*//' | sort | uniq -d` && \\\n + \t\ttest -z \"$$dups\" || { \\\n + \t\techo >&2 \"duplicate test numbers:\" $$dups; exit 1; }\n + \n + test-lint-executable:\n +-\t@bad=`for i in $(T); do test -x \"$$i\" || echo $$i; done` && \\\n ++\t@bad=`for i in $(T) $(TPERF); do test -x \"$$i\" || echo $$i; done` && \\\n + \t\ttest -z \"$$bad\" || { \\\n \t\techo >&2 \"non-executable tests:\" $$bad; exit 1; }\n \n test-lint-shell-syntax:\n @@ t/Makefile: test-lint-executable:\n test-lint-filenames:\n \t@# We do *not* pass a glob to ls-files but use grep instead, to catch\n \n + ## t/perf/Makefile ##\n +@@\n + -include ../../config.mak\n + export GIT_TEST_OPTIONS\n + \n +-all: perf\n ++all: test-lint perf\n + \n + perf: pre-clean\n + \t./run\n +@@ t/perf/Makefile: pre-clean:\n + clean:\n + \trm -rf build \"trash directory\".* test-results\n + \n ++test-lint:\n ++\t$(MAKE) -C .. test-lint\n ++\n + .PHONY: all perf pre-clean clean\n +\n ## t/perf/p3400-rebase.sh ##\n @@ t/perf/p3400-rebase.sh: test_expect_success 'setup rebasing on top of a lot of changes' '\n \tgit checkout -f -B base &&\n 6: b38f2984f9 = 6: 3b20f4c76e p7519-fsmonitor: refactor to avoid code duplication\n 7: d392a523f2 = 7: 6f97439936 p7519-fsmonitor: add a git add benchmark\n\n-- \ngitgitgadget\n"},{"id":"407983","messageId":"cba03dd40bc6af965eb33eba87ea490588dc6bcc.1603201264.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v4.git.1603201264.gitgitgadget@gmail.com","subject":"[PATCH v4 1/7] fsmonitor: use fsmonitor data in `git diff`","fromName":"Alex Vandiver via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-20T13:40:58Z","receivedAt":"2020-10-20T13:41:11Z","isPatch":true,"sender":{"key":"alex@chmrr.net","avatar":"https://avatars.githubusercontent.com/u/28347?v=4"},"body":"From: Alex Vandiver \n\nWith fsmonitor enabled, the first call to match_stat_with_submodule\ncalls refresh_fsmonitor, incurring the overhead of reading the list of\nupdated files -- but run_diff_files does not respect the\nCE_FSMONITOR_VALID flag.\n\nMake use of the fsmonitor extension to skip lstat() calls on files\nthat fsmonitor judged as unmodified.\n\nNotably, this change improves performance of the git shell prompt when\nGIT_PS1_SHOWDIRTYSTATE is set.\n\nSigned-off-by: Alex Vandiver \nSigned-off-by: Nipunn Koorapati \n---\n diff-lib.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f95c6de75f..d2d31b9f82 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -97,6 +97,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n \n+\trefresh_fsmonitor(istate);\n+\n \tif (diff_unmerged_stage < 0)\n \t\tdiff_unmerged_stage = 2;\n \tentries = istate->cache_nr;\n@@ -197,8 +199,17 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (ce_uptodate(ce) || ce_skip_worktree(ce))\n \t\t\tcontinue;\n \n-\t\t/* If CE_VALID is set, don't look at workdir for file removal */\n-\t\tif (ce->ce_flags & CE_VALID) {\n+\t\t/*\n+\t\t * When CE_VALID is set (via \"update-index --assume-unchanged\"\n+\t\t * or via adding paths while core.ignorestat is set to true),\n+\t\t * the user has promised that the working tree file for that\n+\t\t * path will not be modified. When CE_FSMONITOR_VALID is true,\n+\t\t * the fsmonitor knows that the path hasn't been modified since\n+\t\t * we refreshed the cached stat information. In either case,\n+\t\t * we do not have to stat to see if the path has been removed\n+\t\t * or modified.\n+\t\t */\n+\t\tif (ce->ce_flags & (CE_VALID | CE_FSMONITOR_VALID)) {\n \t\t\tchanged = 0;\n \t\t\tnewmode = ce->ce_mode;\n \t\t} else {\n-- \ngitgitgadget\n\n"},{"id":"407984","messageId":"1c7876166f5d9262c44c9df0f613e7d0beb98722.1603201264.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v4.git.1603201264.gitgitgadget@gmail.com","subject":"[PATCH v4 2/7] t/perf/README: elaborate on output format","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-20T13:40:59Z","receivedAt":"2020-10-20T13:41:17Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/README | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/perf/README b/t/perf/README\nindex bd649afa97..fb9127a66f 100644\n--- a/t/perf/README\n+++ b/t/perf/README\n@@ -28,6 +28,8 @@ the tests on the current git repository.\n 7810.3: grep --cached, cheap regex 3.07(3.02+0.25)\n 7810.4: grep --cached, expensive regex 9.39(30.57+0.24)\n \n+Output format is in seconds \"Elapsed(User + System)\"\n+\n You can compare multiple repositories and even git revisions with the\n 'run' script:\n \n-- \ngitgitgadget\n\n"},{"id":"407985","messageId":"3b20f4c76e31e101da99f8f3e2933ea49b95c6ab.1603201265.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v4.git.1603201264.gitgitgadget@gmail.com","subject":"[PATCH v4 6/7] p7519-fsmonitor: refactor to avoid code duplication","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-20T13:41:03Z","receivedAt":"2020-10-20T13:41:19Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nMuch of the benchmark code is redundant. This is\neasier to understand and edit.\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 136 +++++++++++---------------------------\n 1 file changed, 37 insertions(+), 99 deletions(-)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex ef4c3c8c5c..75a0cef01d 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -125,61 +125,53 @@ test_expect_success \"setup for fsmonitor\" '\n \tgit status # Warm caches\n '\n \n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n+test_perf_w_drop_caches () {\n+\tif test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\t\ttest-tool drop-caches\n+\tfi\n \n-test_perf \"status (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status\n-'\n+\ttest_perf \"$@\"\n+}\n \n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n+test_fsmonitor_suite() {\n+\ttest_perf_w_drop_caches \"status (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit status\n+\t'\n \n-test_perf \"status -uno (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status -uno\n-'\n+\ttest_perf_w_drop_caches \"status -uno (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit status -uno\n+\t'\n \n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n+\ttest_perf_w_drop_caches \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit status -uall\n+\t'\n \n-test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status -uall\n-'\n-\n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff\n-'\n+\ttest_perf_w_drop_caches \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff\n+\t'\n \n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n+\ttest_perf_w_drop_caches \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff -- 1_file\n+\t'\n \n-test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 1_file\n-'\n+\ttest_perf_w_drop_caches \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff -- 10_files\n+\t'\n \n-test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 10_files\n-'\n+\ttest_perf_w_drop_caches \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff -- 100_files\n+\t'\n \n-test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 100_files\n-'\n+\ttest_perf_w_drop_caches \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff -- 1000_files\n+\t'\n \n-test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 1000_files\n-'\n+\ttest_perf_w_drop_caches \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit diff -- 10000_files\n+\t'\n+}\n \n-test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 10000_files\n-'\n+test_fsmonitor_suite\n \n test_expect_success \"setup without fsmonitor\" '\n \tunset INTEGRATION_SCRIPT &&\n@@ -187,61 +179,7 @@ test_expect_success \"setup without fsmonitor\" '\n \tgit update-index --no-fsmonitor\n '\n \n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"status (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status\n-'\n-\n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"status -uno (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status -uno\n-'\n-\n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit status -uall\n-'\n-\n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff\n-'\n-\n-if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-\ttest-tool drop-caches\n-fi\n-\n-test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 1_file\n-'\n-\n-test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 10_files\n-'\n-\n-test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 100_files\n-'\n-\n-test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 1000_files\n-'\n-\n-test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n-\tgit diff -- 10000_files\n-'\n+test_fsmonitor_suite\n \n if test_have_prereq WATCHMAN\n then\n-- \ngitgitgadget\n\n"},{"id":"407986","messageId":"401f696c8156acafd1bf91511fde7ae099ff9052.1603201264.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v4.git.1603201264.gitgitgadget@gmail.com","subject":"[PATCH v4 3/7] t/perf/p7519-fsmonitor.sh: warm cache on first git status","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-20T13:41:00Z","receivedAt":"2020-10-20T13:41:20Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nThe first git status would be inflated due to warming of\nfilesystem cache. This makes the results comparable.\n\nBefore\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 2.52(1.59+1.56)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.18(0.12+0.06)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62)\n7519.7: status (fsmonitor=) 0.69(0.52+0.90)\n7519.8: status -uno (fsmonitor=) 0.37(0.28+0.81)\n7519.9: status -uall (fsmonitor=) 1.53(0.93+1.32)\n\nAfter\nTest this tree\n--------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 0.39(0.33+0.06)\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.17(0.13+0.05)\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.34(0.77+0.56)\n7519.7: status (fsmonitor=) 0.70(0.53+0.90)\n7519.8: status -uno (fsmonitor=) 0.37(0.32+0.78)\n7519.9: status -uall (fsmonitor=) 1.55(1.01+1.25)\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex def7ecdbc7..9313d4a51d 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -114,7 +114,8 @@ test_expect_success \"setup for fsmonitor\" '\n \tfi &&\n \n \tgit config core.fsmonitor \"$INTEGRATION_SCRIPT\" &&\n-\tgit update-index --fsmonitor\n+\tgit update-index --fsmonitor &&\n+\tgit status # Warm caches\n '\n \n if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n-- \ngitgitgadget\n\n"},{"id":"407987","messageId":"b534cd137a833de802d6d95c1affb8d2d8f7de85.1603201265.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v4.git.1603201264.gitgitgadget@gmail.com","subject":"[PATCH v4 5/7] perf lint: add make test-lint to perf tests","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-20T13:41:02Z","receivedAt":"2020-10-20T13:41:23Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nPerf tests have not been linted for some time.\nThey've grown some seq instead of test_seq. This\nruns the existing lints on the perf tests as well.\n\nSigned-off-by: Nipunn Koorapati \n---\n t/Makefile | 7 ++++---\n t/perf/Makefile | 5 ++++-\n t/perf/p3400-rebase.sh | 6 +++---\n 3 files changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex c83fd18861..882d26eee3 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -34,6 +34,7 @@ CHAINLINTTMP_SQ = $(subst ','\\'',$(CHAINLINTTMP))\n T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))\n TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))\n THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))\n+TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))\n CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n CHAINLINT = sed -f chainlint.sed\n \n@@ -81,17 +82,17 @@ test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \\\n \ttest-lint-filenames\n \n test-lint-duplicates:\n-\t@dups=`echo $(T) | tr ' ' '\\n' | sed 's/-.*//' | sort | uniq -d` && \\\n+\t@dups=`echo $(T) $(TPERF) | tr ' ' '\\n' | sed 's/-.*//' | sort | uniq -d` && \\\n \t\ttest -z \"$$dups\" || { \\\n \t\techo >&2 \"duplicate test numbers:\" $$dups; exit 1; }\n \n test-lint-executable:\n-\t@bad=`for i in $(T); do test -x \"$$i\" || echo $$i; done` && \\\n+\t@bad=`for i in $(T) $(TPERF); do test -x \"$$i\" || echo $$i; done` && \\\n \t\ttest -z \"$$bad\" || { \\\n \t\techo >&2 \"non-executable tests:\" $$bad; exit 1; }\n \n test-lint-shell-syntax:\n-\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)\n+\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)\n \n test-lint-filenames:\n \t@# We do *not* pass a glob to ls-files but use grep instead, to catch\ndiff --git a/t/perf/Makefile b/t/perf/Makefile\nindex 8c47155a7c..fcb0e8865e 100644\n--- a/t/perf/Makefile\n+++ b/t/perf/Makefile\n@@ -1,7 +1,7 @@\n -include ../../config.mak\n export GIT_TEST_OPTIONS\n \n-all: perf\n+all: test-lint perf\n \n perf: pre-clean\n \t./run\n@@ -12,4 +12,7 @@ pre-clean:\n clean:\n \trm -rf build \"trash directory\".* test-results\n \n+test-lint:\n+\t$(MAKE) -C .. test-lint\n+\n .PHONY: all perf pre-clean clean\ndiff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh\nindex d202aaed06..7a0bb29448 100755\n--- a/t/perf/p3400-rebase.sh\n+++ b/t/perf/p3400-rebase.sh\n@@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '\n \tgit checkout -f -B base &&\n \tgit checkout -B to-rebase &&\n \tgit checkout -B upstream &&\n-\tfor i in $(seq 100)\n+\tfor i in $(test_seq 100)\n \tdo\n \t\t# simulate huge diffs\n \t\techo change$i >unrelated-file$i &&\n-\t\tseq 1000 >>unrelated-file$i &&\n+\t\ttest_seq 1000 >>unrelated-file$i &&\n \t\tgit add unrelated-file$i &&\n \t\ttest_tick &&\n \t\tgit commit -m commit$i unrelated-file$i &&\n \t\techo change$i >unrelated-file$i &&\n-\t\tseq 1000 | tac >>unrelated-file$i &&\n+\t\ttest_seq 1000 | tac >>unrelated-file$i &&\n \t\tgit add unrelated-file$i &&\n \t\ttest_tick &&\n \t\tgit commit -m commit$i-reverse unrelated-file$i ||\n-- \ngitgitgadget\n\n"},{"id":"407988","messageId":"b3ad8faac43f7e499c794b4a9c106f9fcc121281.1603201265.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v4.git.1603201264.gitgitgadget@gmail.com","subject":"[PATCH v4 4/7] t/perf: add fsmonitor perf test for git diff","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-20T13:41:01Z","receivedAt":"2020-10-20T13:41:29Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nResults for the git-diff fsmonitor optimization\nin patch in the parent-rev (using a 400k file repo to test)\n\nAs you can see here - git diff with fsmonitor running is\nsignificantly better with this patch series (80% faster on my\nworkload)!\n\nGIT_PERF_LARGE_REPO=~/src/server ./run v2.29.0-rc1 . -- p7519-fsmonitor.sh\n\nTest v2.29.0-rc1 this tree\n-----------------------------------------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 1.46(0.82+0.64) 1.47(0.83+0.62) +0.7%\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.16(0.12+0.04) 0.17(0.12+0.05) +6.3%\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.73+0.62) 1.37(0.76+0.60) +0.7%\n7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.85(0.22+0.63) 0.14(0.10+0.05) -83.5%\n7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.05) 0.13(0.11+0.02) +8.3%\n7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.04) 0.13(0.09+0.04) +8.3%\n7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.07+0.05) 0.13(0.07+0.06) +8.3%\n7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.09+0.04) 0.13(0.08+0.05) +8.3%\n7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.14(0.09+0.05) 0.13(0.10+0.03) -7.1%\n7519.12: status (fsmonitor=) 1.67(0.93+1.49) 1.67(0.99+1.42) +0.0%\n7519.13: status -uno (fsmonitor=) 0.37(0.30+0.82) 0.37(0.33+0.79) +0.0%\n7519.14: status -uall (fsmonitor=) 1.58(0.97+1.35) 1.57(0.86+1.45) -0.6%\n7519.15: diff (fsmonitor=) 0.34(0.28+0.83) 0.34(0.27+0.83) +0.0%\n7519.16: diff -- 0_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.08+0.02) +0.0%\n7519.17: diff -- 10_files (fsmonitor=) 0.09(0.07+0.03) 0.09(0.06+0.05) +0.0%\n7519.18: diff -- 100_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.06+0.04) +0.0%\n7519.19: diff -- 1000_files (fsmonitor=) 0.09(0.06+0.04) 0.09(0.05+0.05) +0.0%\n7519.20: diff -- 10000_files (fsmonitor=) 0.10(0.08+0.04) 0.10(0.06+0.05) +0.0%\n\nI also added a benchmark for a tiny git diff workload w/ a pathspec.\nI see an approximately .02 second overhead added w/ and w/o fsmonitor\n\nFrom looking at these results, I suspected that refresh_fsmonitor\nis already happening during git diff - independent of this patch\nseries' optimization. Confirmed that suspicion by breaking on\nrefresh_fsmonitor.\n\n(gdb) bt [simplified]\n0 refresh_fsmonitor at fsmonitor.c:176\n1 ie_match_stat at read-cache.c:375\n2 match_stat_with_submodule at diff-lib.c:237\n4 builtin_diff_files at builtin/diff.c:260\n5 cmd_diff at builtin/diff.c:541\n6 run_builtin at git.c:450\n7 handle_builtin at git.c:700\n8 run_argv at git.c:767\n9 cmd_main at git.c:898\n10 main at common-main.c:52\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 71 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 71 insertions(+)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex 9313d4a51d..ef4c3c8c5c 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -115,6 +115,13 @@ test_expect_success \"setup for fsmonitor\" '\n \n \tgit config core.fsmonitor \"$INTEGRATION_SCRIPT\" &&\n \tgit update-index --fsmonitor &&\n+\tmkdir 1_file 10_files 100_files 1000_files 10000_files &&\n+\tfor i in $(test_seq 1 10); do touch 10_files/$i; done &&\n+\tfor i in $(test_seq 1 100); do touch 100_files/$i; done &&\n+\tfor i in $(test_seq 1 1000); do touch 1000_files/$i; done &&\n+\tfor i in $(test_seq 1 10000); do touch 10000_files/$i; done &&\n+\tgit add 1_file 10_files 100_files 1000_files 10000_files &&\n+\tgit commit -m \"Add files\" &&\n \tgit status # Warm caches\n '\n \n@@ -142,6 +149,38 @@ test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n \tgit status -uall\n '\n \n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff\n+'\n+\n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1_file\n+'\n+\n+test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10_files\n+'\n+\n+test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 100_files\n+'\n+\n+test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1000_files\n+'\n+\n+test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10000_files\n+'\n+\n test_expect_success \"setup without fsmonitor\" '\n \tunset INTEGRATION_SCRIPT &&\n \tgit config --unset core.fsmonitor &&\n@@ -172,6 +211,38 @@ test_perf \"status -uall (fsmonitor=$INTEGRATION_SCRIPT)\" '\n \tgit status -uall\n '\n \n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff\n+'\n+\n+if test -n \"$GIT_PERF_7519_DROP_CACHE\"; then\n+\ttest-tool drop-caches\n+fi\n+\n+test_perf \"diff -- 0_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1_file\n+'\n+\n+test_perf \"diff -- 10_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10_files\n+'\n+\n+test_perf \"diff -- 100_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 100_files\n+'\n+\n+test_perf \"diff -- 1000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 1000_files\n+'\n+\n+test_perf \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\tgit diff -- 10000_files\n+'\n+\n if test_have_prereq WATCHMAN\n then\n \twatchman watch-del \"$GIT_WORK_TREE\" >/dev/null 2>&1 &&\n-- \ngitgitgadget\n\n"},{"id":"407989","messageId":"6f974399360ef38059dea65b4ffa5a17c615ade1.1603201265.git.gitgitgadget@gmail.com","threadId":"54452","inReplyTo":"pull.756.v4.git.1603201264.gitgitgadget@gmail.com","subject":"[PATCH v4 7/7] p7519-fsmonitor: add a git add benchmark","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-20T13:41:04Z","receivedAt":"2020-10-20T13:41:30Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nTest v2.29.0-rc1 this tree\n-----------------------------------------------------------------------------------------------------------------\n7519.2: status (fsmonitor=.git/hooks/fsmonitor-watchman) 1.48(0.79+0.67) 1.48(0.79+0.67) +0.0%\n7519.3: status -uno (fsmonitor=.git/hooks/fsmonitor-watchman) 0.16(0.11+0.05) 0.17(0.13+0.04) +6.3%\n7519.4: status -uall (fsmonitor=.git/hooks/fsmonitor-watchman) 1.36(0.77+0.58) 1.37(0.72+0.63) +0.7%\n7519.5: diff (fsmonitor=.git/hooks/fsmonitor-watchman) 0.84(0.21+0.63) 0.14(0.11+0.03) -83.3%\n7519.6: diff -- 0_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.07+0.05) 0.13(0.09+0.04) +8.3%\n7519.7: diff -- 10_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.09+0.04) 0.13(0.07+0.06) +8.3%\n7519.8: diff -- 100_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.05) 0.12(0.08+0.05) +0.0%\n7519.9: diff -- 1000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.12(0.08+0.05) 0.13(0.09+0.04) +8.3%\n7519.10: diff -- 10000_files (fsmonitor=.git/hooks/fsmonitor-watchman) 0.14(0.08+0.06) 0.13(0.07+0.06) -7.1%\n7519.11: add (fsmonitor=.git/hooks/fsmonitor-watchman) 2.75(1.41+1.27) 2.03(1.26+0.70) -26.2%\n7519.13: status (fsmonitor=) 1.38(1.03+1.04) 1.37(1.04+1.04) -0.7%\n7519.14: status -uno (fsmonitor=) 1.11(0.83+0.98) 1.10(0.89+0.90) -0.9%\n7519.15: status -uall (fsmonitor=) 2.30(1.57+1.42) 2.31(1.49+1.50) +0.4%\n7519.16: diff (fsmonitor=) 1.43(1.13+1.76) 1.46(1.19+1.72) +2.1%\n7519.17: diff -- 0_files (fsmonitor=) 0.10(0.08+0.04) 0.11(0.08+0.04) +10.0%\n7519.18: diff -- 10_files (fsmonitor=) 0.10(0.07+0.05) 0.11(0.08+0.04) +10.0%\n7519.19: diff -- 100_files (fsmonitor=) 0.10(0.07+0.04) 0.11(0.07+0.05) +10.0%\n7519.20: diff -- 1000_files (fsmonitor=) 0.10(0.08+0.03) 0.11(0.08+0.04) +10.0%\n7519.21: diff -- 10000_files (fsmonitor=) 0.11(0.08+0.05) 0.12(0.07+0.06) +9.1%\n7519.22: add (fsmonitor=) 2.26(1.46+1.49) 2.27(1.42+1.55) +0.4%\n\nSigned-off-by: Nipunn Koorapati \n---\n t/perf/p7519-fsmonitor.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex 75a0cef01d..fb20fe0937 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -169,6 +169,10 @@ test_fsmonitor_suite() {\n \ttest_perf_w_drop_caches \"diff -- 10000_files (fsmonitor=$INTEGRATION_SCRIPT)\" '\n \t\tgit diff -- 10000_files\n \t'\n+\n+\ttest_perf_w_drop_caches \"add (fsmonitor=$INTEGRATION_SCRIPT)\" '\n+\t\tgit add --all\n+\t'\n }\n \n test_fsmonitor_suite\n-- \ngitgitgadget\n"},{"id":"408027","messageId":"20201020220629.GF75186@nand.local","threadId":"54452","inReplyTo":"b534cd137a833de802d6d95c1affb8d2d8f7de85.1603201265.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 5/7] perf lint: add make test-lint to perf tests","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-20T22:06:29Z","receivedAt":"2020-10-20T22:06:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 20, 2020 at 01:41:02PM +0000, Nipunn Koorapati via GitGitGadget wrote:\n> diff --git a/t/perf/Makefile b/t/perf/Makefile\n> index 8c47155a7c..fcb0e8865e 100644\n> --- a/t/perf/Makefile\n> +++ b/t/perf/Makefile\n> @@ -1,7 +1,7 @@\n> -include ../../config.mak\n> export GIT_TEST_OPTIONS\n>\n> -all: perf\n> +all: test-lint perf\n>\n> perf: pre-clean\n> \t./run\n> @@ -12,4 +12,7 @@ pre-clean:\n> clean:\n> \trm -rf build \"trash directory\".* test-results\n>\n> +test-lint:\n> +\t$(MAKE) -C .. test-lint\n> +\n\nGreat; it sounds like adding a complete definition here was too much\neffort to be worth it, but that adding a '$(MAKE) -C ..' is just right.\nWe can still run 'make test-lint' from within 't/perf', but there isn't\na bunch of clutter in this series to make that happen. Thanks.\n\n> .PHONY: all perf pre-clean clean\n> diff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh\n> index d202aaed06..7a0bb29448 100755\n> --- a/t/perf/p3400-rebase.sh\n> +++ b/t/perf/p3400-rebase.sh\n> @@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '\n> \tgit checkout -f -B base &&\n> \tgit checkout -B to-rebase &&\n> \tgit checkout -B upstream &&\n> -\tfor i in $(seq 100)\n> +\tfor i in $(test_seq 100)\n> \tdo\n> \t\t# simulate huge diffs\n> \t\techo change$i >unrelated-file$i &&\n> -\t\tseq 1000 >>unrelated-file$i &&\n> +\t\ttest_seq 1000 >>unrelated-file$i &&\n> \t\tgit add unrelated-file$i &&\n> \t\ttest_tick &&\n> \t\tgit commit -m commit$i unrelated-file$i &&\n> \t\techo change$i >unrelated-file$i &&\n> -\t\tseq 1000 | tac >>unrelated-file$i &&\n> +\t\ttest_seq 1000 | tac >>unrelated-file$i &&\n\nThe rest of this all looks good, but I think adding 'tac' here is still\nwrong; this isn't available everywhere, so we would want to find an\nalternative before going further. Is there a reason that you couldn't\nuse a different 'N' in 'test_seq N' here?\n\nThanks,\nTaylor\n"},{"id":"408028","messageId":"CAN8Z4-Uedr-6ThkyWCtVgRSrdTW+N0yeOQTUqFtqAj8QsGBEdQ@mail.gmail.com","threadId":"54452","inReplyTo":"20201020220629.GF75186@nand.local","subject":"Re: [PATCH v4 5/7] perf lint: add make test-lint to perf tests","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-20T22:17:23Z","receivedAt":"2020-10-20T22:17:38Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> > --- a/t/perf/p3400-rebase.sh\n> > +++ b/t/perf/p3400-rebase.sh\n> > @@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '\n> > git checkout -f -B base &&\n> > git checkout -B to-rebase &&\n> > git checkout -B upstream &&\n> > - for i in $(seq 100)\n> > + for i in $(test_seq 100)\n> > do\n> > # simulate huge diffs\n> > echo change$i >unrelated-file$i &&\n> > - seq 1000 >>unrelated-file$i &&\n> > + test_seq 1000 >>unrelated-file$i &&\n> > git add unrelated-file$i &&\n> > test_tick &&\n> > git commit -m commit$i unrelated-file$i &&\n> > echo change$i >unrelated-file$i &&\n> > - seq 1000 | tac >>unrelated-file$i &&\n> > + test_seq 1000 | tac >>unrelated-file$i &&\n>\n> The rest of this all looks good, but I think adding 'tac' here is still\n> wrong; this isn't available everywhere, so we would want to find an\n> alternative before going further. Is there a reason that you couldn't\n> use a different 'N' in 'test_seq N' here?\n\nHey. I think there's some confusion. I didn't add `tac`. It was\nalready here. I didn't even notice it until Junio mentioned it.\n\n--Nipunn\n"},{"id":"408029","messageId":"20201020221936.GA93217@nand.local","threadId":"54452","inReplyTo":"CAN8Z4-Uedr-6ThkyWCtVgRSrdTW+N0yeOQTUqFtqAj8QsGBEdQ@mail.gmail.com","subject":"Re: [PATCH v4 5/7] perf lint: add make test-lint to perf tests","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-20T22:19:36Z","receivedAt":"2020-10-20T22:19:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 20, 2020 at 11:17:23PM +0100, Nipunn Koorapati wrote:\n> > > --- a/t/perf/p3400-rebase.sh\n> > > +++ b/t/perf/p3400-rebase.sh\n> > > @@ -9,16 +9,16 @@ test_expect_success 'setup rebasing on top of a lot of changes' '\n> > > git checkout -f -B base &&\n> > > git checkout -B to-rebase &&\n> > > git checkout -B upstream &&\n> > > - for i in $(seq 100)\n> > > + for i in $(test_seq 100)\n> > > do\n> > > # simulate huge diffs\n> > > echo change$i >unrelated-file$i &&\n> > > - seq 1000 >>unrelated-file$i &&\n> > > + test_seq 1000 >>unrelated-file$i &&\n> > > git add unrelated-file$i &&\n> > > test_tick &&\n> > > git commit -m commit$i unrelated-file$i &&\n> > > echo change$i >unrelated-file$i &&\n> > > - seq 1000 | tac >>unrelated-file$i &&\n> > > + test_seq 1000 | tac >>unrelated-file$i &&\n> >\n> > The rest of this all looks good, but I think adding 'tac' here is still\n> > wrong; this isn't available everywhere, so we would want to find an\n> > alternative before going further. Is there a reason that you couldn't\n> > use a different 'N' in 'test_seq N' here?\n>\n> Hey. I think there's some confusion. I didn't add `tac`. It was\n> already here. I didn't even notice it until Junio mentioned it.\n\nYou're right, sorry; I just saw a line beginning with '+' that contained\n'tac' and thought that it was new in this patch. What you have is OK,\nthen, since it's not a new problem with your patch.\n\nIt couldn't hurt to have the linting phase catch that, but let's leave\nthat for another day, since I think what you have in this version looks\ngood to me.\n\nThanks for listening to all of my feedback :).\n\n> --Nipunn\n\nThanks,\nTaylor\n"}]}