{"thread":{"id":"54547","subject":"[PATCH] pack-write: use hashwrite_be32() instead of double-buffering array","startedAt":"2020-11-01T08:54:55Z","lastAt":"2020-11-04T16:23:25Z","messageCount":3,"participants":["René Scharfe","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"408808","messageId":"aec69531-d621-ab26-efd4-96e1ae0ed3a4@web.de","threadId":"54547","inReplyTo":null,"subject":"[PATCH] pack-write: use hashwrite_be32() instead of double-buffering array","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-01T08:52:12Z","receivedAt":"2020-11-01T08:54:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"hashwrite() already buffers writes, so pass the fanout table entries\nindividually via hashwrite_be32(), which also does the endianess\nconversion for us.  This avoids a memory copy, shortens the code and\nreduces the number of magic numbers.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nPatch generated with -U8 for easier review.\n\n pack-write.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex a6cdb3c67c..23e19cc1ec 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -43,17 +43,16 @@ static int need_large_offset(off_t offset, const struct pack_idx_option *opts)\n  */\n const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects,\n \t\t\t   int nr_objects, const struct pack_idx_option *opts,\n \t\t\t   const unsigned char *sha1)\n {\n \tstruct hashfile *f;\n \tstruct pack_idx_entry **sorted_by_sha, **list, **last;\n \toff_t last_obj_offset = 0;\n-\tuint32_t array[256];\n \tint i, fd;\n \tuint32_t index_version;\n\n \tif (nr_objects) {\n \t\tsorted_by_sha = objects;\n \t\tlist = sorted_by_sha;\n \t\tlast = sorted_by_sha + nr_objects;\n \t\tfor (i = 0; i < nr_objects; ++i) {\n@@ -101,20 +100,19 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \tfor (i = 0; i < 256; i++) {\n \t\tstruct pack_idx_entry **next = list;\n \t\twhile (next < last) {\n \t\t\tstruct pack_idx_entry *obj = *next;\n \t\t\tif (obj->oid.hash[0] != i)\n \t\t\t\tbreak;\n \t\t\tnext++;\n \t\t}\n-\t\tarray[i] = htonl(next - sorted_by_sha);\n+\t\thashwrite_be32(f, next - sorted_by_sha);\n \t\tlist = next;\n \t}\n-\thashwrite(f, array, 256 * 4);\n\n \t/*\n \t * Write the actual SHA1 entries..\n \t */\n \tlist = sorted_by_sha;\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tstruct pack_idx_entry *obj = *list++;\n \t\tif (index_version < 2)\n--\n2.29.2\n"},{"id":"409079","messageId":"20201104133653.GC3030480@coredump.intra.peff.net","threadId":"54547","inReplyTo":"aec69531-d621-ab26-efd4-96e1ae0ed3a4@web.de","subject":"Re: [PATCH] pack-write: use hashwrite_be32() instead of double-buffering array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T13:36:53Z","receivedAt":"2020-11-04T13:36:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 01, 2020 at 09:52:12AM +0100, René Scharfe wrote:\n\n> hashwrite() already buffers writes, so pass the fanout table entries\n> individually via hashwrite_be32(), which also does the endianess\n> conversion for us.  This avoids a memory copy, shortens the code and\n> reduces the number of magic numbers.\n\nYep, this seems trivially correct. The key observation is that we are\nfilling the array in order:\n\n> @@ -101,20 +100,19 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n>  \tfor (i = 0; i < 256; i++) {\n>  \t\tstruct pack_idx_entry **next = list;\n>  \t\twhile (next < last) {\n>  \t\t\tstruct pack_idx_entry *obj = *next;\n>  \t\t\tif (obj->oid.hash[0] != i)\n>  \t\t\t\tbreak;\n>  \t\t\tnext++;\n>  \t\t}\n> -\t\tarray[i] = htonl(next - sorted_by_sha);\n> +\t\thashwrite_be32(f, next - sorted_by_sha);\n>  \t\tlist = next;\n>  \t}\n> -\thashwrite(f, array, 256 * 4);\n\nPerhaps obvious, but I got bit trying to do another similar conversion\nrecently that was filling in the array out-of-order (not on the list,\nbut in some improvements in the bitmap code that haven't been sent in\nyet).\n\n-Peff\n"},{"id":"409094","messageId":"xmqqmtzxnlad.fsf@gitster.c.googlers.com","threadId":"54547","inReplyTo":"20201104133653.GC3030480@coredump.intra.peff.net","subject":"Re: [PATCH] pack-write: use hashwrite_be32() instead of double-buffering array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T16:23:22Z","receivedAt":"2020-11-04T16:23:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Nov 01, 2020 at 09:52:12AM +0100, René Scharfe wrote:\n>\n>> hashwrite() already buffers writes, so pass the fanout table entries\n>> individually via hashwrite_be32(), which also does the endianess\n>> conversion for us.  This avoids a memory copy, shortens the code and\n>> reduces the number of magic numbers.\n>\n> Yep, this seems trivially correct. The key observation is that we are\n> filling the array in order:\n>\n>> @@ -101,20 +100,19 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n>>  \tfor (i = 0; i < 256; i++) {\n>>  \t\tstruct pack_idx_entry **next = list;\n>>  \t\twhile (next < last) {\n>>  \t\t\tstruct pack_idx_entry *obj = *next;\n>>  \t\t\tif (obj->oid.hash[0] != i)\n>>  \t\t\t\tbreak;\n>>  \t\t\tnext++;\n>>  \t\t}\n>> -\t\tarray[i] = htonl(next - sorted_by_sha);\n>> +\t\thashwrite_be32(f, next - sorted_by_sha);\n>>  \t\tlist = next;\n>>  \t}\n>> -\thashwrite(f, array, 256 * 4);\n>\n> Perhaps obvious, but I got bit trying to do another similar conversion\n> recently that was filling in the array out-of-order...\n\nYeah, filling an array out of order and then writing the result in\norder would obviously different from writing out individual pieces\nin the order the original codeflow used to fill the array.  Worse,\nthe order the data items are fed to hashwrite() obviously affects\nthe hash computed at the end.  An example of too much abstraction\nbiting us? ;-)\n\n\n"}]}