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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2022, 06:11 UTC
Message-ID: <xmqqilsquwaw.fsf@gitster.g>
In-Reply-To: <xmqqmti2uwzr.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 61 quoted lines

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