{"thread":{"id":"8615","subject":"Fix up ugly open-coded \"alloc_nr()\" user in object.c","startedAt":"2007-06-16T17:30:22Z","lastAt":"2007-06-16T22:37:39Z","messageCount":4,"participants":["Linus Torvalds","Jeff King","Olivier Galibert"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"45163","messageId":"alpine.LFD.0.98.0706161024220.14121@woody.linux-foundation.org","threadId":"8615","inReplyTo":null,"subject":"Fix up ugly open-coded \"alloc_nr()\" user in object.c","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-16T17:30:22Z","receivedAt":"2007-06-16T17:30:22Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nWhen adding objects to the object/mode array, we used to have our own \nalloc_nr() implementation, rather than use the normal one.\n\nAnd since the normal one is arguably a bit nicer (still grows the \nallocation exponentially, just not by more-than-doubling it every time), \nwhy not just use it?\n\nThat array of objects ends up being really quite big when you force a \nwhile repack of a big project, and while we might end up doing a few more \nxreallocs in the process, we also hopefully don't end up with a final \nallocation that is quite as wastefully big.\n\nSigned-off-by: Linus Torvalds \n---\n\n That was an overkill of a situation for a trivial patch that I don't \n think is in the least interesting or even important. I really don't care \n if you take this, Junio, but it seemed the obvious one-liner to do, so \n I'm sending it in anyway.\n\n object.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 16793d9..fdd6ceb 100644\n--- a/object.c\n+++ b/object.c\n@@ -245,7 +245,7 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj\n \tstruct object_array_entry *objects = array->objects;\n \n \tif (nr >= alloc) {\n-\t\talloc = (alloc + 32) * 2;\n+\t\talloc = alloc_nr(alloc);\n \t\tobjects = xrealloc(objects, alloc * sizeof(*objects));\n \t\tarray->alloc = alloc;\n \t\tarray->objects = objects;\n"},{"id":"45164","messageId":"20070616182134.GA22003@coredump.intra.peff.net","threadId":"8615","inReplyTo":"alpine.LFD.0.98.0706161024220.14121@woody.linux-foundation.org","subject":"Re: Fix up ugly open-coded \"alloc_nr()\" user in object.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-06-16T18:21:34Z","receivedAt":"2007-06-16T18:21:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 16, 2007 at 10:30:22AM -0700, Linus Torvalds wrote:\n\n> When adding objects to the object/mode array, we used to have our own \n> alloc_nr() implementation, rather than use the normal one.\n> \n> And since the normal one is arguably a bit nicer (still grows the \n> allocation exponentially, just not by more-than-doubling it every time), \n> why not just use it?\n\nHow about using the new ALLOC_GROW macro to make it even shorter? I also\ngot rid of the aliased variables, which IMO just make it harder to see\nwhat's going on.\n\n---\n object.c | 19 +++++--------------\n 1 files changed, 5 insertions(+), 14 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 16793d9..064e423 100644\n--- a/object.c\n+++ b/object.c\n@@ -240,18 +240,9 @@ void add_object_array(struct object *obj, const char *name, struct object_array\n \n void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n {\n-\tunsigned nr = array->nr;\n-\tunsigned alloc = array->alloc;\n-\tstruct object_array_entry *objects = array->objects;\n-\n-\tif (nr >= alloc) {\n-\t\talloc = (alloc + 32) * 2;\n-\t\tobjects = xrealloc(objects, alloc * sizeof(*objects));\n-\t\tarray->alloc = alloc;\n-\t\tarray->objects = objects;\n-\t}\n-\tobjects[nr].item = obj;\n-\tobjects[nr].name = name;\n-\tobjects[nr].mode = mode;\n-\tarray->nr = ++nr;\n+\tALLOC_GROW(array->objects, array->nr, array->alloc);\n+\tarray->objects[array->nr].item = obj;\n+\tarray->objects[array->nr].name = name;\n+\tarray->objects[array->nr].mode = mode;\n+\tarray->nr++;\n }\n"},{"id":"45192","messageId":"20070616221506.GA78651@dspnet.fr.eu.org","threadId":"8615","inReplyTo":"20070616182134.GA22003@coredump.intra.peff.net","subject":"Re: Fix up ugly open-coded \"alloc_nr()\" user in object.c","fromName":"Olivier Galibert","fromEmail":"galibert@pobox.com","sentAt":"2007-06-16T22:15:06Z","receivedAt":"2007-06-16T22:15:06Z","isPatch":false,"sender":{"key":"galibert@pobox.com","avatar":null},"body":"On Sat, Jun 16, 2007 at 02:21:34PM -0400, Jeff King wrote:\n> How about using the new ALLOC_GROW macro to make it even shorter? I also\n> got rid of the aliased variables, which IMO just make it harder to see\n\n> what's going on.\n> +\tALLOC_GROW(array->objects, array->nr, array->alloc);\n> +\tarray->objects[array->nr].item = obj;\n> +\tarray->objects[array->nr].name = name;\n> +\tarray->objects[array->nr].mode = mode;\n> +\tarray->nr++;\n\nUnless the ALLOC_GROW semantics are weird, shouldn't that be:\n ALLOC_GROW(array->objects, array->nr+1, array->alloc);\n\n OG.\n"},{"id":"45193","messageId":"20070616223738.GA19076@coredump.intra.peff.net","threadId":"8615","inReplyTo":"20070616221506.GA78651@dspnet.fr.eu.org","subject":"Re: Fix up ugly open-coded \"alloc_nr()\" user in object.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-06-16T22:37:39Z","receivedAt":"2007-06-16T22:37:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 17, 2007 at 12:15:06AM +0200, Olivier Galibert wrote:\n\n> > +\tALLOC_GROW(array->objects, array->nr, array->alloc);\n> \n> Unless the ALLOC_GROW semantics are weird, shouldn't that be:\n> ALLOC_GROW(array->objects, array->nr+1, array->alloc);\n\nThe semantics are weird. They never seemed so to me before, since it was\nreplacing some \"grow by 1\" areas where it is natural to assume that you\nneed just one spot more. But the way Junio commented it and tweaked it,\nit can handle arbitrary growth (which is much better), but that means we\nare overly conservative about when to grow.\n\nJunio, patch is below (call-sites using bare 'nr' need to be 'nr+1', but\nI will fix those up in a separate patch since they are in next and this\nis in master).\n\n-- >8 --\nfix ALLOC_GROW off-by-one\n\nThe ALLOC_GROW macro will never let us fill the array completely,\ninstead allocating an extra chunk if that would be the case. This is\nbecause the 'nr' argument was originally treated as \"how much we do have\nnow\" instead of \"how much do we want\". The latter makes much more\nsense because you can grow by more than one item.\n\nThis off-by-one never resulted in an error because it meant we were\noverly conservative about when to allocate. Any callers which passed\n\"how we have now\" need to be updated, or they will fail to allocate\nenough.\n\nSigned-off-by: Jeff King \n---\n cache.h | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c914c1c..ed83d92 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -234,7 +234,7 @@ extern void verify_non_filename(const char *prefix, const char *name);\n */\n #define ALLOC_GROW(x, nr, alloc) \\\n \tdo { \\\n-\t\tif ((nr) >= alloc) { \\\n+\t\tif ((nr) > alloc) { \\\n \t\t\tif (alloc_nr(alloc) < (nr)) \\\n \t\t\t\talloc = (nr); \\\n \t\t\telse \\\n"}]}