From: Ben Peart <peartben@gmail.com>
Date: Thu, 26 Oct 2017 20:05:13 GMT
Subject: Re: [PATCH v2 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman
Message-ID: <9bd24b15-6232-1afd-abbe-02870c51c7ad@gmail.com>
In-Reply-To: <42fd8bccb78992a2894e711e057230a673891628.1508981451.git.alexmv@dropbox.com>



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 $/; <CHLD_OUT>};
> 
>      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 <alexmv@dropbox.com>
> ---
>   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.

>   	    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";
>   
>