[PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()

[PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()

[René Scharfe]

Whenever the hash table becomes too small then its size is increased,
the original part (and the added space) is zerod out using memset(),
and the table is rebuilt from scratch.

Simplify this proceess by returning the old memory using free() and
allocating the new buffer using xcalloc(), which already clears the
buffer for us.  That way we avoid copying the old hash table contents
needlessly inside xrealloc().

While at it, use the first array member with sizeof instead of a
specific type.  The old code used uint32_t and int, while index is
actually an array of int32_t.  Their sizes are the same basically
everywhere, so it's not actually a problem, but the new code is
cleaner and doesn't have to be touched should the type be changed.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 pack-objects.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pack-objects.c b/pack-objects.c
index d01d851..4f36c32 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -47,8 +47,8 @@ static void rehash_objects(struct packing_data *pdata)
 	if (pdata->index_size < 1024)
 		pdata->index_size = 1024;
 
-	pdata->index = xrealloc(pdata->index, sizeof(uint32_t) * pdata->index_size);
-	memset(pdata->index, 0, sizeof(int) * pdata->index_size);
+	free(pdata->index);
+	pdata->index = xcalloc(pdata->index_size, sizeof(*pdata->index));
 
 	entry = pdata->objects;
 
-- 
2.0.0

Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()

[Jeff King]

On Sun, Jun 01, 2014 at 01:07:21PM +0200, René Scharfe wrote:

> Whenever the hash table becomes too small then its size is increased,
> the original part (and the added space) is zerod out using memset(),
> and the table is rebuilt from scratch.
> 
> Simplify this proceess by returning the old memory using free() and
> allocating the new buffer using xcalloc(), which already clears the
> buffer for us.  That way we avoid copying the old hash table contents
> needlessly inside xrealloc().
> 
> While at it, use the first array member with sizeof instead of a
> specific type.  The old code used uint32_t and int, while index is
> actually an array of int32_t.  Their sizes are the same basically
> everywhere, so it's not actually a problem, but the new code is
> cleaner and doesn't have to be touched should the type be changed.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>

Looks good to me.

BTW, the code does git-blame to Vicent's 2834bc2 (which I also worked
on), but actually originated in 7a979d9 (Thin pack - create packfile
with missing delta base., 2006-02-19). Not that it matters, but I was
just surprised since the code you are changing did not seem familiar to
me. I guess there was just too much refactoring during the code movement
for git-blame to pass along the blame in this case.

-Peff

Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()

[David Kastrup]

Jeff King <peff@peff.net> writes:

> On Sun, Jun 01, 2014 at 01:07:21PM +0200, René Scharfe wrote:
>
>> Whenever the hash table becomes too small then its size is increased,
>> the original part (and the added space) is zerod out using memset(),
>> and the table is rebuilt from scratch.
>> 
>> Simplify this proceess by returning the old memory using free() and
>> allocating the new buffer using xcalloc(), which already clears the
>> buffer for us.  That way we avoid copying the old hash table contents
>> needlessly inside xrealloc().
>> 
>> While at it, use the first array member with sizeof instead of a
>> specific type.  The old code used uint32_t and int, while index is
>> actually an array of int32_t.  Their sizes are the same basically
>> everywhere, so it's not actually a problem, but the new code is
>> cleaner and doesn't have to be touched should the type be changed.
>> 
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>
> Looks good to me.
>
> BTW, the code does git-blame to Vicent's 2834bc2 (which I also worked
> on), but actually originated in 7a979d9 (Thin pack - create packfile
> with missing delta base., 2006-02-19). Not that it matters, but I was
> just surprised since the code you are changing did not seem familiar to
> me. I guess there was just too much refactoring during the code movement
> for git-blame to pass along the blame in this case.

Without -M, "too much refactoring" for git-blame may just be moving a
function to a different place in the same file.

-- 
David Kastrup

Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()

[Jeff King]

On Mon, Jun 02, 2014 at 10:40:44PM +0200, David Kastrup wrote:

> > BTW, the code does git-blame to Vicent's 2834bc2 (which I also worked
> > on), but actually originated in 7a979d9 (Thin pack - create packfile
> > with missing delta base., 2006-02-19). Not that it matters, but I was
> > just surprised since the code you are changing did not seem familiar to
> > me. I guess there was just too much refactoring during the code movement
> > for git-blame to pass along the blame in this case.
> 
> Without -M, "too much refactoring" for git-blame may just be moving a
> function to a different place in the same file.

I tried "git blame -M -C -C -C pack-objects.c" but couldn't get anything
but the whole thing blamed to 2834bc2.

-Peff

Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jun 02, 2014 at 10:40:44PM +0200, David Kastrup wrote:
>
>> > BTW, the code does git-blame to Vicent's 2834bc2 (which I also worked
>> > on), but actually originated in 7a979d9 (Thin pack - create packfile
>> > with missing delta base., 2006-02-19). Not that it matters, but I was
>> > just surprised since the code you are changing did not seem familiar to
>> > me. I guess there was just too much refactoring during the code movement
>> > for git-blame to pass along the blame in this case.
>> 
>> Without -M, "too much refactoring" for git-blame may just be moving a
>> function to a different place in the same file.
>
> I tried "git blame -M -C -C -C pack-objects.c" but couldn't get anything
> but the whole thing blamed to 2834bc2.

Are you two being a bit too unreasonable, or trying to be fanciful
and funny and I am not getting the humor?

Here is the relevant part of what 2834bc27 (pack-objects: refactor
the packing list, 2013-10-24) removes from builtin/pack-objects.c:

-	object_ix = xrealloc(object_ix, sizeof(int) * object_ix_hashsz);
-	memset(object_ix, 0, sizeof(int) * object_ix_hashsz);

And here is how the same rehash is done in pack-objects.c at the
toplevel in the new code:

+	pdata->index = xrealloc(pdata->index, sizeof(uint32_t) * pdata->index_size);
+	memset(pdata->index, 0, sizeof(int) * pdata->index_size);

Surely, the code structure may be similar, but the similarity ends
there.  These lines are not equivalent even under the "-w" option.

Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()

[Jeff King]

On Mon, Jun 02, 2014 at 04:09:12PM -0700, Junio C Hamano wrote:

> > I tried "git blame -M -C -C -C pack-objects.c" but couldn't get anything
> > but the whole thing blamed to 2834bc2.
> 
> Are you two being a bit too unreasonable, or trying to be fanciful
> and funny and I am not getting the humor?

We are being too unreasonable.

> Here is the relevant part of what 2834bc27 (pack-objects: refactor
> the packing list, 2013-10-24) removes from builtin/pack-objects.c:
> 
> -	object_ix = xrealloc(object_ix, sizeof(int) * object_ix_hashsz);
> -	memset(object_ix, 0, sizeof(int) * object_ix_hashsz);
> 
> And here is how the same rehash is done in pack-objects.c at the
> toplevel in the new code:
> 
> +	pdata->index = xrealloc(pdata->index, sizeof(uint32_t) * pdata->index_size);
> +	memset(pdata->index, 0, sizeof(int) * pdata->index_size);
> 
> Surely, the code structure may be similar, but the similarity ends
> there.  These lines are not equivalent even under the "-w" option.

Yes, I did not expect these particular lines to get blamed, but I
thought some of the surrounding function would (which could lead to a
parent-blame to find the true origin). Skimming the diff, it looked like
some of them made it through unscathed. But they didn't.

Running:

  git show 2834bc2 |
  perl -lne '
    /^-(.*)/ and $del{$1}++;
    print "$.: $_" if /^\+(.*)/ && $del{$1};
  '

shows that there are only a handful of interesting lines that survived
completely intact, and typically not more than one line in a row.  The
big exceptions are the bits that made it into pack-objects.h, and a "git
blame" there does find the code movement.

So I think everything is operating as expected.

-Peff