Re: [PATCH] sha1_file: use strbuf_add() instead of strbuf_addf()

[Derrick Stolee <stolee@gmail.com>]

On 12/1/2017 1:22 PM, Jeff King wrote:
> On Fri, Dec 01, 2017 at 12:49:56PM -0500, Derrick Stolee wrote:
[snip]
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 8ae6cb6285..2160323c4a 100644
> This overall looks good, but I noticed one bug and a few cosmetic
> improvements.

Thanks for finding quality improvements to this patch. I'll let it sit 
over the weekend for more feedback before sending a v2.

>
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -1914,17 +1914,18 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
>>   	}
>>   
>>   	oid.hash[0] = subdir_nr;
>> +	strbuf_add(path, "/", 1);
> Maybe:
>
>    strbuf_addch(path, '/');
>
> would be a bit more readable (it's also a tiny bit faster, but this
> isn't part of the tight loop).
>
>> +	baselen = path->len;
> We set this here so that the '/' is included as part of the base. Makes
> sense, but can we now drop the earlier setting of baselen before the
> opendir() call?

Yeah, probably. I had briefly considered just adding the '/' before the 
first assignment of "baselen", but didn't want to change the error 
output. I also don't know if there are side effects for other platforms 
by calling opendir() with a '/'-terminated path.

>>   	while ((de = readdir(dir))) {
>>   		if (is_dot_or_dotdot(de->d_name))
>>   			continue;
>>   
>> -		strbuf_setlen(path, baselen);
>> -		strbuf_addf(path, "/%s", de->d_name);
>> -
>>   		if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2 &&
>>   		    !hex_to_bytes(oid.hash + 1, de->d_name,
>>   				  GIT_SHA1_RAWSZ - 1)) {
>> +			strbuf_setlen(path, baselen);
>> +			strbuf_add(path, de->d_name, GIT_SHA1_HEXSZ - 2);
> Moving this code into the conditional makes sense here, since that's
> where we know the actual size. But what about the case when we _don't_
> hit this conditional. We still do:
>
>    if (cruft_cb)
>          cruft_cb(path->buf);
>
> which is now broken (it will just get the directory name without the
> entry appended).

Good catch! A big reason to pull it inside and use strbuf_add over 
strbuf_addstr is to avoid a duplicate strlen() calculation. However, I 
can store the length before the conditional.

> I think the optimized versions probably needs to be something more like:
>
>    while (de = readdir(...)) {
>        strbuf_setlen(path, baselen);
>        if (size is HEXSZ - 2) {
>            strbuf_add(path, de->d_name, HEXSZ - 2);
> 	  obj_cb(path->buf);
>        } else if (cruft_cb) {
>            strbuf_addstr(path, de->d_name);
> 	  cruft_cb(path->buf);
>        }
>    }

Small change by storing the length in advance of the conditional:

while (de = readdir(...)) {
     int namelen = strlen(de->d_name);
     strbuf_setlen(path, baselen);
     strbuf_add(path, de->d_name, namelen);

     if (namelen == HEXSZ - 2)
         obj_cb(path->buf)
     else
         cruft_cb(path->buf);
}

> Two other thoughts:
>
>    - from past discussions, I suspect most of your performance
>      improvement actually comes from avoiding addf(), and the difference
>      between addstr() and add() may be negligible here. It might be worth
>      timing strbuf_addstr(). If that's similarly fast, that means we
>      could keep the logic out of the conditional entirely.

addstr() duplicates the strlen(), which isn't much but we might as well 
save cycles where we can if it isn't too hard.

>    - there's an extra micro-optimization there, which is that if there's
>      no obj_cb, we have no need to assemble the full path at all. I doubt
>      it makes much of a difference, as most callers would pass an object
>      callback (I'd be surprised if we even have one that doesn't).

After doing a few 'git grep' commands, I found several that include a 
NULL cruft_cb but none that have a NULL obj_cb.

Thanks,
-Stolee