Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 4, 2014, 20:24 UTC
- Message-ID
- <xmqqd2j28w3h.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1391544367-14599-1-git-send-email-dak@gnu.org>
David Kastrup <dak@gnu.org> writes:
Show 24 quoted lines
> 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>
> ---
> builtin/blame.c | 40 ++++++++++++++++++++++++----------------
> 1 file changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index e44a6bb..522986d 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1772,25 +1772,33 @@ static int prepare_lines(struct scoreboard *sb)
> {
> const char *buf = sb->final_buf;
> unsigned long len = sb->final_buf_size;
> - int num = 0, incomplete = 0, bol = 1;
> + const char *end = buf + len;
> + const char *p;
> + int *lineno;
> +
> + int num = 0, incomplete = 0;Is there any significance to the blank line between these two variable definitions?
Show 5 quoted lines
> +
> + for (p = buf;;) {
> + if ((p = memchr(p, '\n', end-p)) == NULL)
> + break;
> + ++num, ++p;You have a peculiar style that is somewhat distracting. Why isn't this more like so?
for (p = buf; p++, num++; ) {
p = memchr(p, '\n', end - p);
if (!p)
break;
}which I think is the prevalent style in our codebase. The same for the other loop we see in the new code below.
- favor post-increment unless you use it as rvalue and need pre-increment;
- SP around each binary ops e.g. 'end - p';
- avoid assignments in conditionals when you do not have to.
Show 5 quoted lines
> + } > > - if (len && buf[len-1] != '\n') > + if (len && end[-1] != '\n') > incomplete++; /* incomplete line at the end */
OK, so far we counted "num" complete lines and "incomplete" may be one if there is an incomplete line after them.
Show 13 quoted lines
> - while (len--) {
> - if (bol) {
> - sb->lineno = xrealloc(sb->lineno,
> - sizeof(int *) * (num + 1));
> - sb->lineno[num] = buf - sb->final_buf;
> - bol = 0;
> - }
> - if (*buf++ == '\n') {
> - num++;
> - bol = 1;
> - }
> +
> + sb->lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1));OK, this function is called only once, so we know sb->lineno is NULL originally and there is no reason to start from xrealloc().
Show 8 quoted lines
> + for (p = buf;;) {
> + *lineno++ = p-buf;
> + if ((p = memchr(p, '\n', end-p)) == NULL)
> + break;
> + ++p;
> }
> - sb->lineno = xrealloc(sb->lineno,
> - sizeof(int *) * (num + incomplete + 1));These really *were* unnecessary reallocations.
Thanks for catching them, but this patch needs heavy style fixes.
Show 8 quoted lines
> - sb->lineno[num + incomplete] = buf - sb->final_buf; > + > + if (incomplete) > + *lineno++ = len; > + > sb->num_lines = num + incomplete; > return sb->num_lines; > }