# [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset() 6 messages from 2014-06-01 to 2014-06-03. Participants: René Scharfe, Jeff King, David Kastrup, Junio C Hamano. Thread: https://gitlist.dev/t/36806 ## René Scharfe, 2014-06-01 11:07 Subject: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset() Message-ID: <538B0969.9080409@web.de> URL: https://gitlist.dev/e/538B0969.9080409%40web.de ``` 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 --- 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 ``` ## Jeff King, 2014-06-02 19:42 Subject: Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset() Message-ID: <20140602194246.GD2510@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140602194246.GD2510%40sigill.intra.peff.net In-Reply-To: <538B0969.9080409@web.de> ``` 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 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, 2014-06-02 20:40 Subject: Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset() Message-ID: <878upf9h9v.fsf@fencepost.gnu.org> URL: https://gitlist.dev/e/878upf9h9v.fsf%40fencepost.gnu.org In-Reply-To: <20140602194246.GD2510@sigill.intra.peff.net> ``` Jeff King 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 > > 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, 2014-06-02 21:59 Subject: Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset() Message-ID: <20140602215911.GA4612@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140602215911.GA4612%40sigill.intra.peff.net In-Reply-To: <878upf9h9v.fsf@fencepost.gnu.org> ``` 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 ``` ## Junio C Hamano, 2014-06-02 23:09 Subject: Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset() Message-ID: URL: https://gitlist.dev/e/xmqqbnua3o4n.fsf%40gitster.dls.corp.google.com In-Reply-To: <20140602215911.GA4612@sigill.intra.peff.net> ``` Jeff King 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. ``` ## Jeff King, 2014-06-03 06:29 Subject: Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset() Message-ID: <20140603062951.GA26150@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140603062951.GA26150%40sigill.intra.peff.net In-Reply-To: ``` 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 ```