{"thread":{"id":"52189","subject":"[PATCH 1/1] unpack-trees: skip stat on fsmonitor-valid files","startedAt":"2019-11-04T23:41:31Z","lastAt":"2019-11-05T15:29:58Z","messageCount":3,"participants":["Utsav Shah via GitGitGadget","Utsav Shah"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"385438","messageId":"f76ba554ed25fb9877a223ef6481834f1831c8ca.1572910885.git.gitgitgadget@gmail.com","threadId":"52189","inReplyTo":"pull.446.git.1572910885.gitgitgadget@gmail.com","subject":"[PATCH 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-04T23:41:25Z","receivedAt":"2019-11-04T23:41:31Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"From: Utsav Shah <utsav@dropbox.com>\n\nThe index might be aware that a file hasn't modified via fsmonitor, but\nunpack-trees did not pay attention to it and checked via ie_match_stat\nwhich can be inefficient on certain filesystems. This significantly slows\ndown commands that run oneway_merge, like checkout and reset --hard.\n\nThis patch makes oneway_merge check whether a file is considered\nunchanged through fsmonitor and skips ie_match_stat on it. unpack-trees\nalso now correctly copies over fsmonitor validity state from the source\nindex. Finally, for correctness, we force a refresh of fsmonitor state in\ntweak_fsmonitor.\n\nAfter this change, commands like stash (that use reset --hard\ninternally) go from 8s or more to ~2s on a 250k file repository on a\nmac.\n\nSigned-off-by: Utsav Shah <utsav@dropbox.com>\n---\n fsmonitor.c                       | 20 +++++++++++---------\n t/t7113-post-index-change-hook.sh |  3 ---\n t/t7519-status-fsmonitor.sh       |  9 +++++++--\n unpack-trees.c                    |  6 +++++-\n 4 files changed, 23 insertions(+), 15 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 1f4aa1b150..4362bc6ee9 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \t}\n \tistate->fsmonitor_dirty = fsmonitor_dirty;\n \n-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n+\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n+\n \n \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n \treturn 0;\n@@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n \tuint32_t ewah_size = 0;\n \tint fixup = 0;\n \n-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n+\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n \n \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n@@ -189,6 +190,9 @@ void refresh_fsmonitor(struct index_state *istate)\n \t\t}\n \t\tif (bol < query_result.len)\n \t\t\tfsmonitor_refresh_callback(istate, buf + bol);\n+\n+\t\tif (istate->untracked)\n+\t\t\tistate->untracked->use_fsmonitor = 1;\n \t} else {\n \t\t/* Mark all entries invalid */\n \t\tfor (i = 0; i < istate->cache_nr; i++)\n@@ -257,9 +261,7 @@ void tweak_fsmonitor(struct index_state *istate)\n \t\t\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n \n-\t\t\t/* Now mark the untracked cache for fsmonitor usage */\n-\t\t\tif (istate->untracked)\n-\t\t\t\tistate->untracked->use_fsmonitor = 1;\n+\t\t\trefresh_fsmonitor(istate);\n \t\t}\n \n \t\tewah_free(istate->fsmonitor_dirty);\ndiff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-hook.sh\nindex f011ad7eec..5ca2279d0d 100755\n--- a/t/t7113-post-index-change-hook.sh\n+++ b/t/t7113-post-index-change-hook.sh\n@@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others trigger hook without flags\n \tgit checkout -- dir1/file1.txt &&\n \ttest_path_is_file testsuccess && rm -f testsuccess &&\n \ttest_path_is_missing testfailure &&\n-\tgit update-index &&\n-\ttest_path_is_missing testsuccess &&\n-\ttest_path_is_missing testfailure &&\n \tgit reset --soft &&\n \ttest_path_is_missing testsuccess &&\n \ttest_path_is_missing testfailure\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex d8df990972..9cac3d3d8e 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -106,6 +106,8 @@ EOF\n \n # test that \"update-index --fsmonitor-valid\" sets the fsmonitor valid bit\n test_expect_success 'update-index --fsmonitor-valid\" sets the fsmonitor valid bit' '\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit update-index --fsmonitor &&\n \tgit update-index --fsmonitor-valid dir1/modified &&\n \tgit update-index --fsmonitor-valid dir2/modified &&\n@@ -164,6 +166,8 @@ EOF\n \n # test that newly added files are marked valid\n test_expect_success 'newly added files are marked valid' '\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit add new &&\n \tgit add dir1/new &&\n \tgit add dir2/new &&\n@@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged\n # Ensure commands that call refresh_index() to move the index back in time\n # properly invalidate the fsmonitor cache\n test_expect_success 'refresh_index() invalidates fsmonitor cache' '\n-\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n-\tEOF\n \tclean_repo &&\n+\twrite_integration_script &&\n \tdirty_repo &&\n \tgit add . &&\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit commit -m \"to reset\" &&\n \tgit reset HEAD~1 &&\n \tgit status >actual &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 33ea7810d8..fc5ceb932c 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \to->merge_size = len;\n \tmark_all_ce_unused(o->src_index);\n \n+\tif (o->src_index->fsmonitor_last_update)\n+\t\to->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n+\n \t/*\n \t * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n \t */\n@@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,\n \n \tif (old && same(old, a)) {\n \t\tint update = 0;\n-\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n+\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n+\t\t\t!(old->ce_flags & CE_FSMONITOR_VALID)) {\n \t\t\tstruct stat st;\n \t\t\tif (lstat(old->name, &st) ||\n \t\t\t    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n-- \ngitgitgadget\n"},{"id":"385439","messageId":"pull.446.git.1572910885.gitgitgadget@gmail.com","threadId":"52189","inReplyTo":null,"subject":"[PATCH 0/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-04T23:41:24Z","receivedAt":"2019-11-04T23:41:31Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"The index might be aware that a file hasn't modified via fsmonitor, but\nunpack-trees did not pay attention to it and checked via ie_match_stat which\ncan be inefficient on certain filesystems. This significantly slows down\ncommands that run oneway_merge, like checkout and reset --hard.\n\nThis patch makes oneway_merge check whether a file is considered unchanged\nthrough fsmonitor and skips ie_match_stat on it. unpack-trees also now\ncorrectly copies over fsmonitor validity state from the source index.\nFinally, for correctness, we force a refresh of fsmonitor state in\ntweak_fsmonitor.\n\nAfter this change, commands like stash (that use reset --hard internally) go\nfrom 8s or more to ~2s on a 250k file repository on a mac.\n\nSigned-off-by: Utsav Shah utsav@dropbox.com [utsav@dropbox.com]\n\nUtsav Shah (1):\n  unpack-trees: skip stat on fsmonitor-valid files\n\n fsmonitor.c                       | 20 +++++++++++---------\n t/t7113-post-index-change-hook.sh |  3 ---\n t/t7519-status-fsmonitor.sh       |  9 +++++++--\n unpack-trees.c                    |  6 +++++-\n 4 files changed, 23 insertions(+), 15 deletions(-)\n\n\nbase-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-446%2FUtsav2%2Fskip-lstat-2-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-446/Utsav2/skip-lstat-2-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/446\n-- \ngitgitgadget\n"},{"id":"385454","messageId":"CAPYzU3Pk=vwsAre4hydBaqErRuQLZmtUzqM0PCPYwKD1Aw_UWQ@mail.gmail.com","threadId":"52189","inReplyTo":"f76ba554ed25fb9877a223ef6481834f1831c8ca.1572910885.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah","fromEmail":"utsav@dropbox.com","sentAt":"2019-11-05T15:29:40Z","receivedAt":"2019-11-05T15:29:58Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"Sorry for the noise, I've resubmitted this patch on the original\nthread (pull.424.v2.git.1572967644.gitgitgadget@gmail.com).\n\n\nOn Mon, Nov 4, 2019 at 3:41 PM Utsav Shah via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Utsav Shah <utsav@dropbox.com>\n>\n> The index might be aware that a file hasn't modified via fsmonitor, but\n> unpack-trees did not pay attention to it and checked via ie_match_stat\n> which can be inefficient on certain filesystems. This significantly slows\n> down commands that run oneway_merge, like checkout and reset --hard.\n>\n> This patch makes oneway_merge check whether a file is considered\n> unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees\n> also now correctly copies over fsmonitor validity state from the source\n> index. Finally, for correctness, we force a refresh of fsmonitor state in\n> tweak_fsmonitor.\n>\n> After this change, commands like stash (that use reset --hard\n> internally) go from 8s or more to ~2s on a 250k file repository on a\n> mac.\n>\n> Signed-off-by: Utsav Shah <utsav@dropbox.com>\n> ---\n>  fsmonitor.c                       | 20 +++++++++++---------\n>  t/t7113-post-index-change-hook.sh |  3 ---\n>  t/t7519-status-fsmonitor.sh       |  9 +++++++--\n>  unpack-trees.c                    |  6 +++++-\n>  4 files changed, 23 insertions(+), 15 deletions(-)\n>\n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index 1f4aa1b150..4362bc6ee9 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n>         }\n>         istate->fsmonitor_dirty = fsmonitor_dirty;\n>\n> -       if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> -               BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> -                   (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> +       if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> +               BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> +                   (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n> +\n>\n>         trace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n>         return 0;\n> @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n>         uint32_t ewah_size = 0;\n>         int fixup = 0;\n>\n> -       if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> -               BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> -                   (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> +       if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> +               BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> +                   (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n>\n>         put_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n>         strbuf_add(sb, &hdr_version, sizeof(uint32_t));\n> @@ -189,6 +190,9 @@ void refresh_fsmonitor(struct index_state *istate)\n>                 }\n>                 if (bol < query_result.len)\n>                         fsmonitor_refresh_callback(istate, buf + bol);\n> +\n> +               if (istate->untracked)\n> +                       istate->untracked->use_fsmonitor = 1;\n>         } else {\n>                 /* Mark all entries invalid */\n>                 for (i = 0; i < istate->cache_nr; i++)\n> @@ -257,9 +261,7 @@ void tweak_fsmonitor(struct index_state *istate)\n>                                     (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n>                         ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n>\n> -                       /* Now mark the untracked cache for fsmonitor usage */\n> -                       if (istate->untracked)\n> -                               istate->untracked->use_fsmonitor = 1;\n> +                       refresh_fsmonitor(istate);\n>                 }\n>\n>                 ewah_free(istate->fsmonitor_dirty);\n> diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-hook.sh\n> index f011ad7eec..5ca2279d0d 100755\n> --- a/t/t7113-post-index-change-hook.sh\n> +++ b/t/t7113-post-index-change-hook.sh\n> @@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others trigger hook without flags\n>         git checkout -- dir1/file1.txt &&\n>         test_path_is_file testsuccess && rm -f testsuccess &&\n>         test_path_is_missing testfailure &&\n> -       git update-index &&\n> -       test_path_is_missing testsuccess &&\n> -       test_path_is_missing testfailure &&\n>         git reset --soft &&\n>         test_path_is_missing testsuccess &&\n>         test_path_is_missing testfailure\n> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n> index d8df990972..9cac3d3d8e 100755\n> --- a/t/t7519-status-fsmonitor.sh\n> +++ b/t/t7519-status-fsmonitor.sh\n> @@ -106,6 +106,8 @@ EOF\n>\n>  # test that \"update-index --fsmonitor-valid\" sets the fsmonitor valid bit\n>  test_expect_success 'update-index --fsmonitor-valid\" sets the fsmonitor valid bit' '\n> +       write_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> +       EOF\n>         git update-index --fsmonitor &&\n>         git update-index --fsmonitor-valid dir1/modified &&\n>         git update-index --fsmonitor-valid dir2/modified &&\n> @@ -164,6 +166,8 @@ EOF\n>\n>  # test that newly added files are marked valid\n>  test_expect_success 'newly added files are marked valid' '\n> +       write_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> +       EOF\n>         git add new &&\n>         git add dir1/new &&\n>         git add dir2/new &&\n> @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged\n>  # Ensure commands that call refresh_index() to move the index back in time\n>  # properly invalidate the fsmonitor cache\n>  test_expect_success 'refresh_index() invalidates fsmonitor cache' '\n> -       write_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> -       EOF\n>         clean_repo &&\n> +       write_integration_script &&\n>         dirty_repo &&\n>         git add . &&\n> +       write_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> +       EOF\n>         git commit -m \"to reset\" &&\n>         git reset HEAD~1 &&\n>         git status >actual &&\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 33ea7810d8..fc5ceb932c 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>         o->merge_size = len;\n>         mark_all_ce_unused(o->src_index);\n>\n> +       if (o->src_index->fsmonitor_last_update)\n> +               o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n> +\n>         /*\n>          * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n>          */\n> @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,\n>\n>         if (old && same(old, a)) {\n>                 int update = 0;\n> -               if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n> +               if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n> +                       !(old->ce_flags & CE_FSMONITOR_VALID)) {\n>                         struct stat st;\n>                         if (lstat(old->name, &st) ||\n>                             ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n> --\n> gitgitgadget\n"}]}