threads / patch / 36806

patchpack-objects: use free()+xcalloc() instead of xrealloc()+memset()

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

## tl;dr

6 messages between Jun 1, 2014 and Jun 3, 2014. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

René Scharfe· Jun 1, 2014, 11:07 UTC · lore

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(-)

Show changes to pack-objects.c +2 −2

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

Jeff King· Jun 2, 2014, 19:42 UTC · re: René Scharfe · lore

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

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

Show 16 quoted lines

> 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

David Kastrup· Jun 2, 2014, 20:40 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 27 quoted lines

> 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

Jeff King· Jun 2, 2014, 21:59 UTC · re: David Kastrup · lore

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

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

Show 9 quoted lines

> > 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

Junio C Hamano· Jun 2, 2014, 23:09 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 14 quoted lines

> 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.

Jeff King· Jun 3, 2014, 06:29 UTC · re: Junio C Hamano · lore

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

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

Show 5 quoted lines

> > 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.

Show 14 quoted lines

> 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