pack-objects: use free()+xcalloc() instead of xrealloc()+memset()
PATCH 1/1 pack-objects: use free()+xcalloc() instead of xrealloc()+memset()
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.
--- pack-objects.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
pack-objects.c +2 -2
@@ -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;
Signature · 1 line
2.0.0
Quoted 9 lines · 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 usingxcalloc(), which already clears the buffer for us. That way we avoid copying the old hash table contents needlessly insidexrealloc().
Quoted 7 lines
While at it, use the first array member with sizeof instead of a specific type. The old code used
uint32_tand int, while index is actually an array ofint32_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 <[email protected]>
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
Quoted 19 lines · Jeff King
Quoted 9 lines · 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 usingxcalloc(), which already clears the buffer for us. That way we avoid copying the old hash table contents needlessly insidexrealloc().Quoted 7 lines
While at it, use the first array member with sizeof instead of a specific type. The old code used
uint32_tand int, while index is actually an array ofint32_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 <[email protected]>
Quoted 8 lines
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.
Signature · 1 line
David Kastrup
Quoted 9 lines · David Kastrup
Quoted 6 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
Quoted 9 lines · Jeff King
Quoted 7 lines · David Kastrup
Quoted 6 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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 5 lines · Junio C Hamano
Quoted 2 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.
Quoted 12 lines
Here is the relevant part of what
2834bc27(pack-objects: refactor the packing list, 2013-10-24) removes frombuiltin/pack-objects.c:(quoted diff) +0
-2- 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.cat the toplevel in the new code:(quoted diff) +2
-0+ pdata->index = xrealloc(pdata->index, sizeof(uint32_t) * pdata->index_size); + memset(pdata->index, 0, sizeof(int) * pdata->index_size);
Quoted 2 lines
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