From: Ben Peart Date: Thu, 26 Oct 2017 20:32:10 GMT Subject: Re: [PATCH v2 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman Message-ID: In-Reply-To: <9bd24b15-6232-1afd-abbe-02870c51c7ad@gmail.com> On 10/26/2017 4:05 PM, Ben Peart wrote: > > > On 10/25/2017 9:31 PM, Alex Vandiver wrote: >> This provides modest performance savings.  Benchmarking with the >> following program, with and without `--no-pretty`, we find savings of >> 23% (0.316s -> 0.242s) in the git repository, and savings of 8% (5.24s >> -> 4.86s) on a large repository with 580k files in the working copy. >> > > Given this patch series is all about speed, it's good to see *any* wins > - especially those that don't impact functionality at all.  The > performance win of --no-pretty is greater than I expected. > >>      #!/usr/bin/perl >> >>      use strict; >>      use warnings; >>      use IPC::Open2; >> >>      my $pid = open2(\*CHLD_OUT, \*CHLD_IN, "watchman -j @ARGV") >>          or die "open2() failed: $!\n" . >>          "Falling back to scanning...\n"; >> >>      my $query = qq|["query", "$ENV{PWD}", {}]|; >> >>      print CHLD_IN $query; >>      close CHLD_IN; >>      my $response = do {local $/; }; >> >>      my $json_pkg; >>      eval { >>          require JSON::XSomething; >>          $json_pkg = "JSON::XSomething"; >>          1; >>      } or do { >>          require JSON::PP; >>          $json_pkg = "JSON::PP"; >>      }; >> >>      my $o = $json_pkg->new->utf8->decode($response); >> >> Signed-off-by: Alex Vandiver >> --- >>   t/t7519/fsmonitor-watchman                 | 2 +- >>   templates/hooks--fsmonitor-watchman.sample | 2 +- >>   2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman >> index a3e30bf54..79f24325c 100755 >> --- a/t/t7519/fsmonitor-watchman >> +++ b/t/t7519/fsmonitor-watchman >> @@ -50,7 +50,7 @@ launch_watchman(); >>   sub launch_watchman { >> -    my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j') >> +    my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty') > > Since this is a test script performance isn't critical.  This version of > the integration script logs the response to a file in > .git/watchman-response.json and is much more human readable without the > "--no-pretty."  As such, I'd leave this one pretty. I didn't see anything (including this) worth another roll so only address it if something else comes up. > >>           or die "open2() failed: $!\n" . >>           "Falling back to scanning...\n"; >> diff --git a/templates/hooks--fsmonitor-watchman.sample >> b/templates/hooks--fsmonitor-watchman.sample >> index 9eba8a740..9a082f278 100755 >> --- a/templates/hooks--fsmonitor-watchman.sample >> +++ b/templates/hooks--fsmonitor-watchman.sample >> @@ -49,7 +49,7 @@ launch_watchman(); >>   sub launch_watchman { >> -    my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j') >> +    my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty') > > No human will see this response so the faster --no-pretty option makes > sense. > >>           or die "open2() failed: $!\n" . >>           "Falling back to scanning...\n"; >>