Re: [PATCH v2 2/4] fsmonitor: Don't bother pretty-printing JSON from watchman

From: Ben Peart <peartben@gmail.com>
Date: Oct 26, 2017, 20:32 UTC
Message-ID: <e7820a80-eb47-2241-b36a-62a70bbceb5a@gmail.com>
In-Reply-To: <9bd24b15-6232-1afd-abbe-02870c51c7ad@gmail.com>

On 10/26/2017 4:05 PM, Ben Peart wrote:

Show 60 quoted lines

> 
> 
> 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.

I didn't see anything (including this) worth another roll so only address it if something else comes up.

Show 19 quoted lines

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