Re: [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy
- From
Erik Faye-Lund <kusmabite@googlemail.com>
- Date
- Mar 21, 2010, 21:09 UTC
- Message-ID
- <40aa078e1003211409q50b9efc3pf5bcfccb48b7eeb6@mail.gmail.com>
- In-Reply-To
- <7v39ztmnjp.fsf@alter.siamese.dyndns.org>
On Sun, Mar 21, 2010 at 9:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 36 quoted lines
> Erik Faye-Lund <kusmabite@googlemail.com> writes:
>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>> sha1_name.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/sha1_name.c b/sha1_name.c
>> index bf92417..2b1be58 100644
>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -196,10 +196,10 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
>> int status, exists;
>> static char hex[41];
>>
>> + if (len == 40 || !len)
>> + return sha1_to_hex(sha1);
>> exists = has_sha1_file(sha1);
>> memcpy(hex, sha1_to_hex(sha1), 40);
>> - if (len == 40 || !len)
>> - return hex;
>
> This is somewhat iffy. hex[] being static means there can only be one
> outstanding return value from f-u-a being used, iow
>
> printf("%s %s", f-u-a(a, 0), f-u-a(b, 0))
>
> is a no-no. But at the same time, it means that you can use one more
> recycled buffer than sha1_to_hex() gives us, so this may be safe:
>
> char *ua = f-u-a(a, 0);
> printf("%s %s %s %s %s", ua,
> sha1_to_hex(b), sha1_to_hex(c), sha1_to_hex(d), sha1_to_hex(e));
>
> but with the above it probably is not anymore, no?
>True, I didn't think of that.
But is this really a problem? I mean, we already have 3 guard-buffers here, so it would only trigger when the result of more than four calls to sha1_to_hex (directly, or through find_unique_abbrev) has to be kept alive at the same time.
The only documentation I could find of this was the comment "/* static buffer result! */" from cache.h, and it doesn't mention that there's multiple buffers. So I'd say that any call-site that required four buffers are, well, perhaps a little bit TOO tightly integrated with that function ;)
The test-suite didn't catch any failures in it, but I guess I would have to analyze the calling code a bit more to be sure.
> As an optimization patch, I would buy that delaying the "exists" check > until the "no abbreviation" check returned early would make sense, though. >
...or we could just do this. To be honest, this is probably where most of the change in performance come from. Which in case makes the commit message somewhat misleading.
Then again, the third option is the easiest one; just drop 2/3 and 3/3. I'd be perfectly fine with that, I just went a little OCD with consistency here. Those two patches gain us very little. I haven't measured performance change outside of ls-tree, though.
-- Erik "kusma" Faye-Lund