{"thread":{"id":"47701","subject":"Some rough edges of core.fsmonitor","startedAt":"2018-01-27T00:28:34Z","lastAt":"2018-02-14T08:00:40Z","messageCount":30,"participants":["Ævar Arnfjörð Bjarmason","Duy Nguyen","Nguyễn Thái Ngọc Duy","Thomas Gummerer","Johannes Schindelin","Ben Peart","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"337561","messageId":"87efmcw3fa.fsf@evledraar.gmail.com","threadId":"47701","inReplyTo":null,"subject":"Some rough edges of core.fsmonitor","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-27T00:28:25Z","receivedAt":"2018-01-27T00:28:34Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I just got around to testing this since it landed, for context some\nprevious poking of mine in [1].\n\nIssues / stuff I've noticed:\n\n1) We end up invalidating the untracked cache because stuff in .git/\nchanged. For example:\n\n    01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success\n    01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'\n    01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'\n    01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'\n    01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful\n\nAm I missing something or should we do something like:\n\n    diff --git a/fsmonitor.c b/fsmonitor.c\n    index 0af7c4edba..5067b89bda 100644\n    --- a/fsmonitor.c\n    +++ b/fsmonitor.c\n    @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n\n     static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)\n     {\n    -       int pos = index_name_pos(istate, name, strlen(name));\n    +       int pos;\n    +\n    +       if (!strcmp(name, \".git\") || starts_with(name, \".git/\"))\n    +               return;\n    +\n    +       pos = index_name_pos(istate, name, strlen(name));\n\n            if (pos >= 0) {\n                    struct cache_entry *ce = istate->cache[pos];\n\nWith that patch applied status on a large repo[2] goes from a consistent\n~180-200ms to ~140-150ms, since we're not invalidating some of the UC\nstructure\n\n2) We re-write out the index even though we know nothing changed\n\nWhen you first run with core.fsmonitor it needs to\nmark_fsmonitor_clean() for every path, but is there a reason for why we\nwouldn't supply the equivalent of GIT_OPTIONAL_LOCKS=0 if all paths are\nmarked and we know from the hook that nothing changed? Why write out the\nindex again?\n\n3) A lot of time spend reading the index (or something..)\n\nWhile the hook itself takes ~20ms (and watchman itself 1/4 of that)\nstatus as a whole takes much longer. gprof reveals:\n\n    Each sample counts as 0.01 seconds.\n      %   cumulative   self              self     total\n     time   seconds   seconds    calls  ms/call  ms/call  name\n     15.38      0.02     0.02   221690     0.00     0.00  memihash\n     15.38      0.04     0.02   221689     0.00     0.00  create_from_disk\n      7.69      0.05     0.01  2216897     0.00     0.00  git_bswap32\n      7.69      0.06     0.01   222661     0.00     0.00  ce_path_match\n      7.69      0.07     0.01   221769     0.00     0.00  hashmap_add\n      7.69      0.08     0.01    39941     0.00     0.00  prep_exclude\n      7.69      0.09     0.01    39940     0.00     0.00  strbuf_addch\n      7.69      0.10     0.01        1    10.00    10.00  read_one\n      7.69      0.11     0.01        1    10.00    10.00  refresh_index\n      7.69      0.12     0.01        1    10.00    10.00  tweak_fsmonitor\n      7.69      0.13     0.01                             preload_thread\n\nThe index is 24M in this case, I guess it's unpacking it, but I wonder\nif this couldn't be much faster if we saved away the result of the last\n\"status\" in something that's quick to access, and then if nothing\nchanged we just report that, and no need to re-write the index (or just\nwrite the \"it was clean at this time\" part).\n\n4) core.fsmonitor=false behaves unexpectedly\n\nThe code that reads this variable just treats it as a string, so we do a\nbunch of work for nothing (and nothing warns) if this is set and 'false'\nis executed. Any reason we couldn't do our standard boolean parsing\nhere? You couldn't call your hook 0/1/true/false, but that doesn't seem\nlike a big loss.\n\n1. https://public-inbox.org/git/CACBZZX5a6Op7dH_g9WOFBnejh2zgNK4b34ygxA8daNDqvitFVA@mail.gmail.com/\n2. https://github.com/avar/2015-04-03-1M-git\n"},{"id":"337562","messageId":"CACsJy8BpO0s6facg+zcKC9icijpefkipM326n6xOArjn=ZW6+w@mail.gmail.com","threadId":"47701","inReplyTo":"87efmcw3fa.fsf@evledraar.gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-27T01:36:19Z","receivedAt":"2018-01-27T01:36:55Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 27, 2018 at 7:28 AM, Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> 3) A lot of time spend reading the index (or something..)\n\nI'm resending a patch from my old index-helper series. It should\nmeasure all big time consuming blocks in git. Maybe we should get it\nmerged...\n\n> While the hook itself takes ~20ms (and watchman itself 1/4 of that)\n> status as a whole takes much longer. gprof reveals:\n>\n>     Each sample counts as 0.01 seconds.\n>       %   cumulative   self              self     total\n>      time   seconds   seconds    calls  ms/call  ms/call  name\n>      15.38      0.02     0.02   221690     0.00     0.00  memihash\n\nThis sounds like name-hash to me.\n\n>      15.38      0.04     0.02   221689     0.00     0.00  create_from_disk\n>       7.69      0.05     0.01  2216897     0.00     0.00  git_bswap32\n>       7.69      0.06     0.01   222661     0.00     0.00  ce_path_match\n>       7.69      0.07     0.01   221769     0.00     0.00  hashmap_add\n>       7.69      0.08     0.01    39941     0.00     0.00  prep_exclude\n>       7.69      0.09     0.01    39940     0.00     0.00  strbuf_addch\n>       7.69      0.10     0.01        1    10.00    10.00  read_one\n>       7.69      0.11     0.01        1    10.00    10.00  refresh_index\n>       7.69      0.12     0.01        1    10.00    10.00  tweak_fsmonitor\n>       7.69      0.13     0.01                             preload_thread\n>\n> The index is 24M in this case, I guess it's unpacking it, but I wonder\n> if this couldn't be much faster if we saved away the result of the last\n> \"status\" in something that's quick to access, and then if nothing\n\nNo we could do better, we could cache parsed index content so\neverybody benefits. I demonstrated it with my \"index v254\" patch a\nwhile back:\n\nhttps://public-inbox.org/git/1399980019-8706-1-git-send-email-pclouds@gmail.com/\n\nWith the patch I'm sending soon, we can see how much time reading an\nindex take out of that ~140-150ms (and we probably can cut down index\nread time to like 10-20% when cached).\n\n> changed we just report that, and no need to re-write the index (or just\n> write the \"it was clean at this time\" part).\n\nHmm.. does an index write increase that much time?\n-- \nDuy\n"},{"id":"337563","messageId":"20180127013956.21420-1-pclouds@gmail.com","threadId":"47701","inReplyTo":"CACsJy8BpO0s6facg+zcKC9icijpefkipM326n6xOArjn=ZW6+w@mail.gmail.com","subject":"[PATCH] trace: measure where the time is spent in the index-heavy operations","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-27T01:39:56Z","receivedAt":"2018-01-27T01:40:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"All the known heavy code blocks are measured (except object database\naccess). This should help identify if an optimization is effective or\nnot. An unoptimized git-status would give something like below (92% of\ntime is accounted).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n This was in my old index-helper series. The series was replaced by\n fsmonitor but perhaps some measurements like this still helps.\n\n In my old version I measured packed-refs read time too. But\n packed-refs is mmap'd now, no need to worry about it (or at least its\n initial cost).\n\n diff-lib.c      |  4 ++++\n dir.c           |  2 ++\n name-hash.c     |  3 +++\n preload-index.c |  2 ++\n read-cache.c    | 11 +++++++++++\n 5 files changed, 22 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 8104603a3b..a228e1a219 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -92,6 +92,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \tint diff_unmerged_stage = revs->max_count;\n \tunsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)\n \t\t\t      ? CE_MATCH_RACY_IS_DIRTY : 0);\n+\tuint64_t start = getnanotime();\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n \n@@ -246,6 +247,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t}\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n+\ttrace_performance_since(start, \"diff-files\");\n \treturn 0;\n }\n \n@@ -512,6 +514,7 @@ static int diff_cache(struct rev_info *revs,\n int run_diff_index(struct rev_info *revs, int cached)\n {\n \tstruct object_array_entry *ent;\n+\tuint64_t start = getnanotime();\n \n \tent = revs->pending.objects;\n \tif (diff_cache(revs, &ent->item->oid, ent->name, cached))\n@@ -521,6 +524,7 @@ int run_diff_index(struct rev_info *revs, int cached)\n \tdiffcore_fix_diff_index(&revs->diffopt);\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n+\ttrace_performance_since(start, \"diff-index\");\n \treturn 0;\n }\n \ndiff --git a/dir.c b/dir.c\nindex 7c4b45e30e..4479a02a49 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2248,6 +2248,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t\t   const char *path, int len, const struct pathspec *pathspec)\n {\n \tstruct untracked_cache_dir *untracked;\n+\tuint64_t start = getnanotime();\n \n \tif (has_symlink_leading_path(path, len))\n \t\treturn dir->nr;\n@@ -2286,6 +2287,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t\tdir->nr = i;\n \t}\n \n+\ttrace_performance_since(start, \"read directory %.*s\", len, path);\n \tif (dir->untracked) {\n \t\tstatic struct trace_key trace_untracked_stats = TRACE_KEY_INIT(UNTRACKED_STATS);\n \t\ttrace_printf_key(&trace_untracked_stats,\ndiff --git a/name-hash.c b/name-hash.c\nindex 45c98db0a0..ada66f066a 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -578,6 +578,8 @@ static void threaded_lazy_init_name_hash(\n \n static void lazy_init_name_hash(struct index_state *istate)\n {\n+\tuint64_t start = getnanotime();\n+\n \tif (istate->name_hash_initialized)\n \t\treturn;\n \thashmap_init(&istate->name_hash, cache_entry_cmp, NULL, istate->cache_nr);\n@@ -600,6 +602,7 @@ static void lazy_init_name_hash(struct index_state *istate)\n \t}\n \n \tistate->name_hash_initialized = 1;\n+\ttrace_performance_since(start, \"initialize name hash\");\n }\n \n /*\ndiff --git a/preload-index.c b/preload-index.c\nindex 2a83255e4e..4d08d44874 100644\n--- a/preload-index.c\n+++ b/preload-index.c\n@@ -78,6 +78,7 @@ static void preload_index(struct index_state *index,\n {\n \tint threads, i, work, offset;\n \tstruct thread_data data[MAX_PARALLEL];\n+\tuint64_t start = getnanotime();\n \n \tif (!core_preload_index)\n \t\treturn;\n@@ -108,6 +109,7 @@ static void preload_index(struct index_state *index,\n \t\tif (pthread_join(p->pthread, NULL))\n \t\t\tdie(\"unable to join threaded lstat\");\n \t}\n+\ttrace_performance_since(start, \"preload index\");\n }\n #endif\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 2eb81a66b9..1f00aee6a2 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1372,6 +1372,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \tconst char *typechange_fmt;\n \tconst char *added_fmt;\n \tconst char *unmerged_fmt;\n+\tuint64_t start = getnanotime();\n \n \tmodified_fmt = (in_porcelain ? \"M\\t%s\\n\" : \"%s: needs update\\n\");\n \tdeleted_fmt = (in_porcelain ? \"D\\t%s\\n\" : \"%s: needs update\\n\");\n@@ -1442,6 +1443,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \n \t\treplace_index_entry(istate, i, new);\n \t}\n+\ttrace_performance_since(start, \"refresh index\");\n \treturn has_errors;\n }\n \n@@ -1877,12 +1879,15 @@ int read_index_from(struct index_state *istate, const char *path)\n \tint ret;\n \tchar *base_sha1_hex;\n \tconst char *base_path;\n+\tuint64_t start;\n \n \t/* istate->initialized covers both .git/index and .git/sharedindex.xxx */\n \tif (istate->initialized)\n \t\treturn istate->cache_nr;\n \n+\tstart = getnanotime();\n \tret = do_read_index(istate, path, 0);\n+\ttrace_performance_since(start, \"read cache %s\", path);\n \n \tsplit_index = istate->split_index;\n \tif (!split_index || is_null_sha1(split_index->base_sha1)) {\n@@ -1897,6 +1902,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \n \tbase_sha1_hex = sha1_to_hex(split_index->base_sha1);\n \tbase_path = git_path(\"sharedindex.%s\", base_sha1_hex);\n+\tstart = getnanotime();\n \tret = do_read_index(split_index->base, base_path, 1);\n \tif (hashcmp(split_index->base_sha1, split_index->base->sha1))\n \t\tdie(\"broken index, expect %s in %s, got %s\",\n@@ -1906,6 +1912,9 @@ int read_index_from(struct index_state *istate, const char *path)\n \tfreshen_shared_index(base_sha1_hex, 0);\n \tmerge_base_index(istate);\n \tpost_read_index_from(istate);\n+\ttrace_performance_since(start, \"read cache %s\",\n+\t\t\t\tgit_path(\"sharedindex.%s\",\n+\t\t\t\t\t sha1_to_hex(split_index->base_sha1)));\n \treturn ret;\n }\n \n@@ -2244,6 +2253,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \tstruct ondisk_cache_entry_extended ondisk;\n \tstruct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n \tint drop_cache_tree = 0;\n+\tuint64_t start = getnanotime();\n \n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n@@ -2374,6 +2384,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\treturn -1;\n \tistate->timestamp.sec = (unsigned int)st.st_mtime;\n \tistate->timestamp.nsec = ST_MTIME_NSEC(st);\n+\ttrace_performance_since(start, \"write index, changed mask = %x\", istate->cache_changed);\n \treturn 0;\n }\n \n-- \n2.16.1.205.g271f633410\n\n"},{"id":"337567","messageId":"87bmhfwmqa.fsf@evledraar.gmail.com","threadId":"47701","inReplyTo":"CACsJy8BpO0s6facg+zcKC9icijpefkipM326n6xOArjn=ZW6+w@mail.gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-27T11:43:41Z","receivedAt":"2018-01-27T11:43:51Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jan 27 2018, Duy Nguyen jotted:\n\n> On Sat, Jan 27, 2018 at 7:28 AM, Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>> 3) A lot of time spend reading the index (or something..)\n>\n> I'm resending a patch from my old index-helper series. It should\n> measure all big time consuming blocks in git. Maybe we should get it\n> merged...\n>\n>> While the hook itself takes ~20ms (and watchman itself 1/4 of that)\n>> status as a whole takes much longer. gprof reveals:\n>>\n>>     Each sample counts as 0.01 seconds.\n>>       %   cumulative   self              self     total\n>>      time   seconds   seconds    calls  ms/call  ms/call  name\n>>      15.38      0.02     0.02   221690     0.00     0.00  memihash\n>\n> This sounds like name-hash to me.\n>\n>>      15.38      0.04     0.02   221689     0.00     0.00  create_from_disk\n>>       7.69      0.05     0.01  2216897     0.00     0.00  git_bswap32\n>>       7.69      0.06     0.01   222661     0.00     0.00  ce_path_match\n>>       7.69      0.07     0.01   221769     0.00     0.00  hashmap_add\n>>       7.69      0.08     0.01    39941     0.00     0.00  prep_exclude\n>>       7.69      0.09     0.01    39940     0.00     0.00  strbuf_addch\n>>       7.69      0.10     0.01        1    10.00    10.00  read_one\n>>       7.69      0.11     0.01        1    10.00    10.00  refresh_index\n>>       7.69      0.12     0.01        1    10.00    10.00  tweak_fsmonitor\n>>       7.69      0.13     0.01                             preload_thread\n>>\n>> The index is 24M in this case, I guess it's unpacking it, but I wonder\n>> if this couldn't be much faster if we saved away the result of the last\n>> \"status\" in something that's quick to access, and then if nothing\n>\n> No we could do better, we could cache parsed index content so\n> everybody benefits. I demonstrated it with my \"index v254\" patch a\n> while back:\n>\n> https://public-inbox.org/git/1399980019-8706-1-git-send-email-pclouds@gmail.com/\n>\n> With the patch I'm sending soon, we can see how much time reading an\n> index take out of that ~140-150ms (and we probably can cut down index\n> read time to like 10-20% when cached).\n>\n>> changed we just report that, and no need to re-write the index (or just\n>> write the \"it was clean at this time\" part).\n>\n> Hmm.. does an index write increase that much time?\n\nYour patch is very useful. Here's (with gcc -03) some runtimes. This\nalso includes my .git exclusion patch.\n\nThese are all best out of 5, and with the top (until <0.5% time) of\nstrace -c output (run as another invocation, timing not done with\nstrace)::\n\na) no fsmonitor\n\n    $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n    12:32:44.947651 read-cache.c:1890       performance: 0.053153609 s: read cache .git/index\n    12:32:44.967943 preload-index.c:112     performance: 0.020161093 s: preload index\n    12:32:44.974217 read-cache.c:1446       performance: 0.006230611 s: refresh index\n    12:32:44.979083 diff-lib.c:250          performance: 0.004649994 s: diff-files\n    12:32:44.982511 diff-lib.c:527          performance: 0.002918416 s: diff-index\n    12:32:45.037880 dir.c:2290              performance: 0.055331063 s: read directory\n    On branch master\n    Your branch is up to date with 'origin/master'.\n\n    nothing to commit, working tree clean\n    12:32:45.040666 trace.c:417             performance: 0.146724289 s: git command: '/home/aearnfjord/g/git/git-status'\n\n    real    0m0.153s\n    user    0m0.110s\n    sys     0m0.354s\n    % time     seconds  usecs/call     calls    errors syscall\n    ------ ----------- ----------- --------- --------- ----------------\n     59.93    0.031924           1     39978         9 stat\n     35.86    0.019104        6368         3           futex\n      0.84    0.000446          12        36           mprotect\n      0.73    0.000389          13        29           munmap\n      0.66    0.000349           6        62           mmap\n      0.53    0.000285          14        20           clone\n\nb) with fsmonitor\n\n    $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n    12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index\n    12:34:23.838622 preload-index.c:112     performance: 0.001221197 s: preload index\n    12:34:23.858723 fsmonitor.c:170         performance: 0.020059647 s: fsmonitor process '.git/hooks/fsmonitor-watchman'\n    12:34:23.871532 read-cache.c:1446       performance: 0.032870818 s: refresh index\n    12:34:23.876427 diff-lib.c:250          performance: 0.004731427 s: diff-files\n    12:34:23.880669 diff-lib.c:527          performance: 0.003944422 s: diff-index\n    12:34:23.899225 dir.c:2290              performance: 0.018509066 s: read directory\n    On branch master\n    Your branch is up to date with 'origin/master'.\n\n    nothing to commit, working tree clean\n    12:34:23.901914 trace.c:417             performance: 0.118250995 s: git command: '/home/aearnfjord/g/git/git-status'\n\n    real    0m0.125s\n    user    0m0.086s\n    sys     0m0.043s\n    % time     seconds  usecs/call     calls    errors syscall\n    ------ ----------- ----------- --------- --------- ----------------\n     36.61    0.001281          61        21           clone\n     33.84    0.001184          41        29           munmap\n      5.09    0.000178           5        36           mprotect\n      4.34    0.000152           0       619           brk\n      4.17    0.000146           2        62           mmap\n      3.34    0.000117           2        55        20 open\n      3.00    0.000105           2        60        27 lstat\n      1.77    0.000062           2        37         9 stat\n      1.60    0.000056           1        51           read\n      1.57    0.000055           5        12           write\n      1.17    0.000041          41         1           wait4\n      0.83    0.000029           1        41           close\n      0.83    0.000029           1        22           getcwd\n      0.80    0.000028           1        34           fstat\n\nc) with fsmonitor + don't write index\n\n    $ time GIT_TRACE_PERFORMANCE=1 GIT_OPTIONAL_LOCKS=0 ~/g/git/git-status\n    12:36:03.176866 read-cache.c:1890       performance: 0.048292088 s: read cache .git/index\n    12:36:03.181006 preload-index.c:112     performance: 0.001343593 s: preload index\n    12:36:03.200970 fsmonitor.c:170         performance: 0.019936338 s: fsmonitor process '.git/hooks/fsmonitor-watchman'\n    12:36:03.210556 read-cache.c:1446       performance: 0.029525531 s: refresh index\n    12:36:03.213366 diff-lib.c:250          performance: 0.002718730 s: diff-files\n    12:36:03.216273 diff-lib.c:527          performance: 0.002666604 s: diff-index\n    12:36:03.233579 dir.c:2290              performance: 0.017261702 s: read directory\n    On branch master\n    Your branch is up to date with 'origin/master'.\n\n    nothing to commit, working tree clean\n    12:36:03.233733 trace.c:417             performance: 0.105632105 s: git command: '/home/aearnfjord/g/git/git-status'\n\n    real    0m0.111s\n    user    0m0.073s\n    sys     0m0.044s\n    % time     seconds  usecs/call     calls    errors syscall\n    ------ ----------- ----------- --------- --------- ----------------\n     24.42    0.001081          37        29           munmap\n     20.74    0.000918          44        21           clone\n      7.63    0.000338           5        62           mmap\n      7.43    0.000329           6        54        20 open\n      6.05    0.000268           7        36           mprotect\n      5.99    0.000265           0       619           brk\n      4.99    0.000221           4        60        27 lstat\n      4.13    0.000183           4        51           read\n      3.68    0.000163           9        19        10 access\n      3.25    0.000144           4        34           fstat\n      2.78    0.000123           3        40           close\n      2.48    0.000110           3        37         9 stat\n      2.24    0.000099           5        21           getcwd\n      1.15    0.000051           4        12           write\n      0.99    0.000044          22         2           poll\n\nFor comparison just the output from the hook:\n\n    $ time ('.git/hooks/fsmonitor-watchman' '1' '1517052856494406191')\n    .git\n    real    0m0.017s\n    user    0m0.011s\n    sys     0m0.003s\n\nAnd this is what goes on behind the scenes after we get rid of the\n.git/hooks/fsmonitor-watchman overhead:\n    \n    $ time (echo '[\"query\", \"/home/aearnfjord/git_tree/2015-04-03-1M-git\", {\n                            \"since\": 1517052856,\n                            \"fields\": [\"name\"],\n                            \"expression\": [\"not\", [\"allof\", [\"since\", 1517052856, \"cclock\"], [\"not\", \"exists\"]]]\n                    }]' | watchman -j)\n    {\n        \"version\": \"4.9.0\",\n        \"is_fresh_instance\": false,\n        \"clock\": \"c:1517006351:31165:4:1113968\",\n        \"files\": [\n            \".git\"\n        ]\n    }\n    \n    real    0m0.003s\n    user    0m0.000s\n    sys     0m0.004s\n"},{"id":"337568","messageId":"20180127115811.GB2130@hank","threadId":"47701","inReplyTo":"20180127013956.21420-1-pclouds@gmail.com","subject":"Re: [PATCH] trace: measure where the time is spent in the index-heavy operations","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-01-27T11:58:11Z","receivedAt":"2018-01-27T11:55:48Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 01/27, Nguyễn Thái Ngọc Duy wrote:\n> All the known heavy code blocks are measured (except object database\n> access). This should help identify if an optimization is effective or\n> not. An unoptimized git-status would give something like below (92% of\n> time is accounted).\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  This was in my old index-helper series. The series was replaced by\n>  fsmonitor but perhaps some measurements like this still helps.\n> \n>  In my old version I measured packed-refs read time too. But\n>  packed-refs is mmap'd now, no need to worry about it (or at least its\n>  initial cost).\n> \n>  diff-lib.c      |  4 ++++\n>  dir.c           |  2 ++\n>  name-hash.c     |  3 +++\n>  preload-index.c |  2 ++\n>  read-cache.c    | 11 +++++++++++\n>  5 files changed, 22 insertions(+)\n>\n> [...]\n>  \n> diff --git a/read-cache.c b/read-cache.c\n> index 2eb81a66b9..1f00aee6a2 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1372,6 +1372,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n>  \tconst char *typechange_fmt;\n>  \tconst char *added_fmt;\n>  \tconst char *unmerged_fmt;\n> +\tuint64_t start = getnanotime();\n>  \n>  \tmodified_fmt = (in_porcelain ? \"M\\t%s\\n\" : \"%s: needs update\\n\");\n>  \tdeleted_fmt = (in_porcelain ? \"D\\t%s\\n\" : \"%s: needs update\\n\");\n> @@ -1442,6 +1443,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n>  \n>  \t\treplace_index_entry(istate, i, new);\n>  \t}\n> +\ttrace_performance_since(start, \"refresh index\");\n>  \treturn has_errors;\n>  }\n>  \n> @@ -1877,12 +1879,15 @@ int read_index_from(struct index_state *istate, const char *path)\n>  \tint ret;\n>  \tchar *base_sha1_hex;\n>  \tconst char *base_path;\n> +\tuint64_t start;\n>  \n>  \t/* istate->initialized covers both .git/index and .git/sharedindex.xxx */\n>  \tif (istate->initialized)\n>  \t\treturn istate->cache_nr;\n>  \n> +\tstart = getnanotime();\n>  \tret = do_read_index(istate, path, 0);\n> +\ttrace_performance_since(start, \"read cache %s\", path);\n>  \n>  \tsplit_index = istate->split_index;\n>  \tif (!split_index || is_null_sha1(split_index->base_sha1)) {\n> @@ -1897,6 +1902,7 @@ int read_index_from(struct index_state *istate, const char *path)\n>  \n>  \tbase_sha1_hex = sha1_to_hex(split_index->base_sha1);\n>  \tbase_path = git_path(\"sharedindex.%s\", base_sha1_hex);\n> +\tstart = getnanotime();\n>  \tret = do_read_index(split_index->base, base_path, 1);\n>  \tif (hashcmp(split_index->base_sha1, split_index->base->sha1))\n>  \t\tdie(\"broken index, expect %s in %s, got %s\",\n> @@ -1906,6 +1912,9 @@ int read_index_from(struct index_state *istate, const char *path)\n>  \tfreshen_shared_index(base_sha1_hex, 0);\n>  \tmerge_base_index(istate);\n>  \tpost_read_index_from(istate);\n> +\ttrace_performance_since(start, \"read cache %s\",\n> +\t\t\t\tgit_path(\"sharedindex.%s\",\n> +\t\t\t\t\t sha1_to_hex(split_index->base_sha1)));\n\nWould it be worth doing this on top of tg/split-index-fixes?  OTOH\nthis will only give a wrong output when tracing performance is on, and\nit should be easy enough to figure out where the sharedindex actually\nis.  So it might be better to keep this separate, and then just add a\npatch on top for fixing the path later, which might be less work for\nJunio.\n\nSo dunno what the best way is, just wanted to mention it.\n\n>  \treturn ret;\n>  }\n>  \n> @@ -2244,6 +2253,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>  \tstruct ondisk_cache_entry_extended ondisk;\n>  \tstruct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n>  \tint drop_cache_tree = 0;\n> +\tuint64_t start = getnanotime();\n>  \n>  \tfor (i = removed = extended = 0; i < entries; i++) {\n>  \t\tif (cache[i]->ce_flags & CE_REMOVE)\n> @@ -2374,6 +2384,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>  \t\treturn -1;\n>  \tistate->timestamp.sec = (unsigned int)st.st_mtime;\n>  \tistate->timestamp.nsec = ST_MTIME_NSEC(st);\n> +\ttrace_performance_since(start, \"write index, changed mask = %x\", istate->cache_changed);\n>  \treturn 0;\n>  }\n>  \n> -- \n> 2.16.1.205.g271f633410\n> \n"},{"id":"337570","messageId":"20180127122756.16971-1-pclouds@gmail.com","threadId":"47701","inReplyTo":"20180127115811.GB2130@hank","subject":"[PATCH v2] trace: measure where the time is spent in the index-heavy operations","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-27T12:27:56Z","receivedAt":"2018-01-27T12:28:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"All the known heavy code blocks are measured (except object database\naccess). This should help identify if an optimization is effective or\nnot. An unoptimized git-status would give something like below:\n\n    0.001791141 s: read cache ...\n    0.004011363 s: preload index\n    0.000516161 s: refresh index\n    0.003139257 s: git command: ... 'status' '--porcelain=2'\n    0.006788129 s: diff-files\n    0.002090267 s: diff-index\n    0.001885735 s: initialize name hash\n    0.032013138 s: read directory\n    0.051781209 s: git command: './git' 'status'\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n > Would it be worth doing this on top of tg/split-index-fixes?  OTOH\n > this will only give a wrong output when tracing performance is on, and\n > it should be easy enough to figure out where the sharedindex actually\n > is.  So it might be better to keep this separate, and then just add a\n > patch on top for fixing the path later, which might be less work for\n > Junio.\n\n I updated the patch a bit to avoid git_path(). A merge on 'pu' still\n conflicts, but it's much easier to resolve by making sure free() is\n called after the trace_performance_since() line in read_index_from().\n\n It's technically dangerous to re-use base_path again this way, too\n far away from its assignment since 4 other git_path() calls may have\n been done and changed base_path value. But since tg/split-index-fixes\n should enter 'master' eventually and make it safe to re-use\n base_path, I think it's ok.\n\n diff-lib.c      | 4 ++++\n dir.c           | 2 ++\n name-hash.c     | 3 +++\n preload-index.c | 2 ++\n read-cache.c    | 7 +++++++\n 5 files changed, 18 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 8104603a3b..a228e1a219 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -92,6 +92,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \tint diff_unmerged_stage = revs->max_count;\n \tunsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)\n \t\t\t      ? CE_MATCH_RACY_IS_DIRTY : 0);\n+\tuint64_t start = getnanotime();\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n \n@@ -246,6 +247,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t}\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n+\ttrace_performance_since(start, \"diff-files\");\n \treturn 0;\n }\n \n@@ -512,6 +514,7 @@ static int diff_cache(struct rev_info *revs,\n int run_diff_index(struct rev_info *revs, int cached)\n {\n \tstruct object_array_entry *ent;\n+\tuint64_t start = getnanotime();\n \n \tent = revs->pending.objects;\n \tif (diff_cache(revs, &ent->item->oid, ent->name, cached))\n@@ -521,6 +524,7 @@ int run_diff_index(struct rev_info *revs, int cached)\n \tdiffcore_fix_diff_index(&revs->diffopt);\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n+\ttrace_performance_since(start, \"diff-index\");\n \treturn 0;\n }\n \ndiff --git a/dir.c b/dir.c\nindex 7c4b45e30e..4479a02a49 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2248,6 +2248,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t\t   const char *path, int len, const struct pathspec *pathspec)\n {\n \tstruct untracked_cache_dir *untracked;\n+\tuint64_t start = getnanotime();\n \n \tif (has_symlink_leading_path(path, len))\n \t\treturn dir->nr;\n@@ -2286,6 +2287,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t\tdir->nr = i;\n \t}\n \n+\ttrace_performance_since(start, \"read directory %.*s\", len, path);\n \tif (dir->untracked) {\n \t\tstatic struct trace_key trace_untracked_stats = TRACE_KEY_INIT(UNTRACKED_STATS);\n \t\ttrace_printf_key(&trace_untracked_stats,\ndiff --git a/name-hash.c b/name-hash.c\nindex 45c98db0a0..ada66f066a 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -578,6 +578,8 @@ static void threaded_lazy_init_name_hash(\n \n static void lazy_init_name_hash(struct index_state *istate)\n {\n+\tuint64_t start = getnanotime();\n+\n \tif (istate->name_hash_initialized)\n \t\treturn;\n \thashmap_init(&istate->name_hash, cache_entry_cmp, NULL, istate->cache_nr);\n@@ -600,6 +602,7 @@ static void lazy_init_name_hash(struct index_state *istate)\n \t}\n \n \tistate->name_hash_initialized = 1;\n+\ttrace_performance_since(start, \"initialize name hash\");\n }\n \n /*\ndiff --git a/preload-index.c b/preload-index.c\nindex 2a83255e4e..4d08d44874 100644\n--- a/preload-index.c\n+++ b/preload-index.c\n@@ -78,6 +78,7 @@ static void preload_index(struct index_state *index,\n {\n \tint threads, i, work, offset;\n \tstruct thread_data data[MAX_PARALLEL];\n+\tuint64_t start = getnanotime();\n \n \tif (!core_preload_index)\n \t\treturn;\n@@ -108,6 +109,7 @@ static void preload_index(struct index_state *index,\n \t\tif (pthread_join(p->pthread, NULL))\n \t\t\tdie(\"unable to join threaded lstat\");\n \t}\n+\ttrace_performance_since(start, \"preload index\");\n }\n #endif\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 2eb81a66b9..eac74bc9f1 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1372,6 +1372,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \tconst char *typechange_fmt;\n \tconst char *added_fmt;\n \tconst char *unmerged_fmt;\n+\tuint64_t start = getnanotime();\n \n \tmodified_fmt = (in_porcelain ? \"M\\t%s\\n\" : \"%s: needs update\\n\");\n \tdeleted_fmt = (in_porcelain ? \"D\\t%s\\n\" : \"%s: needs update\\n\");\n@@ -1442,6 +1443,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \n \t\treplace_index_entry(istate, i, new);\n \t}\n+\ttrace_performance_since(start, \"refresh index\");\n \treturn has_errors;\n }\n \n@@ -1873,6 +1875,7 @@ static void freshen_shared_index(char *base_sha1_hex, int warn)\n \n int read_index_from(struct index_state *istate, const char *path)\n {\n+\tuint64_t start = getnanotime();\n \tstruct split_index *split_index;\n \tint ret;\n \tchar *base_sha1_hex;\n@@ -1883,6 +1886,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\treturn istate->cache_nr;\n \n \tret = do_read_index(istate, path, 0);\n+\ttrace_performance_since(start, \"read cache %s\", path);\n \n \tsplit_index = istate->split_index;\n \tif (!split_index || is_null_sha1(split_index->base_sha1)) {\n@@ -1906,6 +1910,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tfreshen_shared_index(base_sha1_hex, 0);\n \tmerge_base_index(istate);\n \tpost_read_index_from(istate);\n+\ttrace_performance_since(start, \"read cache %s\", base_path);\n \treturn ret;\n }\n \n@@ -2234,6 +2239,7 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile\n static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\t  int strip_extensions)\n {\n+\tuint64_t start = getnanotime();\n \tint newfd = tempfile->fd;\n \tgit_SHA_CTX c;\n \tstruct cache_header hdr;\n@@ -2374,6 +2380,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\treturn -1;\n \tistate->timestamp.sec = (unsigned int)st.st_mtime;\n \tistate->timestamp.nsec = ST_MTIME_NSEC(st);\n+\ttrace_performance_since(start, \"write index, changed mask = %x\", istate->cache_changed);\n \treturn 0;\n }\n \n-- \n2.16.1.205.g271f633410\n\n"},{"id":"337571","messageId":"CACsJy8CJtW3LZ+4Z_06uM4rJO88FXsNvcw+zzVqdFpsQUKrvrg@mail.gmail.com","threadId":"47701","inReplyTo":"87bmhfwmqa.fsf@evledraar.gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-27T12:39:27Z","receivedAt":"2018-01-27T12:40:03Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 27, 2018 at 6:43 PM, Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> a) no fsmonitor\n>\n>     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n>     12:32:44.947651 read-cache.c:1890       performance: 0.053153609 s: read cache .git/index\n>     12:32:44.967943 preload-index.c:112     performance: 0.020161093 s: preload index\n>     12:32:44.974217 read-cache.c:1446       performance: 0.006230611 s: refresh index\n>\n> ...\n>\n> b) with fsmonitor\n>\n>     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n>     12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index\n>     12:34:23.838622 preload-index.c:112     performance: 0.001221197 s: preload index\n>     12:34:23.858723 fsmonitor.c:170         performance: 0.020059647 s: fsmonitor process '.git/hooks/fsmonitor-watchman'\n>     12:34:23.871532 read-cache.c:1446       performance: 0.032870818 s: refresh index\n\nHmm.. why does refresh take longer with fsmonitor/watchman? With the\nhelp from watchman, we know what files are modified. We don't need\nmanual stat()'ing and this line should be lower than the \"no\nfsmonitor\" case, which is 0.006230611s.\n\n>     12:34:23.876427 diff-lib.c:250          performance: 0.004731427 s: diff-files\n>     12:34:23.880669 diff-lib.c:527          performance: 0.003944422 s: diff-index\n>     12:34:23.899225 dir.c:2290              performance: 0.018509066 s: read directory\n>     12:34:23.901914 trace.c:417             performance: 0.118250995 s: git command: '/home/aearnfjord/g/git/git-status'\n\nI don't see any \"write index\" line here, which is interesting since\nyour case c) is about \"don't write index\".\n\n> c) with fsmonitor + don't write index\n-- \nDuy\n"},{"id":"337573","messageId":"20180127130954.GA19922@ash","threadId":"47701","inReplyTo":"CACsJy8CJtW3LZ+4Z_06uM4rJO88FXsNvcw+zzVqdFpsQUKrvrg@mail.gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-27T13:09:54Z","receivedAt":"2018-01-27T13:10:06Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 27, 2018 at 07:39:27PM +0700, Duy Nguyen wrote:\n> On Sat, Jan 27, 2018 at 6:43 PM, Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n> > a) no fsmonitor\n> >\n> >     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n> >     12:32:44.947651 read-cache.c:1890       performance: 0.053153609 s: read cache .git/index\n> >     12:32:44.967943 preload-index.c:112     performance: 0.020161093 s: preload index\n> >     12:32:44.974217 read-cache.c:1446       performance: 0.006230611 s: refresh index\n> >\n> > ...\n> >\n> > b) with fsmonitor\n> >\n> >     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n> >     12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index\n> >     12:34:23.838622 preload-index.c:112     performance: 0.001221197 s: preload index\n> >     12:34:23.858723 fsmonitor.c:170         performance: 0.020059647 s: fsmonitor process '.git/hooks/fsmonitor-watchman'\n> >     12:34:23.871532 read-cache.c:1446       performance: 0.032870818 s: refresh index\n> \n> Hmm.. why does refresh take longer with fsmonitor/watchman? With the\n> help from watchman, we know what files are modified. We don't need\n> manual stat()'ing and this line should be lower than the \"no\n> fsmonitor\" case, which is 0.006230611s.\n\nAhh.. my patch probably does not see that fsmonitor could be activated\nlazily inside refresh_index() call. The patch below should fix it.\n\nBut between your normal refresh time (0.020 preload + 0.006 actual\nrefresh) and fsmonitor taking 0.020 just to talk to watchman, this\nrepo seems \"too small\" for fsmonitor/watchman to shine.\n\nI'm still a bit curious that refresh index time, after excluding 0.020\nfor fsmonitor, is stil 0.012s. What does it do? It should really be\ndoing nothing. Either way, read index time seems to be the elephant in\nthe room now.\n\n-- 8< --\ndiff --git a/read-cache.c b/read-cache.c\nindex eac74bc9f1..d60e0a8480 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1367,12 +1367,21 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \tunsigned int options = (CE_MATCH_REFRESH |\n \t\t\t\t(really ? CE_MATCH_IGNORE_VALID : 0) |\n \t\t\t\t(not_new ? CE_MATCH_IGNORE_MISSING : 0));\n+\tint ignore_fsmonitor = options & CE_MATCH_IGNORE_FSMONITOR;\n \tconst char *modified_fmt;\n \tconst char *deleted_fmt;\n \tconst char *typechange_fmt;\n \tconst char *added_fmt;\n \tconst char *unmerged_fmt;\n-\tuint64_t start = getnanotime();\n+\tuint64_t start;\n+\n+\t/*\n+\t * If fsmonitor is used, force its communication early to\n+\t * accurately measure how long this function takes without it.\n+\t */\n+\tif (!ignore_fsmonitor)\n+\t\trefresh_fsmonitor(istate);\n+\tstart = getnanotime();\n \n \tmodified_fmt = (in_porcelain ? \"M\\t%s\\n\" : \"%s: needs update\\n\");\n \tdeleted_fmt = (in_porcelain ? \"D\\t%s\\n\" : \"%s: needs update\\n\");\n-- 8< --\n"},{"id":"337578","messageId":"878tcjw2gd.fsf@evledraar.gmail.com","threadId":"47701","inReplyTo":"20180127130954.GA19922@ash","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-27T19:01:38Z","receivedAt":"2018-01-27T19:01:47Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jan 27 2018, Duy Nguyen jotted:\n\n> On Sat, Jan 27, 2018 at 07:39:27PM +0700, Duy Nguyen wrote:\n>> On Sat, Jan 27, 2018 at 6:43 PM, Ævar Arnfjörð Bjarmason\n>> <avarab@gmail.com> wrote:\n>> > a) no fsmonitor\n>> >\n>> >     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n>> >     12:32:44.947651 read-cache.c:1890       performance: 0.053153609 s: read cache .git/index\n>> >     12:32:44.967943 preload-index.c:112     performance: 0.020161093 s: preload index\n>> >     12:32:44.974217 read-cache.c:1446       performance: 0.006230611 s: refresh index\n>> >\n>> > ...\n>> >\n>> > b) with fsmonitor\n>> >\n>> >     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n>> >     12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index\n>> >     12:34:23.838622 preload-index.c:112     performance: 0.001221197 s: preload index\n>> >     12:34:23.858723 fsmonitor.c:170         performance: 0.020059647 s: fsmonitor process '.git/hooks/fsmonitor-watchman'\n>> >     12:34:23.871532 read-cache.c:1446       performance: 0.032870818 s: refresh index\n>>\n>> Hmm.. why does refresh take longer with fsmonitor/watchman? With the\n>> help from watchman, we know what files are modified. We don't need\n>> manual stat()'ing and this line should be lower than the \"no\n>> fsmonitor\" case, which is 0.006230611s.\n>\n> Ahh.. my patch probably does not see that fsmonitor could be activated\n> lazily inside refresh_index() call. The patch below should fix it.\n\nWill have to get those numbers to you later, or alternatively clone\nhttps://github.com/avar/2015-04-03-1M-git (or some other test repo) and\ntest it yourself, sorry. Don't have time to follow-up much this weekend.\n\n> But between your normal refresh time (0.020 preload + 0.006 actual\n> refresh) and fsmonitor taking 0.020 just to talk to watchman, this\n> repo seems \"too small\" for fsmonitor/watchman to shine.\n\nSurely that's an implementation limitation and not something inherent,\ngiven that watchman itself returns in 5ms?\n\nI.e. status could work like this, no?:\n\n 1. At start, record the timestamp & find out canonical state via some\n    expansive method.\n 2. Print out xyz changed, abc added etc.\n 3. Record *just* what status would report about xyz, abc etc.\n 4. On subsequent git status, just amend that information, e.g. if\n    watchman says nothing changed $(cat .git/last-status-output).\n\nWe shouldn't need to be reading the entire index in the common case\nwhere just a few things change.\n\nThere's also a lot of things that use status to just check \"are we\nclean?\", those would only need to record the last known timestamp when\nthe tree was clean, and then ask watchman if there were any changes, if\nnot we're done.\n\n> I'm still a bit curious that refresh index time, after excluding 0.020\n> for fsmonitor, is stil 0.012s. What does it do? It should really be\n> doing nothing. Either way, read index time seems to be the elephant in\n> the room now.\n>\n> -- 8< --\n> diff --git a/read-cache.c b/read-cache.c\n> index eac74bc9f1..d60e0a8480 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1367,12 +1367,21 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n>  \tunsigned int options = (CE_MATCH_REFRESH |\n>  \t\t\t\t(really ? CE_MATCH_IGNORE_VALID : 0) |\n>  \t\t\t\t(not_new ? CE_MATCH_IGNORE_MISSING : 0));\n> +\tint ignore_fsmonitor = options & CE_MATCH_IGNORE_FSMONITOR;\n>  \tconst char *modified_fmt;\n>  \tconst char *deleted_fmt;\n>  \tconst char *typechange_fmt;\n>  \tconst char *added_fmt;\n>  \tconst char *unmerged_fmt;\n> -\tuint64_t start = getnanotime();\n> +\tuint64_t start;\n> +\n> +\t/*\n> +\t * If fsmonitor is used, force its communication early to\n> +\t * accurately measure how long this function takes without it.\n> +\t */\n> +\tif (!ignore_fsmonitor)\n> +\t\trefresh_fsmonitor(istate);\n> +\tstart = getnanotime();\n>\n>  \tmodified_fmt = (in_porcelain ? \"M\\t%s\\n\" : \"%s: needs update\\n\");\n>  \tdeleted_fmt = (in_porcelain ? \"D\\t%s\\n\" : \"%s: needs update\\n\");\n> -- 8< --\n"},{"id":"337638","messageId":"nycvar.QRO.7.76.6.1801282140330.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"47701","inReplyTo":"87efmcw3fa.fsf@evledraar.gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-01-28T20:44:48Z","receivedAt":"2018-01-28T20:45:00Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 27 Jan 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> I just got around to testing this since it landed, for context some\n> previous poking of mine in [1].\n> \n> Issues / stuff I've noticed:\n> \n> 1) We end up invalidating the untracked cache because stuff in .git/\n> changed. For example:\n> \n>     01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success\n>     01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'\n>     01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'\n>     01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'\n>     01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful\n> \n> Am I missing something or should we do something like:\n> \n>     diff --git a/fsmonitor.c b/fsmonitor.c\n>     index 0af7c4edba..5067b89bda 100644\n>     --- a/fsmonitor.c\n>     +++ b/fsmonitor.c\n>     @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n> \n>      static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)\n>      {\n>     -       int pos = index_name_pos(istate, name, strlen(name));\n>     +       int pos;\n>     +\n>     +       if (!strcmp(name, \".git\") || starts_with(name, \".git/\"))\n>     +               return;\n>     +\n>     +       pos = index_name_pos(istate, name, strlen(name));\n\nI would much rather have the fsmonitor hook already exclude those.\n\nIf you *must* add these comparisons, you have to use fspathcmp() and\nfspathncmp() instead (because case-insensitivity).\n\nCiao,\nDscho"},{"id":"337644","messageId":"CACBZZX74WcK4qX5O1E_6nxv7f4Vpns3O-7dcURbs+QneKsA87Q@mail.gmail.com","threadId":"47701","inReplyTo":"nycvar.QRO.7.76.6.1801282140330.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-28T22:28:48Z","receivedAt":"2018-01-28T22:49:08Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Jan 28, 2018 at 9:44 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi,\n>\n> On Sat, 27 Jan 2018, Ævar Arnfjörð Bjarmason wrote:\n>\n>> I just got around to testing this since it landed, for context some\n>> previous poking of mine in [1].\n>>\n>> Issues / stuff I've noticed:\n>>\n>> 1) We end up invalidating the untracked cache because stuff in .git/\n>> changed. For example:\n>>\n>>     01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success\n>>     01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'\n>>     01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'\n>>     01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'\n>>     01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful\n>>\n>> Am I missing something or should we do something like:\n>>\n>>     diff --git a/fsmonitor.c b/fsmonitor.c\n>>     index 0af7c4edba..5067b89bda 100644\n>>     --- a/fsmonitor.c\n>>     +++ b/fsmonitor.c\n>>     @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n>>\n>>      static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)\n>>      {\n>>     -       int pos = index_name_pos(istate, name, strlen(name));\n>>     +       int pos;\n>>     +\n>>     +       if (!strcmp(name, \".git\") || starts_with(name, \".git/\"))\n>>     +               return;\n>>     +\n>>     +       pos = index_name_pos(istate, name, strlen(name));\n>\n> I would much rather have the fsmonitor hook already exclude those.\n\nAs documented the fsmonitor-watchman hook we ship doesn't work as\ndescribed in githooks(5), unless \"files in the working directory\" is\ntaken to include .git/, but I haven't seen that ever used.\n\nOn the other hand relying on arbitrary user-provided hooks to do the\nright thing at the cost of silent performance degradation is bad. If\nwe're going to expect the hook to remove these we should probably\nwarn/die here if it does send us .git/* files.\n\n> If you *must* add these comparisons, you have to use fspathcmp() and\n> fspathncmp() instead (because case-insensitivity).\n\nThanks.\n"},{"id":"337654","messageId":"20180129094033.GA8670@ash","threadId":"47701","inReplyTo":"87bmhfwmqa.fsf@evledraar.gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-29T09:40:34Z","receivedAt":"2018-01-29T09:40:49Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 27, 2018 at 12:43:41PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> b) with fsmonitor\n> \n>     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n>     12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index\n\nThis is sort of off topic but may be interesting for big repo guys. It\nlooks like read cache's time is partially dominated by malloc().\n\nThis is the performance breakdown of do_read_index()\n\n$ GIT_TRACE_PERFORMANCE=2 ~/w/git/t/helper/test-read-cache\n0.000078489 s: open/mmap/close\n0.038915239 s: main entries\n0.018983150 s: ext TREE\n0.012667080 s: ext UNTR\n0.000005372 s: ext FSMN\n0.001473470 s: munmap\n0.072386911 s: read cache .git/index\n\nReading main index entries takes like half of the time (0.038 vs\n0.072). With the below patch to take out hundred thousands of malloc()\nwe have this, loading main entries now only takes 0.012s:\n\n$ GIT_TRACE_PERFORMANCE=2 ~/w/git/t/helper/test-read-cache\n0.000046587 s: open/mmap/close\n0.012077300 s: main entries\n0.020477683 s: ext TREE\n0.010259998 s: ext UNTR\n0.000010250 s: ext FSMN\n0.000753854 s: munmap\n0.043906473 s: read cache .git/index\n\nWe used to do less malloc until debed2a629 (read-cache.c: allocate\nindex entries individually - 2011-10-24) but I don't think we can\nsimply revert that (not worth the extra complexity of the old way).\n\nNow \"TREE\" and \"UNTR\" extensions become a bigger problem.\n\n-- 8< --\ndiff --git a/read-cache.c b/read-cache.c\nindex d60e0a8480..88f4213c99 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1622,7 +1622,12 @@ static struct cache_entry *cache_entry_from_ondisk(struct ondisk_cache_entry *on\n \t\t\t\t\t\t   const char *name,\n \t\t\t\t\t\t   size_t len)\n {\n+#if 0\n \tstruct cache_entry *ce = xmalloc(cache_entry_size(len));\n+#else\n+\tstatic char buf[1024];\n+\tstruct cache_entry *ce = (struct cache_entry *)buf;\n+#endif\n \n \tce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);\n \tce->ce_stat_data.sd_mtime.sec = get_be32(&ondisk->mtime.sec);\ndiff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c\nindex 48255eef31..e1d21d17a3 100644\n--- a/t/helper/test-read-cache.c\n+++ b/t/helper/test-read-cache.c\n@@ -8,7 +8,9 @@ int cmd_main(int argc, const char **argv)\n \tsetup_git_directory();\n \tfor (i = 0; i < cnt; i++) {\n \t\tread_cache();\n+#if 0\n \t\tdiscard_cache();\n+#endif\n \t}\n \treturn 0;\n }\n-- 8< --\n"},{"id":"337764","messageId":"bb8a433f-bd61-1f3c-2034-1acc96539882@gmail.com","threadId":"47701","inReplyTo":"20180129094033.GA8670@ash","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-01-29T23:16:23Z","receivedAt":"2018-01-29T23:16:30Z","isPatch":false,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 1/29/2018 4:40 AM, Duy Nguyen wrote:\n> On Sat, Jan 27, 2018 at 12:43:41PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> b) with fsmonitor\n>>\n>>      $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n>>      12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index\n> \n> This is sort of off topic but may be interesting for big repo guys. It\n> looks like read cache's time is partially dominated by malloc().\n> \n\nThat is correct.  We have tried a few different ways to address this. \nFirst was my patch series [1] that would parallelize all of the read \ncache code.\n\nWe quickly found that malloc() was the biggest culprit and by speeding \nthat up, we got most of the wins.  At Peff's recommendation [2], we \nlooked into using tcmalloc but found that 1) it has bugs on Windows and \n2) it isn't being actively maintained so it didn't seem those bugs would \never get fixed.\n\nWe are currently working on a patch that will use a refactored version \nof the mem_pool in fast-import.c to do block allocations of the cache \nentries which is giving us about a 22% improvement in \"git status\" \ntimes.  One challenge has been ensuring that cache entries are not \npassed from one index/mem_pool to another which could cause access after \nfree bugs.\n\n[1] \nhttps://public-inbox.org/git/20171109141737.47976-1-benpeart@microsoft.com/\n[2] \nhttps://public-inbox.org/git/20171120153846.v5b7ho42yzrznqoh@sigill.intra.peff.net/\n\n\n> This is the performance breakdown of do_read_index()\n> \n> $ GIT_TRACE_PERFORMANCE=2 ~/w/git/t/helper/test-read-cache\n> 0.000078489 s: open/mmap/close\n> 0.038915239 s: main entries\n> 0.018983150 s: ext TREE\n> 0.012667080 s: ext UNTR\n> 0.000005372 s: ext FSMN\n> 0.001473470 s: munmap\n> 0.072386911 s: read cache .git/index\n> \n> Reading main index entries takes like half of the time (0.038 vs\n> 0.072). With the below patch to take out hundred thousands of malloc()\n> we have this, loading main entries now only takes 0.012s:\n> \n> $ GIT_TRACE_PERFORMANCE=2 ~/w/git/t/helper/test-read-cache\n> 0.000046587 s: open/mmap/close\n> 0.012077300 s: main entries\n> 0.020477683 s: ext TREE\n> 0.010259998 s: ext UNTR\n> 0.000010250 s: ext FSMN\n> 0.000753854 s: munmap\n> 0.043906473 s: read cache .git/index\n> \n> We used to do less malloc until debed2a629 (read-cache.c: allocate\n> index entries individually - 2011-10-24) but I don't think we can\n> simply revert that (not worth the extra complexity of the old way).\n> \n> Now \"TREE\" and \"UNTR\" extensions become a bigger problem.\n> \n> -- 8< --\n> diff --git a/read-cache.c b/read-cache.c\n> index d60e0a8480..88f4213c99 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1622,7 +1622,12 @@ static struct cache_entry *cache_entry_from_ondisk(struct ondisk_cache_entry *on\n>   \t\t\t\t\t\t   const char *name,\n>   \t\t\t\t\t\t   size_t len)\n>   {\n> +#if 0\n>   \tstruct cache_entry *ce = xmalloc(cache_entry_size(len));\n> +#else\n> +\tstatic char buf[1024];\n> +\tstruct cache_entry *ce = (struct cache_entry *)buf;\n> +#endif\n>   \n>   \tce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);\n>   \tce->ce_stat_data.sd_mtime.sec = get_be32(&ondisk->mtime.sec);\n> diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c\n> index 48255eef31..e1d21d17a3 100644\n> --- a/t/helper/test-read-cache.c\n> +++ b/t/helper/test-read-cache.c\n> @@ -8,7 +8,9 @@ int cmd_main(int argc, const char **argv)\n>   \tsetup_git_directory();\n>   \tfor (i = 0; i < cnt; i++) {\n>   \t\tread_cache();\n> +#if 0\n>   \t\tdiscard_cache();\n> +#endif\n>   \t}\n>   \treturn 0;\n>   }\n> -- 8< --\n> \n"},{"id":"337771","messageId":"56b8d8a3-9483-330d-64a4-ec0295b254ac@gmail.com","threadId":"47701","inReplyTo":"CACBZZX74WcK4qX5O1E_6nxv7f4Vpns3O-7dcURbs+QneKsA87Q@mail.gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-01-30T01:21:11Z","receivedAt":"2018-01-30T01:21:20Z","isPatch":false,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 1/28/2018 5:28 PM, Ævar Arnfjörð Bjarmason wrote:\n> On Sun, Jan 28, 2018 at 9:44 PM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n>> Hi,\n>>\n>> On Sat, 27 Jan 2018, Ævar Arnfjörð Bjarmason wrote:\n>>\n>>> I just got around to testing this since it landed, for context some\n>>> previous poking of mine in [1].\n>>>\n>>> Issues / stuff I've noticed:\n>>>\n>>> 1) We end up invalidating the untracked cache because stuff in .git/\n>>> changed. For example:\n>>>\n>>>      01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success\n>>>      01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'\n>>>      01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'\n>>>      01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'\n>>>      01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful\n>>>\n>>> Am I missing something or should we do something like:\n>>>\n>>>      diff --git a/fsmonitor.c b/fsmonitor.c\n>>>      index 0af7c4edba..5067b89bda 100644\n>>>      --- a/fsmonitor.c\n>>>      +++ b/fsmonitor.c\n>>>      @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n>>>\n>>>       static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)\n>>>       {\n>>>      -       int pos = index_name_pos(istate, name, strlen(name));\n>>>      +       int pos;\n>>>      +\n>>>      +       if (!strcmp(name, \".git\") || starts_with(name, \".git/\"))\n>>>      +               return;\n>>>      +\n>>>      +       pos = index_name_pos(istate, name, strlen(name));\n>>\n>> I would much rather have the fsmonitor hook already exclude those.\n> \n> As documented the fsmonitor-watchman hook we ship doesn't work as\n> described in githooks(5), unless \"files in the working directory\" is\n> taken to include .git/, but I haven't seen that ever used.\n> \n> On the other hand relying on arbitrary user-provided hooks to do the\n> right thing at the cost of silent performance degradation is bad. If\n> we're going to expect the hook to remove these we should probably\n> warn/die here if it does send us .git/* files.\n> \n\nI'm not sure how often something is modified in the git directory when \nnothing was modified in the working directory but this seems like a nice \noptimization.\n\nWe can't just blindly ignore changes under \".git\" as the git directory \nmay have been moved somewhere else.  Instead we'd need to use get_git_dir().\n\nRather than assuming the hook will optimize for this particular case, I \nthink a better solution would be to update \nuntracked_cache_invalidate_path() so that it doesn't invalidate the \nuntracked cache and mark the index as dirty when it was asked to \ninvalidate a path under GIT_DIR.  I can't think of a case when that \nwould be the desired behavior.\n\nSomewhat off topic but related to the overall performance discussion: \nI've also thought the untracked cache shouldn't mark the index as dirty \nexcept in the case where the extension is being added or removed.  We've \nobserved that it causes unnecessary index writes that actually slows \ndown overall performance.\n\nSince it is a cache, it does not require the index to be written out for \ncorrectness, it can simply update the cache again the next time it is \nneeded. This is typically faster than the cost of the index write so \nmakes things faster overall.  I adopted this same model with the \nfsmonitor extension.\n\n>> If you *must* add these comparisons, you have to use fspathcmp() and\n>> fspathncmp() instead (because case-insensitivity).\n> \n> Thanks.\n> \n"},{"id":"337850","messageId":"145f8cf4-8ade-7d86-31b6-5d92fcfea3ac@gmail.com","threadId":"47701","inReplyTo":"878tcjw2gd.fsf@evledraar.gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-01-30T22:41:26Z","receivedAt":"2018-01-30T22:41:34Z","isPatch":false,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 1/27/2018 2:01 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sat, Jan 27 2018, Duy Nguyen jotted:\n> \n>> On Sat, Jan 27, 2018 at 07:39:27PM +0700, Duy Nguyen wrote:\n>>> On Sat, Jan 27, 2018 at 6:43 PM, Ævar Arnfjörð Bjarmason\n>>> <avarab@gmail.com> wrote:\n>>>> a) no fsmonitor\n>>>>\n>>>>      $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n>>>>      12:32:44.947651 read-cache.c:1890       performance: 0.053153609 s: read cache .git/index\n>>>>      12:32:44.967943 preload-index.c:112     performance: 0.020161093 s: preload index\n>>>>      12:32:44.974217 read-cache.c:1446       performance: 0.006230611 s: refresh index\n>>>>\n>>>> ...\n>>>>\n>>>> b) with fsmonitor\n>>>>\n>>>>      $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n>>>>      12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index\n>>>>      12:34:23.838622 preload-index.c:112     performance: 0.001221197 s: preload index\n>>>>      12:34:23.858723 fsmonitor.c:170         performance: 0.020059647 s: fsmonitor process '.git/hooks/fsmonitor-watchman'\n>>>>      12:34:23.871532 read-cache.c:1446       performance: 0.032870818 s: refresh index\n>>>\n>>> Hmm.. why does refresh take longer with fsmonitor/watchman? With the\n>>> help from watchman, we know what files are modified. We don't need\n>>> manual stat()'ing and this line should be lower than the \"no\n>>> fsmonitor\" case, which is 0.006230611s.\n>>\n>> Ahh.. my patch probably does not see that fsmonitor could be activated\n>> lazily inside refresh_index() call. The patch below should fix it.\n> \n> Will have to get those numbers to you later, or alternatively clone\n> https://github.com/avar/2015-04-03-1M-git (or some other test repo) and\n> test it yourself, sorry. Don't have time to follow-up much this weekend.\n> \n>> But between your normal refresh time (0.020 preload + 0.006 actual\n>> refresh) and fsmonitor taking 0.020 just to talk to watchman, this\n>> repo seems \"too small\" for fsmonitor/watchman to shine.\n> \n> Surely that's an implementation limitation and not something inherent,\n> given that watchman itself returns in 5ms?\n> \n> I.e. status could work like this, no?:\n> \n>   1. At start, record the timestamp & find out canonical state via some\n>      expansive method.\n>   2. Print out xyz changed, abc added etc.\n>   3. Record *just* what status would report about xyz, abc etc.\n>   4. On subsequent git status, just amend that information, e.g. if\n>      watchman says nothing changed $(cat .git/last-status-output).\n> \n> We shouldn't need to be reading the entire index in the common case\n> where just a few things change.\n> \n\nI agree that reading the entire index in the common case is rather \nexpensive. It is, however, the model we have today and all the code in \ngit assumes all cache entries are in memory.\n\nWe are interested in pursuing a patch series that would enable higher \nperformance especially with large and/or sparse repos by making the \nindex sparse, hierarchical, and incrementally readable/writable.  As you \nmight expect, that is a lot of work and is far beyond what we can \naddress in this patch series.\n\n> There's also a lot of things that use status to just check \"are we\n> clean?\", those would only need to record the last known timestamp when\n> the tree was clean, and then ask watchman if there were any changes, if\n> not we're done.\n> \n>> I'm still a bit curious that refresh index time, after excluding 0.020\n>> for fsmonitor, is stil 0.012s. What does it do? It should really be\n>> doing nothing. Either way, read index time seems to be the elephant in\n>> the room now.\n>>\n>> -- 8< --\n>> diff --git a/read-cache.c b/read-cache.c\n>> index eac74bc9f1..d60e0a8480 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -1367,12 +1367,21 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n>>   \tunsigned int options = (CE_MATCH_REFRESH |\n>>   \t\t\t\t(really ? CE_MATCH_IGNORE_VALID : 0) |\n>>   \t\t\t\t(not_new ? CE_MATCH_IGNORE_MISSING : 0));\n>> +\tint ignore_fsmonitor = options & CE_MATCH_IGNORE_FSMONITOR;\n>>   \tconst char *modified_fmt;\n>>   \tconst char *deleted_fmt;\n>>   \tconst char *typechange_fmt;\n>>   \tconst char *added_fmt;\n>>   \tconst char *unmerged_fmt;\n>> -\tuint64_t start = getnanotime();\n>> +\tuint64_t start;\n>> +\n>> +\t/*\n>> +\t * If fsmonitor is used, force its communication early to\n>> +\t * accurately measure how long this function takes without it.\n>> +\t */\n>> +\tif (!ignore_fsmonitor)\n>> +\t\trefresh_fsmonitor(istate);\n>> +\tstart = getnanotime();\n>>\n>>   \tmodified_fmt = (in_porcelain ? \"M\\t%s\\n\" : \"%s: needs update\\n\");\n>>   \tdeleted_fmt = (in_porcelain ? \"D\\t%s\\n\" : \"%s: needs update\\n\");\n>> -- 8< --\n"},{"id":"337851","messageId":"26b9d795-b2b3-905d-c67a-83bc4e976894@gmail.com","threadId":"47701","inReplyTo":"87efmcw3fa.fsf@evledraar.gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-01-30T22:57:18Z","receivedAt":"2018-01-30T22:57:27Z","isPatch":false,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"While some of these issues have been discussed in other threads, I \nthought I'd summarize my thoughts here.\n\nOn 1/26/2018 7:28 PM, Ævar Arnfjörð Bjarmason wrote:\n> I just got around to testing this since it landed, for context some\n> previous poking of mine in [1].\n> \n> Issues / stuff I've noticed:\n> \n> 1) We end up invalidating the untracked cache because stuff in .git/\n> changed. For example:\n> \n>      01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success\n>      01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'\n>      01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'\n>      01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'\n>      01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful\n> \n> Am I missing something or should we do something like:\n> \n>      diff --git a/fsmonitor.c b/fsmonitor.c\n>      index 0af7c4edba..5067b89bda 100644\n>      --- a/fsmonitor.c\n>      +++ b/fsmonitor.c\n>      @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n> \n>       static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)\n>       {\n>      -       int pos = index_name_pos(istate, name, strlen(name));\n>      +       int pos;\n>      +\n>      +       if (!strcmp(name, \".git\") || starts_with(name, \".git/\"))\n>      +               return;\n>      +\n>      +       pos = index_name_pos(istate, name, strlen(name));\n> \n>              if (pos >= 0) {\n>                      struct cache_entry *ce = istate->cache[pos];\n> \n> With that patch applied status on a large repo[2] goes from a consistent\n> ~180-200ms to ~140-150ms, since we're not invalidating some of the UC\n> structure\n> \n\nI favor making this optimization by updating \nuntracked_cache_invalidate_path() so that it ignores paths under \nget_git_dir() and doesn't invalidate the untracked cache or flag the \nindex as dirty.\n\n> 2) We re-write out the index even though we know nothing changed\n> \n> When you first run with core.fsmonitor it needs to\n> mark_fsmonitor_clean() for every path, but is there a reason for why we\n> wouldn't supply the equivalent of GIT_OPTIONAL_LOCKS=0 if all paths are\n> marked and we know from the hook that nothing changed? Why write out the\n> index again?\n> \n\nWriting out the index when core.fsmonitor is first turned on is \nnecessary to get the index extension added with the current state of the \ndirty flags.  Given it is a one time cost, I don't think we have \nanything worth trying to optimize here.\n\n> 3) A lot of time spend reading the index (or something..)\n> \n> While the hook itself takes ~20ms (and watchman itself 1/4 of that)\n> status as a whole takes much longer. gprof reveals:\n> \n>      Each sample counts as 0.01 seconds.\n>        %   cumulative   self              self     total\n>       time   seconds   seconds    calls  ms/call  ms/call  name\n>       15.38      0.02     0.02   221690     0.00     0.00  memihash\n>       15.38      0.04     0.02   221689     0.00     0.00  create_from_disk\n>        7.69      0.05     0.01  2216897     0.00     0.00  git_bswap32\n>        7.69      0.06     0.01   222661     0.00     0.00  ce_path_match\n>        7.69      0.07     0.01   221769     0.00     0.00  hashmap_add\n>        7.69      0.08     0.01    39941     0.00     0.00  prep_exclude\n>        7.69      0.09     0.01    39940     0.00     0.00  strbuf_addch\n>        7.69      0.10     0.01        1    10.00    10.00  read_one\n>        7.69      0.11     0.01        1    10.00    10.00  refresh_index\n>        7.69      0.12     0.01        1    10.00    10.00  tweak_fsmonitor\n>        7.69      0.13     0.01                             preload_thread\n> \n> The index is 24M in this case, I guess it's unpacking it, but I wonder\n> if this couldn't be much faster if we saved away the result of the last\n> \"status\" in something that's quick to access, and then if nothing\n> changed we just report that, and no need to re-write the index (or just\n> write the \"it was clean at this time\" part).\n\nYes, reading the index is slow.  We've made some improvements (not \ncomputing the SHA, not validating the sort order, etc) and have one more \nin progress that will reduce the malloc() cost.  I haven't found any \nother easy optimizations but it would be great if you could find more! \nTo make significant improvements, I'm afraid it will take more \nsubstantial changes to the in memory and on disk formats and updates to \nthe code to take advantage of those changes.\n\n> \n> 4) core.fsmonitor=false behaves unexpectedly\n> \n> The code that reads this variable just treats it as a string, so we do a\n> bunch of work for nothing (and nothing warns) if this is set and 'false'\n> is executed. Any reason we couldn't do our standard boolean parsing\n> here? You couldn't call your hook 0/1/true/false, but that doesn't seem\n> like a big loss.\n> \n> 1. https://public-inbox.org/git/CACBZZX5a6Op7dH_g9WOFBnejh2zgNK4b34ygxA8daNDqvitFVA@mail.gmail.com/\n> 2. https://github.com/avar/2015-04-03-1M-git\n> \n\nI'm torn on this one.  The core.fsmontior setting isn't a boolean value, \nits a string that is the command to run when we need file system \nchanges.  It would be pretty simple to add a call to \ngit_parse_maybe_bool_text() to treat \"false,\" \"no,\" or \"off\" the same as \nan empty string but that makes it look even more like a boolean when it \nisn't.\n"},{"id":"337854","messageId":"87vafjlyyp.fsf@evledraar.gmail.com","threadId":"47701","inReplyTo":"26b9d795-b2b3-905d-c67a-83bc4e976894@gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-30T23:16:14Z","receivedAt":"2018-01-30T23:16:23Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 30 2018, Ben Peart jotted:\n\n> While some of these issues have been discussed in other threads, I\n> thought I'd summarize my thoughts here.\n\nThanks for this & your other reply. I'm going to get to testing some of\nDuy's patches soon, and if you have some other relevant WIP I'd be happy\nto try them, but meanwhile replying to a few of these:\n\n> On 1/26/2018 7:28 PM, Ævar Arnfjörð Bjarmason wrote:\n>> I just got around to testing this since it landed, for context some\n>> previous poking of mine in [1].\n>>\n>> Issues / stuff I've noticed:\n>>\n>> 1) We end up invalidating the untracked cache because stuff in .git/\n>> changed. For example:\n>>\n>>      01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success\n>>      01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'\n>>      01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'\n>>      01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'\n>>      01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful\n>>\n>> Am I missing something or should we do something like:\n>>\n>>      diff --git a/fsmonitor.c b/fsmonitor.c\n>>      index 0af7c4edba..5067b89bda 100644\n>>      --- a/fsmonitor.c\n>>      +++ b/fsmonitor.c\n>>      @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n>>\n>>       static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)\n>>       {\n>>      -       int pos = index_name_pos(istate, name, strlen(name));\n>>      +       int pos;\n>>      +\n>>      +       if (!strcmp(name, \".git\") || starts_with(name, \".git/\"))\n>>      +               return;\n>>      +\n>>      +       pos = index_name_pos(istate, name, strlen(name));\n>>\n>>              if (pos >= 0) {\n>>                      struct cache_entry *ce = istate->cache[pos];\n>>\n>> With that patch applied status on a large repo[2] goes from a consistent\n>> ~180-200ms to ~140-150ms, since we're not invalidating some of the UC\n>> structure\n>>\n>\n> I favor making this optimization by updating\n> untracked_cache_invalidate_path() so that it ignores paths under\n> get_git_dir() and doesn't invalidate the untracked cache or flag the\n> index as dirty.\n\n*nod*\n\n>> 2) We re-write out the index even though we know nothing changed\n>>\n>> When you first run with core.fsmonitor it needs to\n>> mark_fsmonitor_clean() for every path, but is there a reason for why we\n>> wouldn't supply the equivalent of GIT_OPTIONAL_LOCKS=0 if all paths are\n>> marked and we know from the hook that nothing changed? Why write out the\n>> index again?\n>>\n>\n> Writing out the index when core.fsmonitor is first turned on is\n> necessary to get the index extension added with the current state of\n> the dirty flags.  Given it is a one time cost, I don't think we have\n> anything worth trying to optimize here.\n\nIndeed, that makes sense. What I was showing here is even after the\ninitial setup we continue to write it out when we know nothing changed.\n\nWe do that anyway without fsmonitor, but this seemed like a worthwhile\nthing to optimize.\n\n>> 3) A lot of time spend reading the index (or something..)\n>>\n>> While the hook itself takes ~20ms (and watchman itself 1/4 of that)\n>> status as a whole takes much longer. gprof reveals:\n>>\n>>      Each sample counts as 0.01 seconds.\n>>        %   cumulative   self              self     total\n>>       time   seconds   seconds    calls  ms/call  ms/call  name\n>>       15.38      0.02     0.02   221690     0.00     0.00  memihash\n>>       15.38      0.04     0.02   221689     0.00     0.00  create_from_disk\n>>        7.69      0.05     0.01  2216897     0.00     0.00  git_bswap32\n>>        7.69      0.06     0.01   222661     0.00     0.00  ce_path_match\n>>        7.69      0.07     0.01   221769     0.00     0.00  hashmap_add\n>>        7.69      0.08     0.01    39941     0.00     0.00  prep_exclude\n>>        7.69      0.09     0.01    39940     0.00     0.00  strbuf_addch\n>>        7.69      0.10     0.01        1    10.00    10.00  read_one\n>>        7.69      0.11     0.01        1    10.00    10.00  refresh_index\n>>        7.69      0.12     0.01        1    10.00    10.00  tweak_fsmonitor\n>>        7.69      0.13     0.01                             preload_thread\n>>\n>> The index is 24M in this case, I guess it's unpacking it, but I wonder\n>> if this couldn't be much faster if we saved away the result of the last\n>> \"status\" in something that's quick to access, and then if nothing\n>> changed we just report that, and no need to re-write the index (or just\n>> write the \"it was clean at this time\" part).\n>\n> Yes, reading the index is slow.  We've made some improvements (not\n> computing the SHA, not validating the sort order, etc) and have one\n> more in progress that will reduce the malloc() cost.  I haven't found\n> any other easy optimizations but it would be great if you could find\n> more! To make significant improvements, I'm afraid it will take more\n> substantial changes to the in memory and on disk formats and updates\n> to the code to take advantage of those changes.\n\nWhat I was wondering (not very clearly) is whether an easier\noptimization for now would be to speed up the case where nothing\nchanged, that would involve just reading some flag in the index (or\nelsewhere) saying nothing changed last time, then the timestamp the\nfsmonitor writes, and trusting the hook when it says nothing changed\nsince that timestamp.\n\n>>\n>> 4) core.fsmonitor=false behaves unexpectedly\n>>\n>> The code that reads this variable just treats it as a string, so we do a\n>> bunch of work for nothing (and nothing warns) if this is set and 'false'\n>> is executed. Any reason we couldn't do our standard boolean parsing\n>> here? You couldn't call your hook 0/1/true/false, but that doesn't seem\n>> like a big loss.\n>>\n>> 1. https://public-inbox.org/git/CACBZZX5a6Op7dH_g9WOFBnejh2zgNK4b34ygxA8daNDqvitFVA@mail.gmail.com/\n>> 2. https://github.com/avar/2015-04-03-1M-git\n>>\n>\n> I'm torn on this one.  The core.fsmontior setting isn't a boolean\n> value, its a string that is the command to run when we need file\n> system changes.  It would be pretty simple to add a call to\n> git_parse_maybe_bool_text() to treat \"false,\" \"no,\" or \"off\" the same\n> as an empty string but that makes it look even more like a boolean\n> when it isn't.\n\nYes, that makes sense. I wonder though if we should warn if the hook is\nset and returning non-zero, right now that failure case is silent, maybe\nthe hook would like to return an exit code for \"don't ask me\", but it\nseems better to make that 125 (like git bisect skip) instead of !0,\nsince that makes the hook failing completely indistinguishable to the\nuser from the hook doing its job.\n"},{"id":"337908","messageId":"CACsJy8DH2_gBqj3xs4Cr6kOAUsQ5hn88weMeXR8GtYCNvYgLDQ@mail.gmail.com","threadId":"47701","inReplyTo":"56b8d8a3-9483-330d-64a4-ec0295b254ac@gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-31T10:15:44Z","receivedAt":"2018-01-31T10:16:26Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 30, 2018 at 8:21 AM, Ben Peart <peartben@gmail.com> wrote:\n>\n>\n> On 1/28/2018 5:28 PM, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Sun, Jan 28, 2018 at 9:44 PM, Johannes Schindelin\n>> <Johannes.Schindelin@gmx.de> wrote:\n>>>\n>>> Hi,\n>>>\n>>> On Sat, 27 Jan 2018, Ævar Arnfjörð Bjarmason wrote:\n>>>\n>>>> I just got around to testing this since it landed, for context some\n>>>> previous poking of mine in [1].\n>>>>\n>>>> Issues / stuff I've noticed:\n>>>>\n>>>> 1) We end up invalidating the untracked cache because stuff in .git/\n>>>> changed. For example:\n>>>>\n>>>>      01:09:24.975524 fsmonitor.c:173         fsmonitor process\n>>>> '.git/hooks/fsmonitor-watchman' returned success\n>>>>      01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback\n>>>> '.git'\n>>>>      01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback\n>>>> '.git/config'\n>>>>      01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback\n>>>> '.git/index'\n>>>>      01:09:25.122726 fsmonitor.c:91          write fsmonitor extension\n>>>> successful\n>>>>\n>>>> Am I missing something or should we do something like:\n>>>>\n>>>>      diff --git a/fsmonitor.c b/fsmonitor.c\n>>>>      index 0af7c4edba..5067b89bda 100644\n>>>>      --- a/fsmonitor.c\n>>>>      +++ b/fsmonitor.c\n>>>>      @@ -118,7 +118,12 @@ static int query_fsmonitor(int version,\n>>>> uint64_t last_update, struct strbuf *que\n>>>>\n>>>>       static void fsmonitor_refresh_callback(struct index_state *istate,\n>>>> const char *name)\n>>>>       {\n>>>>      -       int pos = index_name_pos(istate, name, strlen(name));\n>>>>      +       int pos;\n>>>>      +\n>>>>      +       if (!strcmp(name, \".git\") || starts_with(name, \".git/\"))\n>>>>      +               return;\n>>>>      +\n>>>>      +       pos = index_name_pos(istate, name, strlen(name));\n>>>\n>>>\n>>> I would much rather have the fsmonitor hook already exclude those.\n>>\n>>\n>> As documented the fsmonitor-watchman hook we ship doesn't work as\n>> described in githooks(5), unless \"files in the working directory\" is\n>> taken to include .git/, but I haven't seen that ever used.\n>>\n>> On the other hand relying on arbitrary user-provided hooks to do the\n>> right thing at the cost of silent performance degradation is bad. If\n>> we're going to expect the hook to remove these we should probably\n>> warn/die here if it does send us .git/* files.\n>>\n>\n> I'm not sure how often something is modified in the git directory when\n> nothing was modified in the working directory but this seems like a nice\n> optimization.\n>\n> We can't just blindly ignore changes under \".git\" as the git directory may\n> have been moved somewhere else.  Instead we'd need to use get_git_dir().\n\nIn theory. But we do blindly ignore changes under \".git\" in some\ncases, see treat_path() in dir.c for example.\n\n> Rather than assuming the hook will optimize for this particular case, I\n> think a better solution would be to update untracked_cache_invalidate_path()\n> so that it doesn't invalidate the untracked cache and mark the index as\n> dirty when it was asked to invalidate a path under GIT_DIR.  I can't think\n> of a case when that would be the desired behavior.\n\nYou see, my only problem with this is tying the check with $GIT_DIR,\nwhich may involve normalizing the path and all that stuff because the\ncurrent code base is not prepared to deal with that. Simply ignoring\nanything in \".git\" may work too. Not pretty but it's more in line with\nwhat we have. Though I'm still not sure how it interacts with \".git\"\nfrom submodules which is why I still have not sent a patch to update\nuntracked_cache_invalidate_path() because it does make sense to fix it\nin there.\n\n> Somewhat off topic but related to the overall performance discussion: I've\n> also thought the untracked cache shouldn't mark the index as dirty except in\n> the case where the extension is being added or removed.  We've observed that\n> it causes unnecessary index writes that actually slows down overall\n> performance.\n>\n> Since it is a cache, it does not require the index to be written out for\n> correctness, it can simply update the cache again the next time it is\n> needed. This is typically faster than the cost of the index write so makes\n> things faster overall.  I adopted this same model with the fsmonitor\n> extension.\n\nIf you turn on split index, the write cost should be much much less\n(but I think read cost increases a bit due to merging the two indexes\nin core; I noticed this but haven't really dug down). You basically\npay writing modified index entries and extensions.\n\nBut yeah not writing is possible. The index's dirty flag can show that\nonly untracked cache extension is dirty, then it's a judgement call\nwhether to write it down or drop it. You still need to occasionally\nwrite it down though. Dirty directories will fall back to slow path.\nIf you don't write it down, the set of dirty paths keeps increasing\nand will start to slow git-status down. I don't know at what point we\nshould write it down though if you choose not to go the split-index\nroute.\n-- \nDuy\n"},{"id":"337964","messageId":"ee8f571e-a4cc-e8d4-b490-072e1579ee65@gmail.com","threadId":"47701","inReplyTo":"87vafjlyyp.fsf@evledraar.gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-01-31T16:12:36Z","receivedAt":"2018-01-31T16:12:43Z","isPatch":false,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 1/30/2018 6:16 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Jan 30 2018, Ben Peart jotted:\n> \n>> While some of these issues have been discussed in other threads, I\n>> thought I'd summarize my thoughts here.\n> \n> Thanks for this & your other reply. I'm going to get to testing some of\n> Duy's patches soon, and if you have some other relevant WIP I'd be happy\n> to try them, but meanwhile replying to a few of these:\n> \n>> On 1/26/2018 7:28 PM, Ævar Arnfjörð Bjarmason wrote:\n>>> I just got around to testing this since it landed, for context some\n>>> previous poking of mine in [1].\n>>>\n>>> Issues / stuff I've noticed:\n>>>\n>>> 1) We end up invalidating the untracked cache because stuff in .git/\n>>> changed. For example:\n>>>\n>>>       01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success\n>>>       01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'\n>>>       01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'\n>>>       01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'\n>>>       01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful\n>>>\n>>> Am I missing something or should we do something like:\n>>>\n>>>       diff --git a/fsmonitor.c b/fsmonitor.c\n>>>       index 0af7c4edba..5067b89bda 100644\n>>>       --- a/fsmonitor.c\n>>>       +++ b/fsmonitor.c\n>>>       @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n>>>\n>>>        static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)\n>>>        {\n>>>       -       int pos = index_name_pos(istate, name, strlen(name));\n>>>       +       int pos;\n>>>       +\n>>>       +       if (!strcmp(name, \".git\") || starts_with(name, \".git/\"))\n>>>       +               return;\n>>>       +\n>>>       +       pos = index_name_pos(istate, name, strlen(name));\n>>>\n>>>               if (pos >= 0) {\n>>>                       struct cache_entry *ce = istate->cache[pos];\n>>>\n>>> With that patch applied status on a large repo[2] goes from a consistent\n>>> ~180-200ms to ~140-150ms, since we're not invalidating some of the UC\n>>> structure\n>>>\n>>\n>> I favor making this optimization by updating\n>> untracked_cache_invalidate_path() so that it ignores paths under\n>> get_git_dir() and doesn't invalidate the untracked cache or flag the\n>> index as dirty.\n> \n> *nod*\n> \n>>> 2) We re-write out the index even though we know nothing changed\n>>>\n>>> When you first run with core.fsmonitor it needs to\n>>> mark_fsmonitor_clean() for every path, but is there a reason for why we\n>>> wouldn't supply the equivalent of GIT_OPTIONAL_LOCKS=0 if all paths are\n>>> marked and we know from the hook that nothing changed? Why write out the\n>>> index again?\n>>>\n>>\n>> Writing out the index when core.fsmonitor is first turned on is\n>> necessary to get the index extension added with the current state of\n>> the dirty flags.  Given it is a one time cost, I don't think we have\n>> anything worth trying to optimize here.\n> \n> Indeed, that makes sense. What I was showing here is even after the\n> initial setup we continue to write it out when we know nothing changed.\n> \n> We do that anyway without fsmonitor, but this seemed like a worthwhile\n> thing to optimize.\n> \n\nThere is already logic to avoid writing out the index unless there is \nsomething that requires it.  In my testing, it was often the untracked \ncache marking the index as dirty that was causing the unexpected writes.\n\nThe patch to make the untracked cache only flag the index as dirty when \nthe feature is being turned on or off is pretty simple (see below).  The \nchallenge was that many of the untracked cache tests assume all changes \nare saved to disk after every command so they fail after making this \nchange.  The patch below does a simple hack of checking for a test \nenvironment variable and only forcing the index writes if it is set.\n\nInternally, we simply turned off the untracked cache as it's usefulness \nin combination with GVFS is limited and without the patch, it actually \nslowed down common operations more than it sped them up.\n\nTypically, changes to the untracked cache don't accumulate long before \nthe user does some operation that requires the index to be written out \nat which point the untracked cache is updated as well.\n\n\ndiff --git a/dir.c b/dir.c\nindex 5e93a1350b..af1d33aae1 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2256,7 +2256,8 @@ int read_directory(struct dir_struct *dir, struct \nindex_state *istate,\n                                  dir->untracked->gitignore_invalidated,\n                                  dir->untracked->dir_invalidated,\n                                  dir->untracked->dir_opened);\n-               if (dir->untracked == istate->untracked &&\n+               if (getenv(\"GIT_TEST_UNTRACKED_CACHE\") &&\n+                       dir->untracked == istate->untracked &&\n                     (dir->untracked->dir_opened ||\n                      dir->untracked->gitignore_invalidated ||\n                      dir->untracked->dir_invalidated))\ndiff --git a/t/t7063-status-untracked-cache.sh \nb/t/t7063-status-untracked-cache.sh\nindex ea9383e8cb..e5811b6ef2 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -14,6 +14,8 @@ test_description='test untracked cache'\n  # See <20160803174522.5571-1-pclouds@gmail.com> if you want to know\n  # more.\n\n+export GIT_TEST_UNTRACKED_CACHE=true\n+\n  sync_mtime () {\n         if test_have_prereq BUSYBOX\n         then\n\n\n"},{"id":"338017","messageId":"CACsJy8CShVSQAZ6ZaGT9i0JDEn19cTMaEmLRDV3ULjaj9DqZUg@mail.gmail.com","threadId":"47701","inReplyTo":"bb8a433f-bd61-1f3c-2034-1acc96539882@gmail.com","subject":"Re: Some rough edges of core.fsmonitor","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-01T10:40:10Z","receivedAt":"2018-02-01T10:40:46Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 30, 2018 at 6:16 AM, Ben Peart <peartben@gmail.com> wrote:\n>\n>\n> On 1/29/2018 4:40 AM, Duy Nguyen wrote:\n>>\n>> On Sat, Jan 27, 2018 at 12:43:41PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>>>\n>>> b) with fsmonitor\n>>>\n>>>      $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status\n>>>      12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s:\n>>> read cache .git/index\n>>\n>>\n>> This is sort of off topic but may be interesting for big repo guys. It\n>> looks like read cache's time is partially dominated by malloc().\n>>\n>\n> That is correct.  We have tried a few different ways to address this. First\n> was my patch series [1] that would parallelize all of the read cache code.\n>\n> We quickly found that malloc() was the biggest culprit and by speeding that\n> up, we got most of the wins.  At Peff's recommendation [2], we looked into\n> using tcmalloc but found that 1) it has bugs on Windows and 2) it isn't\n> being actively maintained so it didn't seem those bugs would ever get fixed.\n>\n> We are currently working on a patch that will use a refactored version of\n> the mem_pool in fast-import.c to do block allocations of the cache entries\n> which is giving us about a 22% improvement in \"git status\" times.  One\n\nMy apologies if this has been discussed in the second half of 2017\nwhich I have no idea what happened.\n\nI just wonder if it's possible to design a \"file format\" that is\nbasically a memory dump of lots of struct cache_entry? This \"file\"\nwill stay in a shared memory somewhere and never get written down to\ndisk. Since it's very close to the data structure we have in core, the\nmost we need to do after mmap'ing it (and keeping it mmap'd until the\nend) is adjust some pointers. These entries are of course read-only.\nWhen you modify/create new entries, they are created new, using the\nold malloc(). We just need to make sure not free the read-only\ncache_entry entries and munmap() the whole thing when we\ndiscard_index().\n\nThis opens up another option to deal with the large UNTR and TREE\nextensions in a similar way. These will be your next headache after\nyou have reduced parse time for main entries.\n\n> challenge has been ensuring that cache entries are not passed from one\n> index/mem_pool to another which could cause access after free bugs.\n\nWe kind of have something close to that, but not entirely the same.\nWhen split index is used, the same cache_entry can appear in two\nindex_state structs. Of course you can free only one of them (and you\ncan only do so when you know both index_state are gone). I see some\ncode cleanup opportunity :)\n\n>\n> [1]\n> https://public-inbox.org/git/20171109141737.47976-1-benpeart@microsoft.com/\n> [2]\n> https://public-inbox.org/git/20171120153846.v5b7ho42yzrznqoh@sigill.intra.peff.net/\n-- \nDuy\n"},{"id":"338161","messageId":"20180204093823.3671-1-pclouds@gmail.com","threadId":"47701","inReplyTo":"CACsJy8DH2_gBqj3xs4Cr6kOAUsQ5hn88weMeXR8GtYCNvYgLDQ@mail.gmail.com","subject":"[PATCH] dir.c: ignore paths containing .git when invalidating untracked cache","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-04T09:38:23Z","receivedAt":"2018-02-04T09:38:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"read_directory() code ignores all paths named \".git\" even if it's not\na valid git repository. See treat_path() for details. Since \".git\" is\nbasically invisible to read_directory(), when we are asked to\ninvalidate a path that contains \".git\", we can safely ignore it\nbecause the slow path would not consider it anyway.\n\nThis helps when fsmonitor is used and we have a real \".git\" repo at\nworktree top. Occasionally .git/index will be updated and if the\nfsmonitor hook does not filter it, untracked cache is asked to\ninvalidate the path \".git/index\".\n\nWithout this patch, we invalidate the root directory unncessarily,\nwhich:\n\n- makes read_directory() fall back to slow path for root directory\n  (slower)\n\n- makes the index dirty (because UNTR extension is updated). Depending\n  on the index size, writing it down could also be slow.\n\nNoticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Sorry for the resend, I forgot git@vger.\n\n dir.c             | 13 ++++++++++++-\n git-compat-util.h |  2 ++\n wrapper.c         | 12 ++++++++++++\n 3 files changed, 26 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7c4b45e30e..f8b4cabba9 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \tif (!de)\n \t\treturn treat_path_fast(dir, untracked, cdir, istate, path,\n \t\t\t\t       baselen, pathspec);\n-\tif (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, \".git\"))\n+\tif (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, \".git\"))\n \t\treturn path_none;\n \tstrbuf_setlen(path, baselen);\n \tstrbuf_addstr(path, de->d_name);\n@@ -2970,8 +2970,19 @@ static int invalidate_one_component(struct untracked_cache *uc,\n void untracked_cache_invalidate_path(struct index_state *istate,\n \t\t\t\t     const char *path)\n {\n+\tconst char *end;\n+\tint skipped;\n+\n \tif (!istate->untracked || !istate->untracked->root)\n \t\treturn;\n+\tif (!fspathcmp(path, \".git\"))\n+\t\treturn;\n+\tif (ignore_case)\n+\t\tskipped = skip_caseprefix(path, \"/.git\", &end);\n+\telse\n+\t\tskipped = skip_prefix(path, \"/.git\", &end);\n+\tif (skipped && (*end == '\\0' || *end == '/'))\n+\t\treturn;\n \tinvalidate_one_component(istate->untracked, istate->untracked->root,\n \t\t\t\t path, strlen(path));\n }\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 68b2ad531e..27e0b761a3 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -484,6 +484,8 @@ static inline int skip_prefix(const char *str, const char *prefix,\n \treturn 0;\n }\n \n+int skip_caseprefix(const char *str, const char *prefix, const char **out);\n+\n /*\n  * If the string \"str\" is the same as the string in \"prefix\", then the \"arg\"\n  * parameter is set to the \"def\" parameter and 1 is returned.\ndiff --git a/wrapper.c b/wrapper.c\nindex d20356a776..bb888d9401 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -690,3 +690,15 @@ int xgethostname(char *buf, size_t len)\n \t\tbuf[len - 1] = 0;\n \treturn ret;\n }\n+\n+int skip_caseprefix(const char *str, const char *prefix, const char **out)\n+{\n+\tdo {\n+\t\tif (!*prefix) {\n+\t\t\t*out = str;\n+\t\t\treturn 1;\n+\t\t}\n+\t} while (tolower(*str++) == tolower(*prefix++));\n+\n+\treturn 0;\n+}\n-- \n2.16.1.207.gedba492059\n\n"},{"id":"338222","messageId":"f002344e-e8ae-84e8-a1aa-45e44c0ccc88@gmail.com","threadId":"47701","inReplyTo":"20180204093823.3671-1-pclouds@gmail.com","subject":"Re: [PATCH] dir.c: ignore paths containing .git when invalidating untracked cache","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-02-05T17:44:05Z","receivedAt":"2018-02-05T17:44:13Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 2/4/2018 4:38 AM, Nguyễn Thái Ngọc Duy wrote:\n> read_directory() code ignores all paths named \".git\" even if it's not\n> a valid git repository. See treat_path() for details. Since \".git\" is\n> basically invisible to read_directory(), when we are asked to\n> invalidate a path that contains \".git\", we can safely ignore it\n> because the slow path would not consider it anyway.\n> \n> This helps when fsmonitor is used and we have a real \".git\" repo at\n> worktree top. Occasionally .git/index will be updated and if the\n> fsmonitor hook does not filter it, untracked cache is asked to\n> invalidate the path \".git/index\".\n> \n> Without this patch, we invalidate the root directory unncessarily,\n> which:\n> \n> - makes read_directory() fall back to slow path for root directory\n>    (slower)\n> \n> - makes the index dirty (because UNTR extension is updated). Depending\n>    on the index size, writing it down could also be slow.\n> \n> Noticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>   Sorry for the resend, I forgot git@vger.\n> \n>   dir.c             | 13 ++++++++++++-\n>   git-compat-util.h |  2 ++\n>   wrapper.c         | 12 ++++++++++++\n>   3 files changed, 26 insertions(+), 1 deletion(-)\n> \n> diff --git a/dir.c b/dir.c\n> index 7c4b45e30e..f8b4cabba9 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n>   \tif (!de)\n>   \t\treturn treat_path_fast(dir, untracked, cdir, istate, path,\n>   \t\t\t\t       baselen, pathspec);\n> -\tif (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, \".git\"))\n> +\tif (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, \".git\"))\n>   \t\treturn path_none;\n>   \tstrbuf_setlen(path, baselen);\n>   \tstrbuf_addstr(path, de->d_name);\n> @@ -2970,8 +2970,19 @@ static int invalidate_one_component(struct untracked_cache *uc,\n>   void untracked_cache_invalidate_path(struct index_state *istate,\n>   \t\t\t\t     const char *path)\n>   {\n> +\tconst char *end;\n> +\tint skipped;\n> +\n>   \tif (!istate->untracked || !istate->untracked->root)\n>   \t\treturn;\n\nThank you for this patch!  It's great to see people helping improve the \nperformance of git.\n\nI ran a quick test and this is not sufficient to prevent the index from \ngetting flagged as dirty and written out to disk when fsmonitor detects \nchanges for files under the .git folder.  What I'm seeing is that when \n\".git/index\" is returned, the test below doesn't catch them and end \nearly as would be expected.\n\nAs a result, invalidate_one_component() is called which calls \ninvalidate_one_directory() which increments dir_invalidated counter and \nthe regular dirty process continues which triggers the index to be \nwritten to disk unnecessarily.\n\n> +\tif (!fspathcmp(path, \".git\"))\n> +\t\treturn;\n> +\tif (ignore_case)\n> +\t\tskipped = skip_caseprefix(path, \"/.git\", &end);\n> +\telse\n> +\t\tskipped = skip_prefix(path, \"/.git\", &end);\n> +\tif (skipped && (*end == '\\0' || *end == '/'))\n> +\t\treturn;\n\nIf I replace the above lines with:\n\n\tif (ignore_case)\n\t\tskipped = skip_caseprefix(path, \".git\", &end);\n\telse\n\t\tskipped = skip_prefix(path, \".git\", &end);\n\nThen it correctly skips _all_ files under \".git\".  I'm not sure if by \nremoving the leading slash, I'm breaking some other case but I was not \nable to find where that was expected or needed. Removing the leading \nslash also allows me to remove the fsmpathcmp() call as it is now redundant.\n\nI wondered what would happen if there was some other directory named \n\".git\" and how that would behave.  With or without this patch and with \nor without the untracked cache, a file \"dir1/.git/foo\" is ignored by git \nso no change in behavior there either.\n\n>   \tinvalidate_one_component(istate->untracked, istate->untracked->root,\n>   \t\t\t\t path, strlen(path));\n>   }\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 68b2ad531e..27e0b761a3 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -484,6 +484,8 @@ static inline int skip_prefix(const char *str, const char *prefix,\n>   \treturn 0;\n>   }\n>   \n> +int skip_caseprefix(const char *str, const char *prefix, const char **out);\n> +\n>   /*\n>    * If the string \"str\" is the same as the string in \"prefix\", then the \"arg\"\n>    * parameter is set to the \"def\" parameter and 1 is returned.\n> diff --git a/wrapper.c b/wrapper.c\n> index d20356a776..bb888d9401 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -690,3 +690,15 @@ int xgethostname(char *buf, size_t len)\n>   \t\tbuf[len - 1] = 0;\n>   \treturn ret;\n>   }\n> +\n> +int skip_caseprefix(const char *str, const char *prefix, const char **out)\n> +{\n> +\tdo {\n> +\t\tif (!*prefix) {\n> +\t\t\t*out = str;\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t} while (tolower(*str++) == tolower(*prefix++));\n> +\n> +\treturn 0;\n> +}\n> \n"},{"id":"338502","messageId":"CACsJy8DHvae4H+JuMkLxo+M=tWro-C_3th+bZnAi+G-_qA_ZJA@mail.gmail.com","threadId":"47701","inReplyTo":"f002344e-e8ae-84e8-a1aa-45e44c0ccc88@gmail.com","subject":"Re: [PATCH] dir.c: ignore paths containing .git when invalidating untracked cache","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-06T12:02:16Z","receivedAt":"2018-02-06T12:02:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 6, 2018 at 12:44 AM, Ben Peart <peartben@gmail.com> wrote:\n>\n>\n> On 2/4/2018 4:38 AM, Nguyễn Thái Ngọc Duy wrote:\n>>\n>> read_directory() code ignores all paths named \".git\" even if it's not\n>> a valid git repository. See treat_path() for details. Since \".git\" is\n>> basically invisible to read_directory(), when we are asked to\n>> invalidate a path that contains \".git\", we can safely ignore it\n>> because the slow path would not consider it anyway.\n>>\n>> This helps when fsmonitor is used and we have a real \".git\" repo at\n>> worktree top. Occasionally .git/index will be updated and if the\n>> fsmonitor hook does not filter it, untracked cache is asked to\n>> invalidate the path \".git/index\".\n>>\n>> Without this patch, we invalidate the root directory unncessarily,\n>> which:\n>>\n>> - makes read_directory() fall back to slow path for root directory\n>>    (slower)\n>>\n>> - makes the index dirty (because UNTR extension is updated). Depending\n>>    on the index size, writing it down could also be slow.\n>>\n>> Noticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n>>   Sorry for the resend, I forgot git@vger.\n>>\n>>   dir.c             | 13 ++++++++++++-\n>>   git-compat-util.h |  2 ++\n>>   wrapper.c         | 12 ++++++++++++\n>>   3 files changed, 26 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/dir.c b/dir.c\n>> index 7c4b45e30e..f8b4cabba9 100644\n>> --- a/dir.c\n>> +++ b/dir.c\n>> @@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct\n>> dir_struct *dir,\n>>         if (!de)\n>>                 return treat_path_fast(dir, untracked, cdir, istate, path,\n>>                                        baselen, pathspec);\n>> -       if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, \".git\"))\n>> +       if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name,\n>> \".git\"))\n>>                 return path_none;\n>>         strbuf_setlen(path, baselen);\n>>         strbuf_addstr(path, de->d_name);\n>> @@ -2970,8 +2970,19 @@ static int invalidate_one_component(struct\n>> untracked_cache *uc,\n>>   void untracked_cache_invalidate_path(struct index_state *istate,\n>>                                      const char *path)\n>>   {\n>> +       const char *end;\n>> +       int skipped;\n>> +\n>>         if (!istate->untracked || !istate->untracked->root)\n>>                 return;\n>\n>\n> Thank you for this patch!  It's great to see people helping improve the\n> performance of git.\n>\n> I ran a quick test and this is not sufficient to prevent the index from\n> getting flagged as dirty and written out to disk when fsmonitor detects\n> changes for files under the .git folder.  What I'm seeing is that when\n> \".git/index\" is returned, the test below doesn't catch them and end early as\n> would be expected.\n\nRight. I focused too much on \".git\" in the middle and the end of the\npath and forgot that it's also at the beginning.\n\n> As a result, invalidate_one_component() is called which calls\n> invalidate_one_directory() which increments dir_invalidated counter and the\n> regular dirty process continues which triggers the index to be written to\n> disk unnecessarily.\n>\n>> +       if (!fspathcmp(path, \".git\"))\n>> +               return;\n>> +       if (ignore_case)\n>> +               skipped = skip_caseprefix(path, \"/.git\", &end);\n>> +       else\n>> +               skipped = skip_prefix(path, \"/.git\", &end);\n>> +       if (skipped && (*end == '\\0' || *end == '/'))\n>> +               return;\n>\n>\n> If I replace the above lines with:\n>\n>         if (ignore_case)\n>                 skipped = skip_caseprefix(path, \".git\", &end);\n>         else\n>                 skipped = skip_prefix(path, \".git\", &end);\n>\n> Then it correctly skips _all_ files under \".git\".  I'm not sure if by\n> removing the leading slash, I'm breaking some other case but I was not able\n> to find where that was expected or needed. Removing the leading slash also\n> allows me to remove the fsmpathcmp() call as it is now redundant.\n\nRemoving \"/\" could catch things like abc/lala.git/def, which\ntreat_path does not consider special and may show up as untracked\nentries. In that sense, the updated invalidate_... is broken if we\ndon't invalidate them properly.\n\nI think what we need here is something like\n\n    if (!fspathcmp(path, \".git\") || starts_with(path, \".git/\"))\n\nbut can handle case-insensitivity as well (starts_with can't).\n\n> I wondered what would happen if there was some other directory named \".git\"\n> and how that would behave.  With or without this patch and with or without\n> the untracked cache, a file \"dir1/.git/foo\" is ignored by git so no change\n> in behavior there either.\n\nThat's what I meant by treat_path() and invisibility in my commit\nmessage. We always ignore directories (or even files) named \".git\". It\ndoes not matter if it contains anything remotely related to git. I'm\nnot saying it's a good thing, but it's how it is and changing that\nrequires a lot more work, possibly some performance degradation if you\nstart to check if \".git\" is a valid repo before ignoring. So I focus\non improving this function alone first.\n-- \nDuy\n"},{"id":"338596","messageId":"20180207092141.4312-2-pclouds@gmail.com","threadId":"47701","inReplyTo":"20180207092141.4312-1-pclouds@gmail.com","subject":"[PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-07T09:21:41Z","receivedAt":"2018-02-07T09:22:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"read_directory() code ignores all paths named \".git\" even if it's not\na valid git repository. See treat_path() for details. Since \".git\" is\nbasically invisible to read_directory(), when we are asked to\ninvalidate a path that contains \".git\", we can safely ignore it\nbecause the slow path would not consider it anyway.\n\nThis helps when fsmonitor is used and we have a real \".git\" repo at\nworktree top. Occasionally .git/index will be updated and if the\nfsmonitor hook does not filter it, untracked cache is asked to\ninvalidate the path \".git/index\".\n\nWithout this patch, we invalidate the root directory unncessarily,\nwhich:\n\n- makes read_directory() fall back to slow path for root directory\n  (slower)\n\n- makes the index dirty (because UNTR extension is updated). Depending\n  on the index size, writing it down could also be slow.\n\nA note about the new \"safe_path\" knob. Since this new check could be\nrelatively expensive, avoid it when we know it's not needed. If the\npath comes from the index, it can't contain \".git\". If it does\ncontain, we may be screwed up at many more levels, not just this one.\n\nNoticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n My v1 was rubbish. It's no wonder Ben didn't see my intention. v2\n corrects the \"is .git in a given path?\" logic and adds a test to\n verify it.\n\n dir.c                       | 10 ++++++----\n dir.h                       |  2 +-\n fsmonitor.c                 |  2 +-\n fsmonitor.h                 |  2 +-\n t/t7519-status-fsmonitor.sh | 39 +++++++++++++++++++++++++++++++++++++\n unpack-trees.c              |  2 +-\n 6 files changed, 49 insertions(+), 8 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7c4b45e30e..fce45fc55e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \tif (!de)\n \t\treturn treat_path_fast(dir, untracked, cdir, istate, path,\n \t\t\t\t       baselen, pathspec);\n-\tif (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, \".git\"))\n+\tif (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, \".git\"))\n \t\treturn path_none;\n \tstrbuf_setlen(path, baselen);\n \tstrbuf_addstr(path, de->d_name);\n@@ -2968,10 +2968,12 @@ static int invalidate_one_component(struct untracked_cache *uc,\n }\n \n void untracked_cache_invalidate_path(struct index_state *istate,\n-\t\t\t\t     const char *path)\n+\t\t\t\t     const char *path, int safe_path)\n {\n \tif (!istate->untracked || !istate->untracked->root)\n \t\treturn;\n+\tif (!safe_path && !verify_path(path))\n+\t\treturn;\n \tinvalidate_one_component(istate->untracked, istate->untracked->root,\n \t\t\t\t path, strlen(path));\n }\n@@ -2979,13 +2981,13 @@ void untracked_cache_invalidate_path(struct index_state *istate,\n void untracked_cache_remove_from_index(struct index_state *istate,\n \t\t\t\t       const char *path)\n {\n-\tuntracked_cache_invalidate_path(istate, path);\n+\tuntracked_cache_invalidate_path(istate, path, 1);\n }\n \n void untracked_cache_add_to_index(struct index_state *istate,\n \t\t\t\t  const char *path)\n {\n-\tuntracked_cache_invalidate_path(istate, path);\n+\tuntracked_cache_invalidate_path(istate, path, 1);\n }\n \n /* Update gitfile and core.worktree setting to connect work tree and git dir */\ndiff --git a/dir.h b/dir.h\nindex 11a047ba48..06df057054 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -350,7 +350,7 @@ static inline int dir_path_match(const struct dir_entry *ent,\n int cmp_dir_entry(const void *p1, const void *p2);\n int check_dir_entry_contains(const struct dir_entry *out, const struct dir_entry *in);\n \n-void untracked_cache_invalidate_path(struct index_state *, const char *);\n+void untracked_cache_invalidate_path(struct index_state *, const char *, int safe_path);\n void untracked_cache_remove_from_index(struct index_state *, const char *);\n void untracked_cache_add_to_index(struct index_state *, const char *);\n \ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 0af7c4edba..6d7bcd5d0e 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -130,7 +130,7 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *n\n \t * as it could be a new untracked file.\n \t */\n \ttrace_printf_key(&trace_fsmonitor, \"fsmonitor_refresh_callback '%s'\", name);\n-\tuntracked_cache_invalidate_path(istate, name);\n+\tuntracked_cache_invalidate_path(istate, name, 0);\n }\n \n void refresh_fsmonitor(struct index_state *istate)\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex cd3cc0ccf2..65f3743636 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -65,7 +65,7 @@ static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cac\n {\n \tif (core_fsmonitor) {\n \t\tce->ce_flags &= ~CE_FSMONITOR_VALID;\n-\t\tuntracked_cache_invalidate_path(istate, ce->name);\n+\t\tuntracked_cache_invalidate_path(istate, ce->name, 1);\n \t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_invalid '%s'\", ce->name);\n \t}\n }\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex eb2d13bbcf..756beb0d8e 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -314,4 +314,43 @@ test_expect_success 'splitting the index results in the same state' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' '\n+\ttest_create_repo dot-git &&\n+\t(\n+\t\tcd dot-git &&\n+\t\tmkdir -p .git/hooks &&\n+\t\t: >tracked &&\n+\t\t: >modified &&\n+\t\tmkdir dir1 &&\n+\t\t: >dir1/tracked &&\n+\t\t: >dir1/modified &&\n+\t\tmkdir dir2 &&\n+\t\t: >dir2/tracked &&\n+\t\t: >dir2/modified &&\n+\t\twrite_integration_script &&\n+\t\tgit config core.fsmonitor .git/hooks/fsmonitor-test &&\n+\t\tgit update-index --untracked-cache &&\n+\t\tgit update-index --fsmonitor &&\n+\t\tGIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace-before\" \\\n+\t\tgit status &&\n+\t\ttest-dump-untracked-cache >../before\n+\t) &&\n+\tcat >>dot-git/.git/hooks/fsmonitor-test <<-\\EOF &&\n+\tprintf \".git\\0\"\n+\tprintf \".git/index\\0\"\n+\tprintf \"dir1/.git\\0\"\n+\tprintf \"dir1/.git/index\\0\"\n+\tEOF\n+\t(\n+\t\tcd dot-git &&\n+\t\tGIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace-after\" \\\n+\t\tgit status &&\n+\t\ttest-dump-untracked-cache >../after\n+\t) &&\n+\tgrep \"directory invalidation\" trace-before >>before &&\n+\tgrep \"directory invalidation\" trace-after >>after &&\n+\t# UNTR extension unchanged, dir invalidation count unchanged\n+\ttest_cmp before after\n+'\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 96c3327f19..9a327696c5 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1506,7 +1506,7 @@ static void invalidate_ce_path(const struct cache_entry *ce,\n \tif (!ce)\n \t\treturn;\n \tcache_tree_invalidate_path(o->src_index, ce->name);\n-\tuntracked_cache_invalidate_path(o->src_index, ce->name);\n+\tuntracked_cache_invalidate_path(o->src_index, ce->name, 1);\n }\n \n /*\n-- \n2.16.1.207.gedba492059\n\n"},{"id":"338597","messageId":"20180207092141.4312-1-pclouds@gmail.com","threadId":"47701","inReplyTo":"20180204093823.3671-1-pclouds@gmail.com","subject":"[PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-07T09:21:40Z","receivedAt":"2018-02-07T09:23:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"read_directory() code ignores all paths named \".git\" even if it's not\na valid git repository. See treat_path() for details. Since \".git\" is\nbasically invisible to read_directory(), when we are asked to\ninvalidate a path that contains \".git\", we can safely ignore it\nbecause the slow path would not consider it anyway.\n\nThis helps when fsmonitor is used and we have a real \".git\" repo at\nworktree top. Occasionally .git/index will be updated and if the\nfsmonitor hook does not filter it, untracked cache is asked to\ninvalidate the path \".git/index\".\n\nWithout this patch, we invalidate the root directory unncessarily,\nwhich:\n\n- makes read_directory() fall back to slow path for root directory\n  (slower)\n\n- makes the index dirty (because UNTR extension is updated). Depending\n  on the index size, writing it down could also be slow.\n\nA note about the new \"safe_path\" knob. Since this new check could be\nrelatively expensive, avoid it when we know it's not needed. If the\npath comes from the index, it can't contain \".git\". If it does\ncontain, we may be screwed up at many more levels, not just this one.\n\nNoticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n My v1 was rubbish. It's no wonder Ben didn't see my intention. v2\n corrects the \"is .git in a given path?\" logic and adds a test to\n verify it.\n\n dir.c                       | 10 ++++++----\n dir.h                       |  2 +-\n fsmonitor.c                 |  2 +-\n fsmonitor.h                 |  2 +-\n t/t7519-status-fsmonitor.sh | 39 +++++++++++++++++++++++++++++++++++++\n unpack-trees.c              |  2 +-\n 6 files changed, 49 insertions(+), 8 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7c4b45e30e..fce45fc55e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \tif (!de)\n \t\treturn treat_path_fast(dir, untracked, cdir, istate, path,\n \t\t\t\t       baselen, pathspec);\n-\tif (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, \".git\"))\n+\tif (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, \".git\"))\n \t\treturn path_none;\n \tstrbuf_setlen(path, baselen);\n \tstrbuf_addstr(path, de->d_name);\n@@ -2968,10 +2968,12 @@ static int invalidate_one_component(struct untracked_cache *uc,\n }\n \n void untracked_cache_invalidate_path(struct index_state *istate,\n-\t\t\t\t     const char *path)\n+\t\t\t\t     const char *path, int safe_path)\n {\n \tif (!istate->untracked || !istate->untracked->root)\n \t\treturn;\n+\tif (!safe_path && !verify_path(path))\n+\t\treturn;\n \tinvalidate_one_component(istate->untracked, istate->untracked->root,\n \t\t\t\t path, strlen(path));\n }\n@@ -2979,13 +2981,13 @@ void untracked_cache_invalidate_path(struct index_state *istate,\n void untracked_cache_remove_from_index(struct index_state *istate,\n \t\t\t\t       const char *path)\n {\n-\tuntracked_cache_invalidate_path(istate, path);\n+\tuntracked_cache_invalidate_path(istate, path, 1);\n }\n \n void untracked_cache_add_to_index(struct index_state *istate,\n \t\t\t\t  const char *path)\n {\n-\tuntracked_cache_invalidate_path(istate, path);\n+\tuntracked_cache_invalidate_path(istate, path, 1);\n }\n \n /* Update gitfile and core.worktree setting to connect work tree and git dir */\ndiff --git a/dir.h b/dir.h\nindex 11a047ba48..06df057054 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -350,7 +350,7 @@ static inline int dir_path_match(const struct dir_entry *ent,\n int cmp_dir_entry(const void *p1, const void *p2);\n int check_dir_entry_contains(const struct dir_entry *out, const struct dir_entry *in);\n \n-void untracked_cache_invalidate_path(struct index_state *, const char *);\n+void untracked_cache_invalidate_path(struct index_state *, const char *, int safe_path);\n void untracked_cache_remove_from_index(struct index_state *, const char *);\n void untracked_cache_add_to_index(struct index_state *, const char *);\n \ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 0af7c4edba..6d7bcd5d0e 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -130,7 +130,7 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *n\n \t * as it could be a new untracked file.\n \t */\n \ttrace_printf_key(&trace_fsmonitor, \"fsmonitor_refresh_callback '%s'\", name);\n-\tuntracked_cache_invalidate_path(istate, name);\n+\tuntracked_cache_invalidate_path(istate, name, 0);\n }\n \n void refresh_fsmonitor(struct index_state *istate)\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex cd3cc0ccf2..65f3743636 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -65,7 +65,7 @@ static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cac\n {\n \tif (core_fsmonitor) {\n \t\tce->ce_flags &= ~CE_FSMONITOR_VALID;\n-\t\tuntracked_cache_invalidate_path(istate, ce->name);\n+\t\tuntracked_cache_invalidate_path(istate, ce->name, 1);\n \t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_invalid '%s'\", ce->name);\n \t}\n }\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex eb2d13bbcf..756beb0d8e 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -314,4 +314,43 @@ test_expect_success 'splitting the index results in the same state' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' '\n+\ttest_create_repo dot-git &&\n+\t(\n+\t\tcd dot-git &&\n+\t\tmkdir -p .git/hooks &&\n+\t\t: >tracked &&\n+\t\t: >modified &&\n+\t\tmkdir dir1 &&\n+\t\t: >dir1/tracked &&\n+\t\t: >dir1/modified &&\n+\t\tmkdir dir2 &&\n+\t\t: >dir2/tracked &&\n+\t\t: >dir2/modified &&\n+\t\twrite_integration_script &&\n+\t\tgit config core.fsmonitor .git/hooks/fsmonitor-test &&\n+\t\tgit update-index --untracked-cache &&\n+\t\tgit update-index --fsmonitor &&\n+\t\tGIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace-before\" \\\n+\t\tgit status &&\n+\t\ttest-dump-untracked-cache >../before\n+\t) &&\n+\tcat >>dot-git/.git/hooks/fsmonitor-test <<-\\EOF &&\n+\tprintf \".git\\0\"\n+\tprintf \".git/index\\0\"\n+\tprintf \"dir1/.git\\0\"\n+\tprintf \"dir1/.git/index\\0\"\n+\tEOF\n+\t(\n+\t\tcd dot-git &&\n+\t\tGIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace-after\" \\\n+\t\tgit status &&\n+\t\ttest-dump-untracked-cache >../after\n+\t) &&\n+\tgrep \"directory invalidation\" trace-before >>before &&\n+\tgrep \"directory invalidation\" trace-after >>after &&\n+\t# UNTR extension unchanged, dir invalidation count unchanged\n+\ttest_cmp before after\n+'\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 96c3327f19..9a327696c5 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1506,7 +1506,7 @@ static void invalidate_ce_path(const struct cache_entry *ce,\n \tif (!ce)\n \t\treturn;\n \tcache_tree_invalidate_path(o->src_index, ce->name);\n-\tuntracked_cache_invalidate_path(o->src_index, ce->name);\n+\tuntracked_cache_invalidate_path(o->src_index, ce->name, 1);\n }\n \n /*\n-- \n2.16.1.207.gedba492059\n\n"},{"id":"338635","messageId":"c755e388-89a5-fc0f-f872-16fd5d5686b0@gmail.com","threadId":"47701","inReplyTo":"20180207092141.4312-2-pclouds@gmail.com","subject":"Re: [PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-02-07T16:59:58Z","receivedAt":"2018-02-07T17:00:11Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 2/7/2018 4:21 AM, Nguyễn Thái Ngọc Duy wrote:\n> read_directory() code ignores all paths named \".git\" even if it's not\n> a valid git repository. See treat_path() for details. Since \".git\" is\n> basically invisible to read_directory(), when we are asked to\n> invalidate a path that contains \".git\", we can safely ignore it\n> because the slow path would not consider it anyway.\n> \n> This helps when fsmonitor is used and we have a real \".git\" repo at\n> worktree top. Occasionally .git/index will be updated and if the\n> fsmonitor hook does not filter it, untracked cache is asked to\n> invalidate the path \".git/index\".\n> \n> Without this patch, we invalidate the root directory unncessarily,\n> which:\n> \n> - makes read_directory() fall back to slow path for root directory\n>    (slower)\n> \n> - makes the index dirty (because UNTR extension is updated). Depending\n>    on the index size, writing it down could also be slow.\n> \n\nThank you again, this patch makes much more sense to me.\n\n> A note about the new \"safe_path\" knob. Since this new check could be\n> relatively expensive, avoid it when we know it's not needed. If the\n> path comes from the index, it can't contain \".git\". If it does\n> contain, we may be screwed up at many more levels, not just this one.\n> \n\nI do have a simplifying suggestion to make.  I noticed that other uses \nof verify_path() check when the potentially erroneous path is passed in \nand then all the underlying code can assume it is valid.  I think that \nmakes sense here as well and it makes for a smaller patch.\n\n\n> diff --git a/fsmonitor.h b/fsmonitor.h\n> index cd3cc0ccf2..65f3743636 100644\n> --- a/fsmonitor.h\n> +++ b/fsmonitor.h\n> @@ -65,7 +65,7 @@ static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cac\n>   {\n>   \tif (core_fsmonitor) {\n>   \t\tce->ce_flags &= ~CE_FSMONITOR_VALID;\n> -\t\tuntracked_cache_invalidate_path(istate, ce->name);\n> +\t\tuntracked_cache_invalidate_path(istate, ce->name, 1);\n\nThis test isn't needed because we're pulling the name right out of the \ncache entry so it doesn't need to be verified.\n\nHere is a modified version of your patch for consideration:\n\n================\n\nread_directory() code ignores all paths named \".git\" even if it's not\na valid git repository. See treat_path() for details. Since \".git\" is\nbasically invisible to read_directory(), when we are asked to\ninvalidate a path that contains \".git\", we can safely ignore it\nbecause the slow path would not consider it anyway.\n\nThis helps when fsmonitor is used and we have a real \".git\" repo at\nworktree top. Occasionally .git/index will be updated and if the\nfsmonitor hook does not filter it, untracked cache is asked to\ninvalidate the path \".git/index\".\n\nWithout this patch, we invalidate the root directory unnecessarily,\nwhich:\n\n- makes read_directory() fall back to slow path for root directory\n   (slower)\n\n- makes the index dirty (because UNTR extension is updated). Depending\n   on the index size, writing it down could also be slow.\n\nNoticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n\nNotes:\n     Base Ref: master\n     Web-Diff: https://github.com/benpeart/git/commit/218a577618\n     Checkout: git fetch https://github.com/benpeart/git verify_path-v1 \n&& git checkout 218a577618\n\n  dir.c                       |  2 +-\n  fsmonitor.c                 |  6 +++++-\n  t/t7519-status-fsmonitor.sh | 39 +++++++++++++++++++++++++++++++++++++++\n  3 files changed, 45 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7c4b45e30e..d431da46f5 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct \ndir_struct *dir,\n  \tif (!de)\n  \t\treturn treat_path_fast(dir, untracked, cdir, istate, path,\n  \t\t\t\t       baselen, pathspec);\n-\tif (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, \".git\"))\n+\tif (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, \".git\"))\n  \t\treturn path_none;\n  \tstrbuf_setlen(path, baselen);\n  \tstrbuf_addstr(path, de->d_name);\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 0af7c4edba..019576f306 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -118,8 +118,12 @@ static int query_fsmonitor(int version, uint64_t \nlast_update, struct strbuf *que\n\n  static void fsmonitor_refresh_callback(struct index_state *istate, \nconst char *name)\n  {\n-\tint pos = index_name_pos(istate, name, strlen(name));\n+\tint pos;\n\n+\tif (!verify_path(name))\n+\t\treturn;\n+\n+\tpos = index_name_pos(istate, name, strlen(name));\n  \tif (pos >= 0) {\n  \t\tstruct cache_entry *ce = istate->cache[pos];\n  \t\tce->ce_flags &= ~CE_FSMONITOR_VALID;\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex eb2d13bbcf..756beb0d8e 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -314,4 +314,43 @@ test_expect_success 'splitting the index results in \nthe same state' '\n  \ttest_cmp expect actual\n  '\n\n+test_expect_success UNTRACKED_CACHE 'ignore .git changes when \ninvalidating UNTR' '\n+\ttest_create_repo dot-git &&\n+\t(\n+\t\tcd dot-git &&\n+\t\tmkdir -p .git/hooks &&\n+\t\t: >tracked &&\n+\t\t: >modified &&\n+\t\tmkdir dir1 &&\n+\t\t: >dir1/tracked &&\n+\t\t: >dir1/modified &&\n+\t\tmkdir dir2 &&\n+\t\t: >dir2/tracked &&\n+\t\t: >dir2/modified &&\n+\t\twrite_integration_script &&\n+\t\tgit config core.fsmonitor .git/hooks/fsmonitor-test &&\n+\t\tgit update-index --untracked-cache &&\n+\t\tgit update-index --fsmonitor &&\n+\t\tGIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace-before\" \\\n+\t\tgit status &&\n+\t\ttest-dump-untracked-cache >../before\n+\t) &&\n+\tcat >>dot-git/.git/hooks/fsmonitor-test <<-\\EOF &&\n+\tprintf \".git\\0\"\n+\tprintf \".git/index\\0\"\n+\tprintf \"dir1/.git\\0\"\n+\tprintf \"dir1/.git/index\\0\"\n+\tEOF\n+\t(\n+\t\tcd dot-git &&\n+\t\tGIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace-after\" \\\n+\t\tgit status &&\n+\t\ttest-dump-untracked-cache >../after\n+\t) &&\n+\tgrep \"directory invalidation\" trace-before >>before &&\n+\tgrep \"directory invalidation\" trace-after >>after &&\n+\t# UNTR extension unchanged, dir invalidation count unchanged\n+\ttest_cmp before after\n+'\n+\n  test_done\n\nbase-commit: 5be1f00a9a701532232f57958efab4be8c959a29\n-- \n2.15.0.windows.1\n\n"},{"id":"339176","messageId":"CACsJy8AHa6QDmWu7TFjgW7m0pHysYkZq_WJ1QOVBVYaT4XdL4Q@mail.gmail.com","threadId":"47701","inReplyTo":"c755e388-89a5-fc0f-f872-16fd5d5686b0@gmail.com","subject":"Re: [PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-13T10:00:50Z","receivedAt":"2018-02-13T10:01:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 7, 2018 at 11:59 PM, Ben Peart <peartben@gmail.com> wrote:\n> diff --git a/dir.c b/dir.c\n> index 7c4b45e30e..d431da46f5 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct\n> dir_struct *dir,\n>         if (!de)\n>                 return treat_path_fast(dir, untracked, cdir, istate, path,\n>                                        baselen, pathspec);\n> -       if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, \".git\"))\n> +       if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, \".git\"))\n>                 return path_none;\n>         strbuf_setlen(path, baselen);\n>         strbuf_addstr(path, de->d_name);\n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index 0af7c4edba..019576f306 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -118,8 +118,12 @@ static int query_fsmonitor(int version, uint64_t\n> last_update, struct strbuf *que\n>\n>  static void fsmonitor_refresh_callback(struct index_state *istate, const\n> char *name)\n>  {\n> -       int pos = index_name_pos(istate, name, strlen(name));\n> +       int pos;\n>\n> +       if (!verify_path(name))\n> +               return;\n> +\n> +       pos = index_name_pos(istate, name, strlen(name));\n>         if (pos >= 0) {\n>                 struct cache_entry *ce = istate->cache[pos];\n>                 ce->ce_flags &= ~CE_FSMONITOR_VALID;\n>\n\nIt's very tempting considering that the amount of changes is much\nsmaller. But I think we should go with my version. The hope is when a\n_new_ call site appears, the author would think twice before passing\nzero or one to the safe_path argument.\n\n> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n> index eb2d13bbcf..756beb0d8e 100755\n> --- a/t/t7519-status-fsmonitor.sh\n> +++ b/t/t7519-status-fsmonitor.sh\n> @@ -314,4 +314,43 @@ test_expect_success 'splitting the index results in the\n> same state' '\n>         test_cmp expect actual\n>  '\n>\n> +test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating\n> UNTR' '\n> +       test_create_repo dot-git &&\n> +       (\n> +               cd dot-git &&\n> +               mkdir -p .git/hooks &&\n> +               : >tracked &&\n> +               : >modified &&\n> +               mkdir dir1 &&\n> +               : >dir1/tracked &&\n> +               : >dir1/modified &&\n> +               mkdir dir2 &&\n> +               : >dir2/tracked &&\n> +               : >dir2/modified &&\n> +               write_integration_script &&\n> +               git config core.fsmonitor .git/hooks/fsmonitor-test &&\n> +               git update-index --untracked-cache &&\n> +               git update-index --fsmonitor &&\n> +               GIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace-before\" \\\n> +               git status &&\n> +               test-dump-untracked-cache >../before\n> +       ) &&\n> +       cat >>dot-git/.git/hooks/fsmonitor-test <<-\\EOF &&\n> +       printf \".git\\0\"\n> +       printf \".git/index\\0\"\n> +       printf \"dir1/.git\\0\"\n> +       printf \"dir1/.git/index\\0\"\n> +       EOF\n> +       (\n> +               cd dot-git &&\n> +               GIT_TRACE_UNTRACKED_STATS=\"$TRASH_DIRECTORY/trace-after\" \\\n> +               git status &&\n> +               test-dump-untracked-cache >../after\n> +       ) &&\n> +       grep \"directory invalidation\" trace-before >>before &&\n> +       grep \"directory invalidation\" trace-after >>after &&\n> +       # UNTR extension unchanged, dir invalidation count unchanged\n> +       test_cmp before after\n> +'\n> +\n>  test_done\n>\n> base-commit: 5be1f00a9a701532232f57958efab4be8c959a29\n> --\n> 2.15.0.windows.1\n>\n\n\n\n-- \nDuy\n"},{"id":"339209","messageId":"xmqqinb0rcw5.fsf@gitster-ct.c.googlers.com","threadId":"47701","inReplyTo":"CACsJy8AHa6QDmWu7TFjgW7m0pHysYkZq_WJ1QOVBVYaT4XdL4Q@mail.gmail.com","subject":"Re: [PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-13T17:57:14Z","receivedAt":"2018-02-13T17:57:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> It's very tempting considering that the amount of changes is much\n> smaller. But I think we should go with my version. The hope is when a\n> _new_ call site appears, the author would think twice before passing\n> zero or one to the safe_path argument.\n\nWouldn't it be a better API if the author of new callsite does not\nhave to think twice and can instead rely on the called function\nuntracked_cache_invalidate_path() to always do the right thing?\n\n\n"},{"id":"339250","messageId":"CACsJy8DYwvNGSWBP-2RNUn9xLBHUGtW1D7EuEYn7q_5f-N9Kvg@mail.gmail.com","threadId":"47701","inReplyTo":"xmqqinb0rcw5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-14T01:24:20Z","receivedAt":"2018-02-14T01:24:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 14, 2018 at 12:57 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> It's very tempting considering that the amount of changes is much\n>> smaller. But I think we should go with my version. The hope is when a\n>> _new_ call site appears, the author would think twice before passing\n>> zero or one to the safe_path argument.\n>\n> Wouldn't it be a better API if the author of new callsite does not\n> have to think twice and can instead rely on the called function\n> untracked_cache_invalidate_path() to always do the right thing?\n\nI am worried that always doing the right thing may carry performance\npenalty (this is based purely on reading verify_path() code, no actual\nbenchmarking). For safety, you can always set safe_path to zero. But\nif you do a lot of invalidation and something starts to slow down,\nthen you can consider setting safe_path to 1 (if it's actually safe to\ndo so). I think we do mass invalidation in some case, so I will try to\nactually benchmark that and see if this safe_path argument is\njustified or if we can always call verify_path().\n-- \nDuy\n"},{"id":"339263","messageId":"CAPc5daU5qjoKkqTvy-xUUP9VAE5KWy0B+CqpujSrabp8MtzqPg@mail.gmail.com","threadId":"47701","inReplyTo":"CACsJy8DYwvNGSWBP-2RNUn9xLBHUGtW1D7EuEYn7q_5f-N9Kvg@mail.gmail.com","subject":"Re: [PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-14T08:00:14Z","receivedAt":"2018-02-14T08:00:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Feb 13, 2018 at 5:24 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> I am worried that always doing the right thing may carry performance\n> penalty (this is based purely on reading verify_path() code, no actual\n> benchmarking). For safety, you can always set safe_path to zero. But\n> if you do a lot of invalidation and something starts to slow down,\n> then you can consider setting safe_path to 1 (if it's actually safe to\n> do so).\n\nFair enough. Thanks for articulating the reasoning.\n"}]}