Re: [PATCH] cat-file: skip expanding default format

[John Cai <johncai86@gmail.com>]

Hi Junio,

On 7 Mar 2022, at 1:11, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> 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.
>>
>> That is OK in principle, but ...
>>
>>> +	if (!opt->format && !opt->print_contents) {
>>> +		char buf[1024];
>>> +
>>> +		print_default_format(buf, 1024, data);
>>> +		batch_write(opt, buf, strlen(buf));
>>> +		goto cleanup;
>>> +	}
>>> +
>>> +	fmt = opt->format ? opt->format : default_format;
>>
>> ... instead of doing this, wouldn't it be nicer to base the decision
>> to call print_default_format() on purely the contents of the format,
>> i.e.
>>
>> 	fmt = opt->format ? opt->format : default_format;
>> 	if (!strcmp(fmt, DEFAULT_FORMAT) && !opt->print_contents) {
>> 		... the above print_default_format() call block here ...
>> 		goto cleanup;
>> 	}
>>
>> where DEFAULT_FORMAT is
>>
>> #define DEFAULT_FORMAT = "%(objectname) %(objecttype) %(objectsize)"
>>
>> and
>>
>>> @@ -515,9 +543,7 @@ static int batch_objects(struct batch_options *opt)
>>>  	struct expand_data data;
>>>  	int save_warning;
>>>  	int retval = 0;
>>> -
>>> -	if (!opt->format)
>>> -		opt->format = "%(objectname) %(objecttype) %(objectsize)";
>>
>> retain the defaulting with
>>
>> 	if (!opt->format)
>> 		opt->format = DEFAULT_FORMAT;
>>
>> instead of making opt->format == NULL to mean something special?
>>
>> That way, even if the user-input happens to name the format that is
>> identical to DEFAULT_FORMAT, because we only care what the format
>> is, and not where the format comes from, we will get the same
>> optimization.  Wouldn't it make more sense?
>
> Actually, doing that literally and naively would not be a good idea,
> as the special case code is inside batch_object_write() that is
> called once per each object, and because the format used will not
> change for each call, doing strcmp() every time is wasteful.  The
> same is true for
>
> 	fmt = opt->format ? opt->format : default_format;
>
> as opt->format will not change across calls to this function.
>
> So, if we were to do this optimization:
>
>  * we key on the fact that opt->format is NULL to trigger the
>    optimization inside batch_object_write(), so that we do not have
>    to strcmp(DEFAULT_FORMAT, fmt) for each and every object.
>
>  * a while loop in batch_objects() or for_each_*_object() calls is
>    what calls batch_object_write() for each object.  So somewhere
>    early in that function (or before we enter the function), we can
>    check opt->format and
>
>     - if it is NULL, we can leave it NULL.
>     - if it is the same as DEFAULT_FORMAT, clear it to NULL.
>
>    so that the optimization in batch_object_write() can cheaply kick
>    in.
>
> would be a good way to go, perhaps?

thanks for looking into this. Yeah, I think the approach you outlined makes
sense for the reasons given.