diff mbox series

[v3] cat-file: skip expanding default format

Message ID pull.1221.v3.git.git.1646777327043.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v3] cat-file: skip expanding default format | expand

Commit Message

John Cai March 8, 2022, 10:08 p.m. UTC 
From: John Cai <johncai86@gmail.com>

When format is passed into --batch, --batch-check, --batch-command,
the format gets expanded. When nothing is passed in, the default format
is set and the expand_format() gets called.

We can save on these cycles by hardcoding how to print the
information when nothing is passed as the format, or when the default
format is passed. There is no need for the fully expanded format with
the default. Since batch_object_write() happens on every object provided
in batch mode, we get a nice performance improvement.

git rev-list --all > /tmp/all-obj.txt

git cat-file --batch-check </tmp/all-obj.txt

with HEAD^:

Time (mean ± σ): 57.6 ms ± 1.7 ms [User: 51.5 ms, System: 6.2 ms]
Range (min … max): 54.6 ms … 64.7 ms 50 runs

with HEAD:

Time (mean ± σ): 49.8 ms ± 1.7 ms [User: 42.6 ms, System: 7.3 ms]
Range (min … max): 46.9 ms … 55.9 ms 56 runs

If nothing is provided as a format argument, or if the default format is
passed, skip expanding of the format and print the object info with a
default format.

See https://lore.kernel.org/git/87eecf8ork.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: John Cai <johncai86@gmail.com>
---
    optimize cat file batch info writing
    
    When cat-file --batch or --batch-check is used, we can skip having to
    expand the format if no format is specified or if the default format is
    specified. In this case we know exactly how to print the objects without
    the full expanded format.
    
    This was first discussed in [1].
    
    We get a little performance boost from this optimization because this
    happens for each objects provided to --batch, --batch-check, or
    --batch-command. Because batch_object_write() is called on every oid
    provided in batch mode, this optimization adds up when a large number of
    oid info is printed.
    
    git rev-list --all >/tmp/all-objs.txt
    
    git cat-file --batch-check </tmp/all-obj.txt (with hyperfine)
    
    run on origin/master:
    
    Time (mean ± σ): 57.6 ms ± 1.7 ms [User: 51.5 ms, System: 6.2 ms] Range
    (min … max): 54.6 ms … 64.7 ms 50 runs
    
    run on jc/optimize-cat-file-batch-default-format:
    
    Time (mean ± σ): 49.8 ms ± 1.7 ms [User: 42.6 ms, System: 7.3 ms] Range
    (min … max): 46.9 ms … 55.9 ms 56 runs
    
    Changes since v1:
    
     * set opt->format in batch_objects so that the loop that prints objects
       only has to check if the format is null to know to print the object
       info in the default format
     * fixed up commit trailer to include Ævar as Signed-off-by
    
     1. https://lore.kernel.org/git/87eecf8ork.fsf@evledraar.gmail.com/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1221%2Fjohn-cai%2Fjc%2Foptimize-cat-file-batch-default-format-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1221/john-cai/jc/optimize-cat-file-batch-default-format-v3
Pull-Request: https://github.com/git/git/pull/1221

Range-diff vs v2:

 1:  f5d578d14a9 ! 1:  56d13da5141 cat-file: skip expanding default format
     @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, s
      +static int print_default_format(char *buf, int len, struct expand_data *data)
      +{
      +	return xsnprintf(buf, len, "%s %s %"PRIuMAX"\n", oid_to_hex(&data->oid),
     -+		 data->info.type_name->buf,
     -+		 (uintmax_t)*data->info.sizep);
     -+
     ++			 type_name(data->type),
     ++			 (uintmax_t)*data->info.sizep);
      +}
      +
       /*
        * If "pack" is non-NULL, then "offset" is the byte offset within the pack from
        * which the object may be accessed (though note that we may also rely on
      @@ builtin/cat-file.c: static void batch_object_write(const char *obj_name,
     - 			       struct packed_git *pack,
     - 			       off_t offset)
     - {
     -+	struct strbuf type_name = STRBUF_INIT;
     -+
     -+	if (!opt->format)
     -+		data->info.type_name = &type_name;
     -+
     - 	if (!data->skip_object_info) {
     - 		int ret;
     - 
     -@@ builtin/cat-file.c: static void batch_object_write(const char *obj_name,
     - 			printf("%s missing\n",
     - 			       obj_name ? obj_name : oid_to_hex(&data->oid));
     - 			fflush(stdout);
     --			return;
     -+			goto cleanup;
       		}
       	}
       
     @@ builtin/cat-file.c: static void batch_object_write(const char *obj_name,
       
       	if (opt->print_contents) {
       		print_object_or_die(opt, data);
     - 		batch_write(opt, "\n", 1);
     - 	}
     -+
     -+cleanup:
     -+	strbuf_release(&type_name);
     - }
     - 
     -+
     - static void batch_one_object(const char *obj_name,
     - 			     struct strbuf *scratch,
     - 			     struct batch_options *opt,
      @@ builtin/cat-file.c: static int batch_unordered_packed(const struct object_id *oid,
       				      data);
       }


 builtin/cat-file.c       | 34 ++++++++++++++++++++++++++--------
 t/perf/p1006-cat-file.sh | 16 ++++++++++++++++
 2 files changed, 42 insertions(+), 8 deletions(-)
 create mode 100755 t/perf/p1006-cat-file.sh


base-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7

Comments

Taylor Blau March 8, 2022, 10:30 p.m. UTC | #1 
On Tue, Mar 08, 2022 at 10:08:46PM +0000, John Cai via GitGitGadget wrote:
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 7b3f42950ec..e2edba70b41 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -351,6 +351,13 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  	}
>  }
>
> +static int print_default_format(char *buf, int len, struct expand_data *data)
> +{
> +	return xsnprintf(buf, len, "%s %s %"PRIuMAX"\n", oid_to_hex(&data->oid),
> +			 type_name(data->type),
> +			 (uintmax_t)*data->info.sizep);
> +}

Two small nits here. It looks like the indentation on the second and
third lines is off a little bit, since we'd typically expect those to be
indented to the same margin as the first argument to xsnprintf().

The other is that you're reading data->info.sizep by dereferencing it,
but we know that it points to data->size. So I think there it makes
sense to just read the value directly out of data->size, though note
that you'll still need the cast to uintmax_t since you're formatting it
with PRIuMAX.

> +
>  /*
>   * If "pack" is non-NULL, then "offset" is the byte offset within the pack from
>   * which the object may be accessed (though note that we may also rely on
> @@ -381,10 +388,16 @@ static void batch_object_write(const char *obj_name,
>  		}
>  	}
>
> -	strbuf_reset(scratch);
> -	strbuf_expand(scratch, opt->format, expand_format, data);
> -	strbuf_addch(scratch, '\n');
> -	batch_write(opt, scratch->buf, scratch->len);
> +	if (!opt->format) {
> +		char buf[1024];
> +		int len = print_default_format(buf, 1024, data);
> +		batch_write(opt, buf, len);

Just curious (and apologies if this was discussed earlier and I missed
it), but: is there a reason that we have to use a scratch buffer here
that is separate from the strbuf we already have allocated?

That would avoid a large-ish stack variable, but it means that the two
paths are a little more similar, and can share the batch_write call
outside of the if/else statement.

The rest of the changes in this file all look good to me.

> diff --git a/t/perf/p1006-cat-file.sh b/t/perf/p1006-cat-file.sh
> new file mode 100755
> index 00000000000..e463623f5a3
> --- /dev/null
> +++ b/t/perf/p1006-cat-file.sh
> @@ -0,0 +1,16 @@
> +#!/bin/sh
> +
> +test_description='Basic sort performance tests'

Is this description a hold-over from p0071? If so, it may be worth
updating here.

> +test_expect_success 'setup' '
> +	git rev-list --all >rla
> +'
> +
> +test_perf 'cat-file --batch-check' '
> +	git cat-file --batch-check <rla
> +'

We could probably get away with dropping the setup test and using
`--batch-all-objects` here. Note that right now you're only printing
commit objects, so there would be a slight behavior change from the way
the patch is currently written, but it should demonstrate the same
performance improvement.

Thanks,
Taylor
John Cai March 8, 2022, 11:09 p.m. UTC | #2 
Hi Taylor,

On 8 Mar 2022, at 17:30, Taylor Blau wrote:

> On Tue, Mar 08, 2022 at 10:08:46PM +0000, John Cai via GitGitGadget wrote:
>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
>> index 7b3f42950ec..e2edba70b41 100644
>> --- a/builtin/cat-file.c
>> +++ b/builtin/cat-file.c
>> @@ -351,6 +351,13 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>>  	}
>>  }
>>
>> +static int print_default_format(char *buf, int len, struct expand_data *data)
>> +{
>> +	return xsnprintf(buf, len, "%s %s %"PRIuMAX"\n", oid_to_hex(&data->oid),
>> +			 type_name(data->type),
>> +			 (uintmax_t)*data->info.sizep);
>> +}
>
> Two small nits here. It looks like the indentation on the second and
> third lines is off a little bit, since we'd typically expect those to be
> indented to the same margin as the first argument to xsnprintf().

Thanks for bringing this up. I did have a question about indentation in this
case. for the second line, I did try to indent it to align with buf. I attempted
to do the same with the third line, but it's the ( that lines up with buf so
optically it looks a little off.

>
> The other is that you're reading data->info.sizep by dereferencing it,
> but we know that it points to data->size. So I think there it makes
> sense to just read the value directly out of data->size, though note
> that you'll still need the cast to uintmax_t since you're formatting it
> with PRIuMAX.

good point, I'll adjust this in the next version.

>
>> +
>>  /*
>>   * If "pack" is non-NULL, then "offset" is the byte offset within the pack from
>>   * which the object may be accessed (though note that we may also rely on
>> @@ -381,10 +388,16 @@ static void batch_object_write(const char *obj_name,
>>  		}
>>  	}
>>
>> -	strbuf_reset(scratch);
>> -	strbuf_expand(scratch, opt->format, expand_format, data);
>> -	strbuf_addch(scratch, '\n');
>> -	batch_write(opt, scratch->buf, scratch->len);
>> +	if (!opt->format) {
>> +		char buf[1024];
>> +		int len = print_default_format(buf, 1024, data);
>> +		batch_write(opt, buf, len);
>
> Just curious (and apologies if this was discussed earlier and I missed
> it), but: is there a reason that we have to use a scratch buffer here
> that is separate from the strbuf we already have allocated?
>
> That would avoid a large-ish stack variable, but it means that the two
> paths are a little more similar, and can share the batch_write call
> outside of the if/else statement.

This was holdover code from before. Looks like the scratch buffer gets passed
in. Do you mean we don't need to allocate char buf[1024] and instead we can just
use scratch and pass it into print_default_format?

>
> The rest of the changes in this file all look good to me.
>
>> diff --git a/t/perf/p1006-cat-file.sh b/t/perf/p1006-cat-file.sh
>> new file mode 100755
>> index 00000000000..e463623f5a3
>> --- /dev/null
>> +++ b/t/perf/p1006-cat-file.sh
>> @@ -0,0 +1,16 @@
>> +#!/bin/sh
>> +
>> +test_description='Basic sort performance tests'
>
> Is this description a hold-over from p0071? If so, it may be worth
> updating here.
>
>> +test_expect_success 'setup' '
>> +	git rev-list --all >rla
>> +'
>> +
>> +test_perf 'cat-file --batch-check' '
>> +	git cat-file --batch-check <rla
>> +'
>
> We could probably get away with dropping the setup test and using
> `--batch-all-objects` here. Note that right now you're only printing
> commit objects, so there would be a slight behavior change from the way
> the patch is currently written, but it should demonstrate the same
> performance improvement.

This sounds good to me!

>
> Thanks,
> Taylor
John Cai March 8, 2022, 11:34 p.m. UTC | #3 
On 8 Mar 2022, at 18:09, John Cai wrote:

> Hi Taylor,
>
> On 8 Mar 2022, at 17:30, Taylor Blau wrote:
>
>> On Tue, Mar 08, 2022 at 10:08:46PM +0000, John Cai via GitGitGadget wrote:
>>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
>>> index 7b3f42950ec..e2edba70b41 100644
>>> --- a/builtin/cat-file.c
>>> +++ b/builtin/cat-file.c
>>> @@ -351,6 +351,13 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>>>  	}
>>>  }
>>>
>>> +static int print_default_format(char *buf, int len, struct expand_data *data)
>>> +{
>>> +	return xsnprintf(buf, len, "%s %s %"PRIuMAX"\n", oid_to_hex(&data->oid),
>>> +			 type_name(data->type),
>>> +			 (uintmax_t)*data->info.sizep);
>>> +}
>>
>> Two small nits here. It looks like the indentation on the second and
>> third lines is off a little bit, since we'd typically expect those to be
>> indented to the same margin as the first argument to xsnprintf().
>
> Thanks for bringing this up. I did have a question about indentation in this
> case. for the second line, I did try to indent it to align with buf. I attempted
> to do the same with the third line, but it's the ( that lines up with buf so
> optically it looks a little off.
>
>>
>> The other is that you're reading data->info.sizep by dereferencing it,
>> but we know that it points to data->size. So I think there it makes
>> sense to just read the value directly out of data->size, though note
>> that you'll still need the cast to uintmax_t since you're formatting it
>> with PRIuMAX.
>
> good point, I'll adjust this in the next version.
>
>>
>>> +
>>>  /*
>>>   * If "pack" is non-NULL, then "offset" is the byte offset within the pack from
>>>   * which the object may be accessed (though note that we may also rely on
>>> @@ -381,10 +388,16 @@ static void batch_object_write(const char *obj_name,
>>>  		}
>>>  	}
>>>
>>> -	strbuf_reset(scratch);
>>> -	strbuf_expand(scratch, opt->format, expand_format, data);
>>> -	strbuf_addch(scratch, '\n');
>>> -	batch_write(opt, scratch->buf, scratch->len);
>>> +	if (!opt->format) {
>>> +		char buf[1024];
>>> +		int len = print_default_format(buf, 1024, data);
>>> +		batch_write(opt, buf, len);
>>
>> Just curious (and apologies if this was discussed earlier and I missed
>> it), but: is there a reason that we have to use a scratch buffer here
>> that is separate from the strbuf we already have allocated?
>>
>> That would avoid a large-ish stack variable, but it means that the two
>> paths are a little more similar, and can share the batch_write call
>> outside of the if/else statement.
>
> This was holdover code from before. Looks like the scratch buffer gets passed
> in. Do you mean we don't need to allocate char buf[1024] and instead we can just
> use scratch and pass it into print_default_format?

something like this?

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index e2edba70b418..2336bcc80850 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -351,11 +351,11 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
        }
 }

-static int print_default_format(char *buf, int len, struct expand_data *data)
+static void print_default_format(struct strbuf *scratch, struct expand_data *data)
 {
-       return xsnprintf(buf, len, "%s %s %"PRIuMAX"\n", oid_to_hex(&data->oid),
-                        type_name(data->type),
-                        (uintmax_t)*data->info.sizep);
+       strbuf_addf(scratch, "%s %s %"PRIuMAX"\n", oid_to_hex(&data->oid),
+                   type_name(data->type),
+                   (uintmax_t)data->size);
 }

 /*
@@ -388,17 +388,17 @@ static void batch_object_write(const char *obj_name,
                }
        }

+       strbuf_reset(scratch);
+
        if (!opt->format) {
-               char buf[1024];
-               int len = print_default_format(buf, 1024, data);
-               batch_write(opt, buf, len);
+               print_default_format(scratch, data);
        } else {
-               strbuf_reset(scratch);
                strbuf_expand(scratch, opt->format, expand_format, data);
                strbuf_addch(scratch, '\n');
-               batch_write(opt, scratch->buf, scratch->len);
        }

+       batch_write(opt, scratch->buf, scratch->len);
+
        if (opt->print_contents) {
                print_object_or_die(opt, data);
                batch_write(opt, "\n", 1);
>
>>
>> The rest of the changes in this file all look good to me.
>>
>>> diff --git a/t/perf/p1006-cat-file.sh b/t/perf/p1006-cat-file.sh
>>> new file mode 100755
>>> index 00000000000..e463623f5a3
>>> --- /dev/null
>>> +++ b/t/perf/p1006-cat-file.sh
>>> @@ -0,0 +1,16 @@
>>> +#!/bin/sh
>>> +
>>> +test_description='Basic sort performance tests'
>>
>> Is this description a hold-over from p0071? If so, it may be worth
>> updating here.
>>
>>> +test_expect_success 'setup' '
>>> +	git rev-list --all >rla
>>> +'
>>> +
>>> +test_perf 'cat-file --batch-check' '
>>> +	git cat-file --batch-check <rla
>>> +'
>>
>> We could probably get away with dropping the setup test and using
>> `--batch-all-objects` here. Note that right now you're only printing
>> commit objects, so there would be a slight behavior change from the way
>> the patch is currently written, but it should demonstrate the same
>> performance improvement.
>
> This sounds good to me!
>
>>
>> Thanks,
>> Taylor
diff mbox series

Patch

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 7b3f42950ec..e2edba70b41 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -351,6 +351,13 @@  static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 	}
 }
 
+static int print_default_format(char *buf, int len, struct expand_data *data)
+{
+	return xsnprintf(buf, len, "%s %s %"PRIuMAX"\n", oid_to_hex(&data->oid),
+			 type_name(data->type),
+			 (uintmax_t)*data->info.sizep);
+}
+
 /*
  * If "pack" is non-NULL, then "offset" is the byte offset within the pack from
  * which the object may be accessed (though note that we may also rely on
@@ -381,10 +388,16 @@  static void batch_object_write(const char *obj_name,
 		}
 	}
 
-	strbuf_reset(scratch);
-	strbuf_expand(scratch, opt->format, expand_format, data);
-	strbuf_addch(scratch, '\n');
-	batch_write(opt, scratch->buf, scratch->len);
+	if (!opt->format) {
+		char buf[1024];
+		int len = print_default_format(buf, 1024, data);
+		batch_write(opt, buf, len);
+	} else {
+		strbuf_reset(scratch);
+		strbuf_expand(scratch, opt->format, expand_format, data);
+		strbuf_addch(scratch, '\n');
+		batch_write(opt, scratch->buf, scratch->len);
+	}
 
 	if (opt->print_contents) {
 		print_object_or_die(opt, data);
@@ -508,6 +521,9 @@  static int batch_unordered_packed(const struct object_id *oid,
 				      data);
 }
 
+
+#define DEFAULT_FORMAT "%(objectname) %(objecttype) %(objectsize)"
+
 static int batch_objects(struct batch_options *opt)
 {
 	struct strbuf input = STRBUF_INIT;
@@ -516,9 +532,6 @@  static int batch_objects(struct batch_options *opt)
 	int save_warning;
 	int retval = 0;
 
-	if (!opt->format)
-		opt->format = "%(objectname) %(objecttype) %(objectsize)";
-
 	/*
 	 * Expand once with our special mark_query flag, which will prime the
 	 * object_info to be handed to oid_object_info_extended for each
@@ -526,12 +539,17 @@  static int batch_objects(struct batch_options *opt)
 	 */
 	memset(&data, 0, sizeof(data));
 	data.mark_query = 1;
-	strbuf_expand(&output, opt->format, expand_format, &data);
+	strbuf_expand(&output,
+		      opt->format ? opt->format : DEFAULT_FORMAT,
+		      expand_format,
+		      &data);
 	data.mark_query = 0;
 	strbuf_release(&output);
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
 
+	if (opt->format && !strcmp(opt->format, DEFAULT_FORMAT))
+		opt->format = NULL;
 	/*
 	 * If we are printing out the object, then always fill in the type,
 	 * since we will want to decide whether or not to stream.
diff --git a/t/perf/p1006-cat-file.sh b/t/perf/p1006-cat-file.sh
new file mode 100755
index 00000000000..e463623f5a3
--- /dev/null
+++ b/t/perf/p1006-cat-file.sh
@@ -0,0 +1,16 @@ 
+#!/bin/sh
+
+test_description='Basic sort performance tests'
+. ./perf-lib.sh
+
+test_perf_large_repo
+
+test_expect_success 'setup' '
+	git rev-list --all >rla
+'
+
+test_perf 'cat-file --batch-check' '
+	git cat-file --batch-check <rla
+'
+
+test_done