Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line

From: David Kastrup <dak@gnu.org>
Date: Feb 4, 2014, 21:09 UTC
Message-ID: <87d2j2wpnr.fsf@fencepost.gnu.org>
In-Reply-To: <xmqq4n4e8uf1.fsf@gitster.dls.corp.google.com>

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

Show 25 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> David Kastrup <dak@gnu.org> writes:
>>
>>> Whitespace error in line 1778.  Should I be reposting?
>>
>> Heh, let me try to clean it up first and then repost for your
>> review.
>>
>> Thanks.
>
> -- >8 --
> From: David Kastrup <dak@gnu.org>
>
> Making a single preparation run for counting the lines will avoid memory
> fragmentation.  Also, fix the allocated memory size which was wrong
> when sizeof(int *) != sizeof(int), and would have been too small
> for sizeof(int *) < sizeof(int), admittedly unlikely.
>
> Signed-off-by: David Kastrup <dak@gnu.org>
> ---
>
>  One logic difference from what was posted is that sb->lineno[num]
>  is filled with the length of the entire buffer when the file ends
>  with a complete line.

Where's the difference? This is exactly what will happen with my code as well. I _do_ rely on memchr(whatever, '\n', 0) to return NULL without looking at any memory for that. If there is a fear of memchr not being able to deal with a count of 0, this code needs to be somewhat more complex.

>  I do not remember if the rest of the logic
>  actually depends on it (I think I use lineno[n+1] - lineno[n] to
>  find the end of line,

Well, you do it about _half_ the time. The other half, you scan for the '\n' explicitly.

Show 5 quoted lines

>  The original code dates back to 2006 when the author of the code
>  was not paid for doing anything for Git but was doing it as a
>  weekend and evening hobby, so it may not be so surprising to find
>  this kind of "what was I thinking when I wrote it" inefficiency in
>  such a code with $0 monetary value ;-)

Oh, _this_ patch is not in the "I want to make money from it" range. If that were the case, I should not have bothered at all. This is just the "this code offends my sense of aesthetics" class. It's purely optional to apply. It's conceivable that it will make a performance difference on non-glibc (or what it's called) platforms.

-- 
David Kastrup