{"thread":{"id":"47002","subject":"[PATCH 0/4] fsmonitor fixes","startedAt":"2017-10-20T01:12:00Z","lastAt":"2017-10-27T15:13:12Z","messageCount":21,"participants":["Alex Vandiver","Johannes Schindelin","Ben Peart","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"330701","messageId":"20171020011136.14170-1-alexmv@dropbox.com","threadId":"47002","inReplyTo":null,"subject":"[PATCH 0/4] fsmonitor fixes","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-10-20T01:11:32Z","receivedAt":"2017-10-20T01:12:00Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"A few fixes found from playing around with the fsmonitor branch in\nnext.\n - Alex\n\n"},{"id":"330702","messageId":"6887851348e177728849964cc95ae783b0feb971.1508461850.git.alexmv@dropbox.com","threadId":"47002","inReplyTo":"20171020011136.14170-1-alexmv@dropbox.com","subject":"[PATCH 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-10-20T01:11:34Z","receivedAt":"2017-10-20T01:12:03Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"This provides small performance savings.\n\nSigned-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n t/t7519/fsmonitor-watchman                 | 2 +-\n templates/hooks--fsmonitor-watchman.sample | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman\nindex 377edc7be..eba46c78b 100755\n--- a/t/t7519/fsmonitor-watchman\n+++ b/t/t7519/fsmonitor-watchman\n@@ -51,7 +51,7 @@ launch_watchman();\n \n sub launch_watchman {\n \n-\tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j')\n+\tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j --no-pretty')\n \t    or die \"open2() failed: $!\\n\" .\n \t    \"Falling back to scanning...\\n\";\n \ndiff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample\nindex 7df590d29..60eb7e70b 100755\n--- a/templates/hooks--fsmonitor-watchman.sample\n+++ b/templates/hooks--fsmonitor-watchman.sample\n@@ -50,7 +50,7 @@ launch_watchman();\n \n sub launch_watchman {\n \n-\tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j')\n+\tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j --no-pretty')\n \t    or die \"open2() failed: $!\\n\" .\n \t    \"Falling back to scanning...\\n\";\n \n-- \n2.15.0.rc1.417.g05670bb6e\n\n"},{"id":"330703","messageId":"169bbc6117f518ffc9371fea7dfd11439105cc80.1508461850.git.alexmv@dropbox.com","threadId":"47002","inReplyTo":"20171020011136.14170-1-alexmv@dropbox.com","subject":"[PATCH 3/4] fsmonitor: Document GIT_TRACE_FSMONITOR","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-10-20T01:11:35Z","receivedAt":"2017-10-20T01:12:06Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"Signed-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n Documentation/git.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 1fca63634..720db196e 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -594,6 +594,10 @@ into it.\n Unsetting the variable, or setting it to empty, \"0\" or\n \"false\" (case insensitive) disables trace messages.\n \n+`GIT_TRACE_FSMONITOR`::\n+\tEnables trace messages for the filesystem monitor extension.\n+\tSee `GIT_TRACE` for available trace output options.\n+\n `GIT_TRACE_PACK_ACCESS`::\n \tEnables trace messages for all accesses to any packs. For each\n \taccess, the pack file name and an offset in the pack is\n-- \n2.15.0.rc1.417.g05670bb6e\n\n"},{"id":"330704","messageId":"05670bb6e3c6378119b1649144c80dd6d72bfe29.1508461850.git.alexmv@dropbox.com","threadId":"47002","inReplyTo":"20171020011136.14170-1-alexmv@dropbox.com","subject":"[PATCH 4/4] fsmonitor: Delay updating state until after split index is merged","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-10-20T01:11:36Z","receivedAt":"2017-10-20T01:12:10Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"If the fsmonitor extension is used in conjunction with the split index\nextension, the set of entries in the index when it is first loaded is\nonly a subset of the real index.  This leads to only the non-\"base\"\nindex being marked as CE_FSMONITOR_VALID.\n\nDelay the expansion of the ewah bitmap until after tweak_split_index\nhas been called to merge in the base index as well.\n\nSigned-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n cache.h      |  1 +\n fsmonitor.c  | 38 ++++++++++++++++++++++++--------------\n read-cache.c |  4 ++++\n 3 files changed, 29 insertions(+), 14 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 25adcf681..0a4f43ec2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -348,6 +348,7 @@ struct index_state {\n \tunsigned char sha1[20];\n \tstruct untracked_cache *untracked;\n \tuint64_t fsmonitor_last_update;\n+\tstruct ewah_bitmap *fsmonitor_dirty;\n };\n \n extern struct index_state the_index;\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 7c1540c05..4c2668f57 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -49,20 +49,7 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \t\tewah_free(fsmonitor_dirty);\n \t\treturn error(\"failed to parse ewah bitmap reading fsmonitor index extension\");\n \t}\n-\n-\tif (git_config_get_fsmonitor()) {\n-\t\t/* Mark all entries valid */\n-\t\tfor (i = 0; i < istate->cache_nr; i++)\n-\t\t\tistate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;\n-\n-\t\t/* Mark all previously saved entries as dirty */\n-\t\tewah_each_bit(fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n-\n-\t\t/* Now mark the untracked cache for fsmonitor usage */\n-\t\tif (istate->untracked)\n-\t\t\tistate->untracked->use_fsmonitor = 1;\n-\t}\n-\tewah_free(fsmonitor_dirty);\n+\tistate->fsmonitor_dirty = fsmonitor_dirty;\n \n \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n \treturn 0;\n@@ -238,6 +225,29 @@ void remove_fsmonitor(struct index_state *istate)\n \n void tweak_fsmonitor(struct index_state *istate)\n {\n+\tint i;\n+\n+\tif (istate->fsmonitor_dirty) {\n+\t\t/* Mark all entries valid */\n+\t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor is enabled; cache is %d\", istate->cache_nr);\n+\t\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\t\tistate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;\n+\t\t}\n+\t\ttrace_printf_key(&trace_fsmonitor, \"marked all as valid\");\n+\n+\t\t/* Mark all previously saved entries as dirty */\n+\t\ttrace_printf_key(&trace_fsmonitor, \"setting each bit on %p\", istate->fsmonitor_dirty);\n+\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n+\n+\t\t/* Now mark the untracked cache for fsmonitor usage */\n+\t\ttrace_printf_key(&trace_fsmonitor, \"setting untracked state\");\n+\t\tif (istate->untracked)\n+\t\t\tistate->untracked->use_fsmonitor = 1;\n+\t\tewah_free(istate->fsmonitor_dirty);\n+\t} else {\n+\t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor not enabled\");\n+\t}\n+\n \tswitch (git_config_get_fsmonitor()) {\n \tcase -1: /* keep: do nothing */\n \t\tbreak;\ndiff --git a/read-cache.c b/read-cache.c\nindex c18e5e623..3b5cd00a2 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -330,6 +330,10 @@ int ie_match_stat(struct index_state *istate,\n \t\treturn 0;\n \tif (!ignore_fsmonitor && (ce->ce_flags & CE_FSMONITOR_VALID))\n \t\treturn 0;\n+\tif (ce->ce_flags & CE_FSMONITOR_VALID)\n+\t\ttrace_printf_key(&trace_fsmonitor, \"fsmon marked valid for %s\", ce->name);\n+\tif (ignore_fsmonitor)\n+\t\ttrace_printf_key(&trace_fsmonitor, \"Ignoring fsmonitor for %s\", ce->name);\n \n \t/*\n \t * Intent-to-add entries have not been added, so the index entry\n-- \n2.15.0.rc1.417.g05670bb6e\n\n"},{"id":"330705","messageId":"4f8e3dab26cf50cc6aa055605784680f5c33fcfa.1508461850.git.alexmv@dropbox.com","threadId":"47002","inReplyTo":"20171020011136.14170-1-alexmv@dropbox.com","subject":"[PATCH 1/4] fsmonitor: Watch, and ask about, the top of the repo, not the CWD","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-10-20T01:11:33Z","receivedAt":"2017-10-20T01:12:12Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"Signed-off-by: Alex Vandiver <alexmv@dropbox.com>\n---\n t/t7519/fsmonitor-watchman                 | 3 ++-\n templates/hooks--fsmonitor-watchman.sample | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman\nindex a3e30bf54..377edc7be 100755\n--- a/t/t7519/fsmonitor-watchman\n+++ b/t/t7519/fsmonitor-watchman\n@@ -41,7 +41,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {\n \t$git_work_tree =~ s/[\\r\\n]+//g;\n \t$git_work_tree =~ s,\\\\,/,g;\n } else {\n-\t$git_work_tree = $ENV{'PWD'};\n+\t$git_work_tree = `git rev-parse --show-toplevel`;\n+\tchomp $git_work_tree;\n }\n \n my $retry = 1;\ndiff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample\nindex 9eba8a740..7df590d29 100755\n--- a/templates/hooks--fsmonitor-watchman.sample\n+++ b/templates/hooks--fsmonitor-watchman.sample\n@@ -40,7 +40,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {\n \t$git_work_tree =~ s/[\\r\\n]+//g;\n \t$git_work_tree =~ s,\\\\,/,g;\n } else {\n-\t$git_work_tree = $ENV{'PWD'};\n+\t$git_work_tree = `git rev-parse --show-toplevel`;\n+\tchomp $git_work_tree;\n }\n \n my $retry = 1;\n-- \n2.15.0.rc1.417.g05670bb6e\n\n"},{"id":"330726","messageId":"alpine.DEB.2.21.1.1710201457180.40514@virtualbox","threadId":"47002","inReplyTo":"4f8e3dab26cf50cc6aa055605784680f5c33fcfa.1508461850.git.alexmv@dropbox.com","subject":"Re: [PATCH 1/4] fsmonitor: Watch, and ask about, the top of the repo, not the CWD","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-20T12:58:40Z","receivedAt":"2017-10-20T12:58:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alex,\n\nOn Thu, 19 Oct 2017, Alex Vandiver wrote:\n\n> Signed-off-by: Alex Vandiver <alexmv@dropbox.com>\n> ---\n>  t/t7519/fsmonitor-watchman                 | 3 ++-\n>  templates/hooks--fsmonitor-watchman.sample | 3 ++-\n>  2 files changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman\n> index a3e30bf54..377edc7be 100755\n> --- a/t/t7519/fsmonitor-watchman\n> +++ b/t/t7519/fsmonitor-watchman\n> @@ -41,7 +41,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {\n>  \t$git_work_tree =~ s/[\\r\\n]+//g;\n>  \t$git_work_tree =~ s,\\\\,/,g;\n>  } else {\n> -\t$git_work_tree = $ENV{'PWD'};\n> +\t$git_work_tree = `git rev-parse --show-toplevel`;\n> +\tchomp $git_work_tree;\n\nThis is super expensive, as it means a full-blown new process instead of\njust a simple environment variable expansion.\n\nThe idea behind using `PWD` instead was that Git will already have done\nall of the work of figuring out the top-level directory and switched to\nthere before calling the fsmonitor hook.\n\nDid you see any case where the script was *not* called from the top-level\ndirectory?\n\nCiao,\nJohannes\n"},{"id":"330727","messageId":"alpine.DEB.2.21.1.1710201459020.40514@virtualbox","threadId":"47002","inReplyTo":"6887851348e177728849964cc95ae783b0feb971.1508461850.git.alexmv@dropbox.com","subject":"Re: [PATCH 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-20T13:00:40Z","receivedAt":"2017-10-20T13:00:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alex,\n\nOn Thu, 19 Oct 2017, Alex Vandiver wrote:\n\n> This provides small performance savings.\n> \n> diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman\n> index 377edc7be..eba46c78b 100755\n> --- a/t/t7519/fsmonitor-watchman\n> +++ b/t/t7519/fsmonitor-watchman\n> @@ -51,7 +51,7 @@ launch_watchman();\n>  \n>  sub launch_watchman {\n>  \n> -\tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j')\n> +\tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j --no-pretty')\n\nWhile I am very much infavor of this change (I was not aware of the\n--no-pretty option), I would like to see some statistics on that. Could\nyou measure the impact, please, and include the results in the commit\nmessage?\n\nCiao,\nJohannes\n"},{"id":"330729","messageId":"alpine.DEB.2.21.1.1710201503380.40514@virtualbox","threadId":"47002","inReplyTo":"05670bb6e3c6378119b1649144c80dd6d72bfe29.1508461850.git.alexmv@dropbox.com","subject":"Re: [PATCH 4/4] fsmonitor: Delay updating state until after split index is merged","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-20T13:16:20Z","receivedAt":"2017-10-20T13:16:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alex,\n\nOn Thu, 19 Oct 2017, Alex Vandiver wrote:\n\n>  extern struct index_state the_index;\n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index 7c1540c05..4c2668f57 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -49,20 +49,7 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n>  \t\tewah_free(fsmonitor_dirty);\n>  \t\treturn error(\"failed to parse ewah bitmap reading fsmonitor index extension\");\n>  \t}\n> -\n> -\tif (git_config_get_fsmonitor()) {\n> -\t\t/* Mark all entries valid */\n> -\t\tfor (i = 0; i < istate->cache_nr; i++)\n> -\t\t\tistate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;\n> -\n> -\t\t/* Mark all previously saved entries as dirty */\n> -\t\tewah_each_bit(fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n> -\n> -\t\t/* Now mark the untracked cache for fsmonitor usage */\n> -\t\tif (istate->untracked)\n> -\t\t\tistate->untracked->use_fsmonitor = 1;\n> -\t}\n> -\tewah_free(fsmonitor_dirty);\n> +\tistate->fsmonitor_dirty = fsmonitor_dirty;\n\nFrom the diff, it is not immediately clear that fsmonitor_dirty is not\nleaked in any code path.\n\nCould you clarify this in the commit message, please?\n\n> @@ -238,6 +225,29 @@ void remove_fsmonitor(struct index_state *istate)\n>  \n>  void tweak_fsmonitor(struct index_state *istate)\n>  {\n> +\tint i;\n> +\n> +\tif (istate->fsmonitor_dirty) {\n> +\t\t/* Mark all entries valid */\n> +\t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor is enabled; cache is %d\", istate->cache_nr);\n\nSadly, a call to trace_printf_key() is not really a noop when tracing is\ndisabled. The call to trace_printf_key() hands off to trace_vprintf_fl(),\nwhich in turn calls prepare_trace_line() which asks trace_want() whether\nwe need to trace, which finally calls get_trace_fd(). This last function\ninitializes a trace key if needed, and this entire call stack takes time.\n\nIn this case, where we trace whether fsmonitor is enabled essentially once\nduring the life cycle of the current Git command invocation, it may be\nokay, but later we perform a trace for every single ie_match_stat() call,\nwhich I think should be guarded to avoid unnecessary code churn in\nperformance critical code paths (O(N) is pretty good until the constant\nfactor becomes large).\n\n> +\t\tfor (i = 0; i < istate->cache_nr; i++) {\n> +\t\t\tistate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;\n> +\t\t}\n> +\t\ttrace_printf_key(&trace_fsmonitor, \"marked all as valid\");\n> +\n> +\t\t/* Mark all previously saved entries as dirty */\n> +\t\ttrace_printf_key(&trace_fsmonitor, \"setting each bit on %p\", istate->fsmonitor_dirty);\n> +\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n> +\n> +\t\t/* Now mark the untracked cache for fsmonitor usage */\n> +\t\ttrace_printf_key(&trace_fsmonitor, \"setting untracked state\");\n> +\t\tif (istate->untracked)\n> +\t\t\tistate->untracked->use_fsmonitor = 1;\n> +\t\tewah_free(istate->fsmonitor_dirty);\n\nAt this point, please set istate->fsmonitor_dirty = NULL, as it is not\nimmediately obvious from this patch (or from the postimage of this diff)\nthat the array is not used later on.\n\n> +\t} else {\n> +\t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor not enabled\");\n> +\t}\n> +\n>  \tswitch (git_config_get_fsmonitor()) {\n>  \tcase -1: /* keep: do nothing */\n>  \t\tbreak;\n> diff --git a/read-cache.c b/read-cache.c\n> index c18e5e623..3b5cd00a2 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -330,6 +330,10 @@ int ie_match_stat(struct index_state *istate,\n>  \t\treturn 0;\n>  \tif (!ignore_fsmonitor && (ce->ce_flags & CE_FSMONITOR_VALID))\n>  \t\treturn 0;\n> +\tif (ce->ce_flags & CE_FSMONITOR_VALID)\n> +\t\ttrace_printf_key(&trace_fsmonitor, \"fsmon marked valid for %s\", ce->name);\n> +\tif (ignore_fsmonitor)\n> +\t\ttrace_printf_key(&trace_fsmonitor, \"Ignoring fsmonitor for %s\", ce->name);\n\nThis is the code path I am fairly certain should not be penalized if\ntracing is disabled.\n\nCiao,\nJohannes\n"},{"id":"330730","messageId":"c43315f3-4155-6b07-bde2-5855dceacf80@gmail.com","threadId":"47002","inReplyTo":"alpine.DEB.2.21.1.1710201459020.40514@virtualbox","subject":"Re: [PATCH 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-10-20T13:17:14Z","receivedAt":"2017-10-20T13:17:21Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 10/20/2017 9:00 AM, Johannes Schindelin wrote:\n> Hi Alex,\n> \n> On Thu, 19 Oct 2017, Alex Vandiver wrote:\n> \n>> This provides small performance savings.\n>>\n>> diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman\n>> index 377edc7be..eba46c78b 100755\n>> --- a/t/t7519/fsmonitor-watchman\n>> +++ b/t/t7519/fsmonitor-watchman\n>> @@ -51,7 +51,7 @@ launch_watchman();\n>>   \n>>   sub launch_watchman {\n>>   \n>> -\tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j')\n>> +\tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j --no-pretty')\n> \n> While I am very much infavor of this change (I was not aware of the\n> --no-pretty option), I would like to see some statistics on that. Could\n> you measure the impact, please, and include the results in the commit\n> message?\n> \n> Ciao,\n> Johannes\n> \n\nI was also unaware of the --no-pretty option. I've tested this on \nWindows running version 4.9.0 of Watchman and verified that it does work \ncorrectly.  I'm also curious if it produces any measurable difference in \nperformance.\n"},{"id":"330731","messageId":"alpine.DEB.2.21.1.1710201516320.40514@virtualbox","threadId":"47002","inReplyTo":"20171020011136.14170-1-alexmv@dropbox.com","subject":"Re: [PATCH 0/4] fsmonitor fixes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-20T13:17:12Z","receivedAt":"2017-10-20T13:17:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alex,\n\nOn Thu, 19 Oct 2017, Alex Vandiver wrote:\n\n> A few fixes found from playing around with the fsmonitor branch in\n> next.\n\nThank you for having a look, and even better: for sending a couple of\nimprovements on top of Ben's & my work.\n\nI sent a couple of suggestions and hope you will find the useful?\n\nCiao,\nJohannes\n"},{"id":"330732","messageId":"2ef8f550-610e-6a9f-32e8-f2fe99b044ff@gmail.com","threadId":"47002","inReplyTo":"169bbc6117f518ffc9371fea7dfd11439105cc80.1508461850.git.alexmv@dropbox.com","subject":"Re: [PATCH 3/4] fsmonitor: Document GIT_TRACE_FSMONITOR","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-10-20T13:19:04Z","receivedAt":"2017-10-20T13:19:10Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 10/19/2017 9:11 PM, Alex Vandiver wrote:\n> Signed-off-by: Alex Vandiver <alexmv@dropbox.com>\n> ---\n>   Documentation/git.txt | 4 ++++\n>   1 file changed, 4 insertions(+)\n> \n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 1fca63634..720db196e 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -594,6 +594,10 @@ into it.\n>   Unsetting the variable, or setting it to empty, \"0\" or\n>   \"false\" (case insensitive) disables trace messages.\n>   \n> +`GIT_TRACE_FSMONITOR`::\n> +\tEnables trace messages for the filesystem monitor extension.\n> +\tSee `GIT_TRACE` for available trace output options.\n> +\n>   `GIT_TRACE_PACK_ACCESS`::\n>   \tEnables trace messages for all accesses to any packs. For each\n>   \taccess, the pack file name and an offset in the pack is\n> \n\nLooks like a reasonable addition.  Thank you.\n"},{"id":"330746","messageId":"7c6bfaef-d05a-4c06-5a53-4f7811c81aec@gmail.com","threadId":"47002","inReplyTo":"alpine.DEB.2.21.1.1710201503380.40514@virtualbox","subject":"Re: [PATCH 4/4] fsmonitor: Delay updating state until after split index is merged","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-10-20T21:47:50Z","receivedAt":"2017-10-20T21:47:57Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 10/20/2017 9:16 AM, Johannes Schindelin wrote:\n> Hi Alex,\n> \n> On Thu, 19 Oct 2017, Alex Vandiver wrote:\n> \n>>   extern struct index_state the_index;\n>> diff --git a/fsmonitor.c b/fsmonitor.c\n>> index 7c1540c05..4c2668f57 100644\n>> --- a/fsmonitor.c\n>> +++ b/fsmonitor.c\n>> @@ -49,20 +49,7 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n>>   \t\tewah_free(fsmonitor_dirty);\n>>   \t\treturn error(\"failed to parse ewah bitmap reading fsmonitor index extension\");\n>>   \t}\n>> -\n>> -\tif (git_config_get_fsmonitor()) {\n>> -\t\t/* Mark all entries valid */\n>> -\t\tfor (i = 0; i < istate->cache_nr; i++)\n>> -\t\t\tistate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;\n>> -\n>> -\t\t/* Mark all previously saved entries as dirty */\n>> -\t\tewah_each_bit(fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n>> -\n>> -\t\t/* Now mark the untracked cache for fsmonitor usage */\n>> -\t\tif (istate->untracked)\n>> -\t\t\tistate->untracked->use_fsmonitor = 1;\n>> -\t}\n>> -\tewah_free(fsmonitor_dirty);\n>> +\tistate->fsmonitor_dirty = fsmonitor_dirty;\n> \n>  From the diff, it is not immediately clear that fsmonitor_dirty is not\n> leaked in any code path.\n> \n> Could you clarify this in the commit message, please?\n> \n>> @@ -238,6 +225,29 @@ void remove_fsmonitor(struct index_state *istate)\n>>   \n>>   void tweak_fsmonitor(struct index_state *istate)\n>>   {\n>> +\tint i;\n>> +\n\nHere we should test git_config_get_fsmonitor() (see the logic where you \nmoved this from) as there is no reason to set up the fsmonitor state if \nwe're about to remove the extension.  Save the results and use them in \nthe case statement below.\n\n>> +\tif (istate->fsmonitor_dirty) {\n>> +\t\t/* Mark all entries valid */\n>> +\t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor is enabled; cache is %d\", istate->cache_nr);\n> \n> Sadly, a call to trace_printf_key() is not really a noop when tracing is\n> disabled. The call to trace_printf_key() hands off to trace_vprintf_fl(),\n> which in turn calls prepare_trace_line() which asks trace_want() whether\n> we need to trace, which finally calls get_trace_fd(). This last function\n> initializes a trace key if needed, and this entire call stack takes time.\n> \n> In this case, where we trace whether fsmonitor is enabled essentially once\n> during the life cycle of the current Git command invocation, it may be\n> okay, but later we perform a trace for every single ie_match_stat() call,\n> which I think should be guarded to avoid unnecessary code churn in\n> performance critical code paths (O(N) is pretty good until the constant\n> factor becomes large).\n> \n>> +\t\tfor (i = 0; i < istate->cache_nr; i++) {\n>> +\t\t\tistate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;\n>> +\t\t}\n>> +\t\ttrace_printf_key(&trace_fsmonitor, \"marked all as valid\");\n>> +\n>> +\t\t/* Mark all previously saved entries as dirty */\n>> +\t\ttrace_printf_key(&trace_fsmonitor, \"setting each bit on %p\", istate->fsmonitor_dirty);\n>> +\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n>> +\n>> +\t\t/* Now mark the untracked cache for fsmonitor usage */\n>> +\t\ttrace_printf_key(&trace_fsmonitor, \"setting untracked state\");\n>> +\t\tif (istate->untracked)\n>> +\t\t\tistate->untracked->use_fsmonitor = 1;\n>> +\t\tewah_free(istate->fsmonitor_dirty);\n> \n> At this point, please set istate->fsmonitor_dirty = NULL, as it is not\n> immediately obvious from this patch (or from the postimage of this diff)\n> that the array is not used later on.\n> \n>> +\t} else {\n>> +\t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor not enabled\");\n>> +\t}\n>> +\n\nI'd remove the trace statement above as it isn't always accurate. \nfsmonitor could be enabled but just hasn't written/read the extension yet.\n\n>>   \tswitch (git_config_get_fsmonitor()) {\n>>   \tcase -1: /* keep: do nothing */\n>>   \t\tbreak;\n>> diff --git a/read-cache.c b/read-cache.c\n>> index c18e5e623..3b5cd00a2 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -330,6 +330,10 @@ int ie_match_stat(struct index_state *istate,\n>>   \t\treturn 0;\n>>   \tif (!ignore_fsmonitor && (ce->ce_flags & CE_FSMONITOR_VALID))\n>>   \t\treturn 0;\n>> +\tif (ce->ce_flags & CE_FSMONITOR_VALID)\n>> +\t\ttrace_printf_key(&trace_fsmonitor, \"fsmon marked valid for %s\", ce->name);\n>> +\tif (ignore_fsmonitor)\n>> +\t\ttrace_printf_key(&trace_fsmonitor, \"Ignoring fsmonitor for %s\", ce->name);\n> \n> This is the code path I am fairly certain should not be penalized if\n> tracing is disabled.\n> \n\nDefinitely agree with the need to remove this tracing as it will get \ncalled a lot and we don't want to pay the perf penalty.\n\n> Ciao,\n> Johannes\n> \n\nThank you for detecting the issue with split index and even better, \nsending patches to fix it!\n"},{"id":"330762","messageId":"xmqq376dgsq4.fsf@gitster.mtv.corp.google.com","threadId":"47002","inReplyTo":"7c6bfaef-d05a-4c06-5a53-4f7811c81aec@gmail.com","subject":"Re: [PATCH 4/4] fsmonitor: Delay updating state until after split index is merged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-21T02:06:11Z","receivedAt":"2017-10-21T02:06:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <peartben@gmail.com> writes:\n\n>>> +\t} else {\n>>> +\t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor not enabled\");\n>>> +\t}\n>>> +\n>\n> I'd remove the trace statement above as it isn't always\n> accurate. fsmonitor could be enabled but just hasn't written/read the\n> extension yet.\n\nI agree; when it is not enabled, we shouldn't be paying the penalty,\neither.  I wonder if tweak_*() function can return early upfront if\nwe know fsmonitor is not enabled to make it even more obvious.\n\n>>> +\tif (ignore_fsmonitor)\n>>> +\t\ttrace_printf_key(&trace_fsmonitor, \"Ignoring fsmonitor for %s\", ce->name);\n>>\n>> This is the code path I am fairly certain should not be penalized if\n>> tracing is disabled.\n>\n> Definitely agree with the need to remove this tracing as it will get\n> called a lot and we don't want to pay the perf penalty.\n\nYes.\n"},{"id":"330773","messageId":"20171021033534.pgjtyiwlhvheuueo@sigill.intra.peff.net","threadId":"47002","inReplyTo":"alpine.DEB.2.21.1.1710201503380.40514@virtualbox","subject":"Re: [PATCH 4/4] fsmonitor: Delay updating state until after split index is merged","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-21T03:35:34Z","receivedAt":"2017-10-21T03:35:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 20, 2017 at 03:16:20PM +0200, Johannes Schindelin wrote:\n\n> >  void tweak_fsmonitor(struct index_state *istate)\n> >  {\n> > +\tint i;\n> > +\n> > +\tif (istate->fsmonitor_dirty) {\n> > +\t\t/* Mark all entries valid */\n> > +\t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor is enabled; cache is %d\", istate->cache_nr);\n> \n> Sadly, a call to trace_printf_key() is not really a noop when tracing is\n> disabled. The call to trace_printf_key() hands off to trace_vprintf_fl(),\n> which in turn calls prepare_trace_line() which asks trace_want() whether\n> we need to trace, which finally calls get_trace_fd(). This last function\n> initializes a trace key if needed, and this entire call stack takes time.\n\nIt seems like we could pretty easily turn noop traces into a trivial\nconditional, like:\n\ndiff --git a/trace.h b/trace.h\nindex 179b249c59..c46b92cbde 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -80,8 +80,11 @@ extern void trace_performance_since(uint64_t start, const char *format, ...);\n #define trace_printf(...) \\\n \ttrace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)\n \n-#define trace_printf_key(key, ...) \\\n-\ttrace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__)\n+#define trace_printf_key(key, ...) do { \\\n+\tif (!key->initialized || key->fd) \\\n+\t\ttrace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__) \\\n+} while(0)\n+\n \n #define trace_argv_printf(argv, ...) \\\n \ttrace_argv_printf_fl(TRACE_CONTEXT, __LINE__, argv, __VA_ARGS__)\n\n\n(OK, that's got an OR, but if we are really pinching instructions we\ncould obviously store a single \"I've been initialized and am disabled\"\nflag).\n\nI don't have an opinion one way or the other on these particular\nmessages, but in general I'd like to see _more_ tracing in Git, not\nless. I've often traced Git with a debugger or other tools like perf,\nbut there's real value in the author of code annotating high-level\nlogical events.\n\n-Peff\n"},{"id":"330826","messageId":"alpine.DEB.2.21.1.1710231157030.6482@virtualbox","threadId":"47002","inReplyTo":"20171021033534.pgjtyiwlhvheuueo@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] fsmonitor: Delay updating state until after split index is merged","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-23T09:57:32Z","receivedAt":"2017-10-23T09:57:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 20 Oct 2017, Jeff King wrote:\n\n> On Fri, Oct 20, 2017 at 03:16:20PM +0200, Johannes Schindelin wrote:\n> \n> > >  void tweak_fsmonitor(struct index_state *istate)\n> > >  {\n> > > +\tint i;\n> > > +\n> > > +\tif (istate->fsmonitor_dirty) {\n> > > +\t\t/* Mark all entries valid */\n> > > +\t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor is enabled; cache is %d\", istate->cache_nr);\n> > \n> > Sadly, a call to trace_printf_key() is not really a noop when tracing is\n> > disabled. The call to trace_printf_key() hands off to trace_vprintf_fl(),\n> > which in turn calls prepare_trace_line() which asks trace_want() whether\n> > we need to trace, which finally calls get_trace_fd(). This last function\n> > initializes a trace key if needed, and this entire call stack takes time.\n> \n> It seems like we could pretty easily turn noop traces into a trivial\n> conditional, like:\n> \n> diff --git a/trace.h b/trace.h\n> index 179b249c59..c46b92cbde 100644\n> --- a/trace.h\n> +++ b/trace.h\n> @@ -80,8 +80,11 @@ extern void trace_performance_since(uint64_t start, const char *format, ...);\n>  #define trace_printf(...) \\\n>  \ttrace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)\n>  \n> -#define trace_printf_key(key, ...) \\\n> -\ttrace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__)\n> +#define trace_printf_key(key, ...) do { \\\n> +\tif (!key->initialized || key->fd) \\\n> +\t\ttrace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__) \\\n> +} while(0)\n> +\n>  \n>  #define trace_argv_printf(argv, ...) \\\n>  \ttrace_argv_printf_fl(TRACE_CONTEXT, __LINE__, argv, __VA_ARGS__)\n> \n> \n> (OK, that's got an OR, but if we are really pinching instructions we\n> could obviously store a single \"I've been initialized and am disabled\"\n> flag).\n\nI'd really like that.\n\n> I don't have an opinion one way or the other on these particular\n> messages, but in general I'd like to see _more_ tracing in Git, not\n> less. I've often traced Git with a debugger or other tools like perf,\n> but there's real value in the author of code annotating high-level\n> logical events.\n\nYes, I like that idea, too: to add more tracing.\n\nThanks,\nDscho\n"},{"id":"330834","messageId":"541ed27f-3219-fe0a-beb6-99ecb4ed36da@gmail.com","threadId":"47002","inReplyTo":"alpine.DEB.2.21.1.1710231157030.6482@virtualbox","subject":"Re: [PATCH 4/4] fsmonitor: Delay updating state until after split index is merged","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-10-23T12:36:19Z","receivedAt":"2017-10-23T12:36:27Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 10/23/2017 5:57 AM, Johannes Schindelin wrote:\n> Hi Peff,\n> \n> On Fri, 20 Oct 2017, Jeff King wrote:\n> \n>> On Fri, Oct 20, 2017 at 03:16:20PM +0200, Johannes Schindelin wrote:\n>>\n>>>>   void tweak_fsmonitor(struct index_state *istate)\n>>>>   {\n>>>> +\tint i;\n>>>> +\n>>>> +\tif (istate->fsmonitor_dirty) {\n>>>> +\t\t/* Mark all entries valid */\n>>>> +\t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor is enabled; cache is %d\", istate->cache_nr);\n>>>\n>>> Sadly, a call to trace_printf_key() is not really a noop when tracing is\n>>> disabled. The call to trace_printf_key() hands off to trace_vprintf_fl(),\n>>> which in turn calls prepare_trace_line() which asks trace_want() whether\n>>> we need to trace, which finally calls get_trace_fd(). This last function\n>>> initializes a trace key if needed, and this entire call stack takes time.\n>>\n>> It seems like we could pretty easily turn noop traces into a trivial\n>> conditional, like:\n>>\n>> diff --git a/trace.h b/trace.h\n>> index 179b249c59..c46b92cbde 100644\n>> --- a/trace.h\n>> +++ b/trace.h\n>> @@ -80,8 +80,11 @@ extern void trace_performance_since(uint64_t start, const char *format, ...);\n>>   #define trace_printf(...) \\\n>>   \ttrace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)\n>>   \n>> -#define trace_printf_key(key, ...) \\\n>> -\ttrace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__)\n>> +#define trace_printf_key(key, ...) do { \\\n>> +\tif (!key->initialized || key->fd) \\\n>> +\t\ttrace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__) \\\n>> +} while(0)\n>> +\n>>   \n>>   #define trace_argv_printf(argv, ...) \\\n>>   \ttrace_argv_printf_fl(TRACE_CONTEXT, __LINE__, argv, __VA_ARGS__)\n>>\n>>\n>> (OK, that's got an OR, but if we are really pinching instructions we\n>> could obviously store a single \"I've been initialized and am disabled\"\n>> flag).\n> \n> I'd really like that.\n> \n>> I don't have an opinion one way or the other on these particular\n>> messages, but in general I'd like to see _more_ tracing in Git, not\n>> less. I've often traced Git with a debugger or other tools like perf,\n>> but there's real value in the author of code annotating high-level\n>> logical events.\n> \n> Yes, I like that idea, too: to add more tracing.\n> \n> Thanks,\n> Dscho\n> \n\nTwo big thumbs up.\n\nI find turning on tracking very informative as I can better see the flow \nof execution and sometimes am surprised by what I discover. :)\n\nI've often added additional tracing while working on a feature only to \nremove it before submitting the patch because of the performance \noverhead.  Being able to leave that in would be very helpful.\n"},{"id":"331048","messageId":"alpine.DEB.2.10.1710251700300.9817@alexmv-linux","threadId":"47002","inReplyTo":"alpine.DEB.2.21.1.1710201457180.40514@virtualbox","subject":"Re: [PATCH 1/4] fsmonitor: Watch, and ask about, the top of the repo, not the CWD","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-10-26T00:20:33Z","receivedAt":"2017-10-26T00:20:47Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"On Fri, 20 Oct 2017, Johannes Schindelin wrote:\n> This is super expensive, as it means a full-blown new process instead of\n> just a simple environment variable expansion.\n> \n> The idea behind using `PWD` instead was that Git will already have done\n> all of the work of figuring out the top-level directory and switched to\n> there before calling the fsmonitor hook.\n\nI'm not seeing that PWD has been at all altered.  The following does\nseem like a better solution:\n------8<-----\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 7c1540c05..4ea44dcc6 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n        argv[3] = NULL;\n        cp.argv = argv;\n        cp.use_shell = 1;\n+       cp.dir = get_git_work_tree();\n\n        return capture_command(&cp, query_result, 1024);\n }\n------8<-----\n\nI'll re-roll with that.\n\n> Did you see any case where the script was *not* called from the top-level\n> directory?\n\nMerely calling `git status` inside a subdirectory is enough to for the\nstock watchman config to report that it's in a \"new\" directory:\n\n    $ watchman watch-list\n    {\n        \"roots\": [],\n        \"version\": \"4.7.0\"\n    }\n    $ git status\n    Adding '/Users/alexmv/src/git' to watchman's watch list.\n    On branch test\n    nothing to commit, working tree clean\n    $ cd builtin/\n    $ git status\n    Adding '/Users/alexmv/src/git/builtin' to watchman's watch list.\n    On branch test\n    nothing to commit, working tree clean\n    $ watchman watch-list\n    {\n        \"roots\": [\n            \"/Users/alexmv/src/git/builtin\",\n            \"/Users/alexmv/src/git\"\n        ],\n        \"version\": \"4.7.0\"\n    }\n\nAs I understand it, that means that it then loses all performance\ngains in the new directory, as it spits out \"all dirty.\"\n\n - Alex\n"},{"id":"331049","messageId":"alpine.DEB.2.10.1710251722310.9817@alexmv-linux","threadId":"47002","inReplyTo":"c43315f3-4155-6b07-bde2-5855dceacf80@gmail.com","subject":"Re: [PATCH 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-10-26T00:44:10Z","receivedAt":"2017-10-26T00:44:25Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"On Fri, 20 Oct 2017, Ben Peart wrote:\n> > While I am very much infavor of this change (I was not aware of the\n> > --no-pretty option), I would like to see some statistics on that. Could\n> > you measure the impact, please, and include the results in the commit\n> > message?\n> > \n> > Ciao,\n> > Johannes\n> > \n> \n> I was also unaware of the --no-pretty option. I've tested this on Windows\n> running version 4.9.0 of Watchman and verified that it does work correctly.\n> I'm also curious if it produces any measurable difference in performance.\n\nOn a repository with ~160k files, the following test harness, which\nrequests all files inside the repository and parses that output:\n\n--------------8<-----------\n#!/usr/bin/perl\n\nuse strict;\nuse warnings;\nuse IPC::Open2;\n\nmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, \"watchman -j @ARGV\")\n    or die \"open2() failed: $!\\n\" .\n    \"Falling back to scanning...\\n\";\n\nmy $query = qq|[\"query\", \"$ENV{PWD}\", {}]|;\n\nprint CHLD_IN $query;\nclose CHLD_IN;\nmy $response = do {local $/; <CHLD_OUT>};\n\nmy $json_pkg;\neval {\n    require JSON::XS;\n    $json_pkg = \"JSON::XS\";\n    1;\n} or do {\n    require JSON::PP;\n    $json_pkg = \"JSON::PP\";\n};\n\nmy $o = $json_pkg->new->utf8->decode($response);\n--------------8<-----------\n\n...run with dumbbench[1], produces:\n\n    $ dumbbench -- ./test.pl\n    cmd: Ran 22 iterations (2 outliers).\n    cmd: Rounded run time per iteration: 5.240e+00 +/- 1.1e-02 (0.2%)\n    $ dumbbench -- ./test.pl --no-pretty\n    cmd: Ran 21 iterations (1 outliers).\n    cmd: Rounded run time per iteration: 4.866e+00 +/- 1.3e-02 (0.3%)\n\n...so a modest 8% speedup.  I note that those numbers are for a perl\nwith JSON::XS installed; without it installed, the runtime is so long\nthat I gave up waiting for it.\n\nAnyways, I'll put that in the commit message in the re-roll.\n - Alex\n\n\n[1] https://metacpan.org/release/Dumbbench\n"},{"id":"331051","messageId":"alpine.DEB.2.10.1710251818360.9817@alexmv-linux","threadId":"47002","inReplyTo":"alpine.DEB.2.21.1.1710201503380.40514@virtualbox","subject":"Re: [PATCH 4/4] fsmonitor: Delay updating state until after split index is merged","fromName":"Alex Vandiver","fromEmail":"alexmv@dropbox.com","sentAt":"2017-10-26T01:20:14Z","receivedAt":"2017-10-26T01:20:28Z","isPatch":true,"sender":{"key":"alexmv@dropbox.com","avatar":null},"body":"On Fri, 20 Oct 2017, Johannes Schindelin wrote:\n> From the diff, it is not immediately clear that fsmonitor_dirty is not\n> leaked in any code path.\n> \n> Could you clarify this in the commit message, please?\n\nWill do!\n\n> > @@ -238,6 +225,29 @@ void remove_fsmonitor(struct index_state *istate)\n> >  \n> >  void tweak_fsmonitor(struct index_state *istate)\n> >  {\n> > +\tint i;\n> > +\n> > +\tif (istate->fsmonitor_dirty) {\n> > +\t\t/* Mark all entries valid */\n> > +\t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor is enabled; cache is %d\", istate->cache_nr);\n> \n> Sadly, a call to trace_printf_key() is not really a noop when tracing is\n> disabled. [snip]\n\nApologies -- I'd meant to remove the tracing before committing.  I\nthink we're all on the same page that it would be nice to lower the\nimpact of tracing to let it be more prevalent, but I'd rather not\nblock these changes on that.\n\nThanks for the comments!\n - Alex\n"},{"id":"331165","messageId":"alpine.DEB.2.21.1.1710271709570.6482@virtualbox","threadId":"47002","inReplyTo":"alpine.DEB.2.10.1710251700300.9817@alexmv-linux","subject":"Re: [PATCH 1/4] fsmonitor: Watch, and ask about, the top of the repo, not the CWD","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-27T15:11:13Z","receivedAt":"2017-10-27T15:11:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alex,\n\nOn Wed, 25 Oct 2017, Alex Vandiver wrote:\n\n> On Fri, 20 Oct 2017, Johannes Schindelin wrote:\n> > This is super expensive, as it means a full-blown new process instead of\n> > just a simple environment variable expansion.\n> > \n> > The idea behind using `PWD` instead was that Git will already have done\n> > all of the work of figuring out the top-level directory and switched to\n> > there before calling the fsmonitor hook.\n> \n> I'm not seeing that PWD has been at all altered.\n\nNo, PWD is not altered.\n\nBut a simple environment variable expansion (fast) was replaced by\nspawning to `git rev-parse --show-top-level` (slow, ~60ms on Windows).\n\n*That* is expensive.\n\nCiao,\nJohannes\n"},{"id":"331167","messageId":"alpine.DEB.2.21.1.1710271712020.6482@virtualbox","threadId":"47002","inReplyTo":"alpine.DEB.2.10.1710251722310.9817@alexmv-linux","subject":"Re: [PATCH 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-27T15:13:03Z","receivedAt":"2017-10-27T15:13:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alex,\n\nOn Wed, 25 Oct 2017, Alex Vandiver wrote:\n\n> [Johannes asked about the speedup when using --no-pretty]\n>\n> ...so a modest 8% speedup.  I note that those numbers are for a perl\n> with JSON::XS installed; without it installed, the runtime is so long\n> that I gave up waiting for it.\n\nVery nice! AFAIR Git for Windows does *not* come with JSON::XS.\n\n> Anyways, I'll put that in the commit message in the re-roll.\n\nThank you,\nJohannes\n"}]}