Re: [PATCH] cat-file: skip expanding default format
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 7, 2022, 12:15 UTC
- Message-ID
- <220307.865yoq0x7e.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <pull.1221.git.git.1646429845306.gitgitgadget@gmail.com>
On Fri, Mar 04 2022, John Cai via GitGitGadget wrote:
Show 31 quoted lines
> 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. > > Based-on-patch-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Nit: I think it's probably better to just add a Signed-off-by here for me instead to indicate that it's originally based on my crappy WIP code (but most of what you've got here is thoroughly yours & better).
Show 40 quoted lines
> Signed-off-by: John Cai <johncai86@gmail.com>
> [...]
> +static void print_default_format(char *buf, int len, struct expand_data *data)
> +{
> + snprintf(buf, len, "%s %s %"PRIuMAX"\n", oid_to_hex(&data->oid),
> + data->info.type_name->buf,
> + (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
> @@ -363,6 +372,12 @@ static void batch_object_write(const char *obj_name,
> struct packed_git *pack,
> off_t offset)
> {
> + const char *fmt;
> +
> + struct strbuf type_name = STRBUF_INIT;
> + if (!opt->format)
> + data->info.type_name = &type_name;
> +
> if (!data->skip_object_info) {
> int ret;
>
> @@ -377,12 +392,21 @@ 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;
> }
> }
>
> + if (!opt->format && !opt->print_contents) {
> + char buf[1024];
> +
> + print_default_format(buf, 1024, data);
> + batch_write(opt, buf, strlen(buf));Just a nit (Junio comment on most of the rest), for something that's an optimization patch we shouldn't ever need to do strlen() here, since we just called snprintf(), let's just use its return value instead.
I also think that in this case you'll want xsnprintf(), and if not this code is buggy & needs to check the return value (but let's just use x*() ...).
FWIW snprintf() relly should be in a mostly-banned.h, but we only have the blanket banned.h, and there's a few legitimate uses of it :)
(And yes, this is all probably commentary on my own bugs in some WIP code, but at this point I honestly can't remember & didn't look it up)
Thanks for hacking on this & carrying it forward!