{"thread":{"id":"53967","subject":"[PATCH] midx: use buffered I/O to talk to pack-objects","startedAt":"2020-08-02T14:38:44Z","lastAt":"2020-08-13T09:11:05Z","messageCount":14,"participants":["René Scharfe","Chris Torek","Derrick Stolee","Johannes Sixt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"402650","messageId":"c5920e08-b7dd-e870-f99e-225d0aafc663@web.de","threadId":"53967","inReplyTo":null,"subject":"[PATCH] midx: use buffered I/O to talk to pack-objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-08-02T14:38:40Z","receivedAt":"2020-08-02T14:38:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,\n2016-06-08), significantly reduce the number of system calls and\nsimplify the code for sending object IDs to pack-objects by using\nstdio's buffering and handling errors after the loop.\n\nSigned-off-by: René Scharfe \n---\n midx.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 6d1584ca51d..742638c3e51 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1383,6 +1383,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tuint32_t i;\n \tunsigned char *include_pack;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tFILE *cmd_in;\n \tstruct strbuf base_name = STRBUF_INIT;\n \tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n\n@@ -1435,6 +1436,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t\tgoto cleanup;\n \t}\n\n+\tcmd_in = xfdopen(cmd.in, \"w\");\n+\n \tfor (i = 0; i < m->num_objects; i++) {\n \t\tstruct object_id oid;\n \t\tuint32_t pack_int_id = nth_midxed_pack_int_id(m, i);\n@@ -1443,10 +1446,15 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t\t\tcontinue;\n\n \t\tnth_midxed_object_oid(&oid, m, i);\n-\t\txwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);\n-\t\txwrite(cmd.in, \"\\n\", 1);\n+\t\tfprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n+\t}\n+\n+\tif (fclose(cmd_in)) {\n+\t\terror_errno(_(\"could not close stdin of pack-objects\"));\n+\t\tresult = 1;\n+\t\tfinish_command(&cmd);\n+\t\tgoto cleanup;\n \t}\n-\tclose(cmd.in);\n\n \tif (finish_command(&cmd)) {\n \t\terror(_(\"could not finish pack-objects\"));\n--\n2.28.0\n"},{"id":"402660","messageId":"CAPx1GveChRzsA=FayzhZRSQLPz+o1Po3-_9ZvAGLPztpH5Jg9w@mail.gmail.com","threadId":"53967","inReplyTo":"c5920e08-b7dd-e870-f99e-225d0aafc663@web.de","subject":"Re: [PATCH] midx: use buffered I/O to talk to pack-objects","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-08-02T16:11:17Z","receivedAt":"2020-08-02T16:13:25Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Sun, Aug 2, 2020 at 7:40 AM René Scharfe wrote:\n> @@ -1443,10 +1446,15 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n> continue;\n>\n> nth_midxed_object_oid(&oid, m, i);\n> - xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);\n> - xwrite(cmd.in, \"\\n\", 1);\n> + fprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n> + }\n> +\n> + if (fclose(cmd_in)) {\n> + error_errno(_(\"could not close stdin of pack-objects\"));\n> + result = 1;\n> + finish_command(&cmd);\n> + goto cleanup;\n> }\n> - close(cmd.in);\n>\n> if (finish_command(&cmd)) {\n> error(_(\"could not finish pack-objects\"));\n> --\n> 2.28.0\n\nHere, we don't have any explicit errno checking, but\nof course error_errno() uses errno. This too needs\nan ferror() (or fflush()) test before the final fclose(),\nand then we just need to use plain error(). Otherwise\nyou'll need the clumsier test-after-each-fprintf() and\nan explicit final fflush()-and-test.\n\nChris\n"},{"id":"402679","messageId":"90541678-f412-89a1-2ee0-4cae30e26551@gmail.com","threadId":"53967","inReplyTo":"c5920e08-b7dd-e870-f99e-225d0aafc663@web.de","subject":"Re: [PATCH] midx: use buffered I/O to talk to pack-objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-03T12:39:05Z","receivedAt":"2020-08-03T12:39:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/2/2020 10:38 AM, René Scharfe wrote:\n> Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,\n> 2016-06-08), significantly reduce the number of system calls and\n> simplify the code for sending object IDs to pack-objects by using\n> stdio's buffering and handling errors after the loop.\n\nGood find. Thanks for doing this important cleanup.\n\nOutside of Chris's other feedback, this looks like an obviously\ncorrect transformation.\n\nThanks,\n-Stolee\n"},{"id":"402714","messageId":"1fd18b0c-8c2c-54ab-89aa-357b046eb403@kdbg.org","threadId":"53967","inReplyTo":"CAPx1GveChRzsA=FayzhZRSQLPz+o1Po3-_9ZvAGLPztpH5Jg9w@mail.gmail.com","subject":"Re: [PATCH] midx: use buffered I/O to talk to pack-objects","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-08-03T18:10:49Z","receivedAt":"2020-08-03T18:10:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.08.20 um 18:11 schrieb Chris Torek:\n> On Sun, Aug 2, 2020 at 7:40 AM René Scharfe wrote:\n>> @@ -1443,10 +1446,15 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n>> continue;\n>>\n>> nth_midxed_object_oid(&oid, m, i);\n>> - xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);\n>> - xwrite(cmd.in, \"\\n\", 1);\n>> + fprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n>> + }\n>> +\n>> + if (fclose(cmd_in)) {\n>> + error_errno(_(\"could not close stdin of pack-objects\"));\n>> + result = 1;\n>> + finish_command(&cmd);\n>> + goto cleanup;\n>> }\n>> - close(cmd.in);\n>>\n>> if (finish_command(&cmd)) {\n>> error(_(\"could not finish pack-objects\"));\n>> --\n>> 2.28.0\n> \n> Here, we don't have any explicit errno checking, but\n> of course error_errno() uses errno. This too needs\n> an ferror() (or fflush()) test before the final fclose(),\n> and then we just need to use plain error(). Otherwise\n> you'll need the clumsier test-after-each-fprintf() and\n> an explicit final fflush()-and-test.\n\nWe need this explicit test after each fprintf anyway because SIGPIPE may\nbe ignored, and then writing fails with EPIPE. On Windows, this is\ndoubly important because we do not have SIGPIPE at all (and always see\nEPIPE), but we see EPIPE only on the first failed write; subsequent\nwrites produce EINVAL.\n\n-- Hannes\n"},{"id":"402761","messageId":"fdf66f66-b7ed-f733-22d0-23ec17209f40@web.de","threadId":"53967","inReplyTo":"1fd18b0c-8c2c-54ab-89aa-357b046eb403@kdbg.org","subject":"Re: [PATCH] midx: use buffered I/O to talk to pack-objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-08-03T22:27:35Z","receivedAt":"2020-08-03T22:28:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.08.20 um 20:10 schrieb Johannes Sixt:\n> Am 02.08.20 um 18:11 schrieb Chris Torek:\n>> On Sun, Aug 2, 2020 at 7:40 AM René Scharfe wrote:\n>>> @@ -1443,10 +1446,15 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n>>> continue;\n>>>\n>>> nth_midxed_object_oid(&oid, m, i);\n>>> - xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);\n>>> - xwrite(cmd.in, \"\\n\", 1);\n>>> + fprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n>>> + }\n>>> +\n>>> + if (fclose(cmd_in)) {\n>>> + error_errno(_(\"could not close stdin of pack-objects\"));\n>>> + result = 1;\n>>> + finish_command(&cmd);\n>>> + goto cleanup;\n>>> }\n>>> - close(cmd.in);\n>>>\n>>> if (finish_command(&cmd)) {\n>>> error(_(\"could not finish pack-objects\"));\n>>> --\n>>> 2.28.0\n>>\n>> Here, we don't have any explicit errno checking, but\n>> of course error_errno() uses errno. This too needs\n>> an ferror() (or fflush()) test before the final fclose(),\n>> and then we just need to use plain error(). Otherwise\n>> you'll need the clumsier test-after-each-fprintf() and\n>> an explicit final fflush()-and-test.\n\nOK, the implicit fflush() called by fclose() and thus fclose() itself\ncan succeed even if the error indicator is set, in particular if that\nfflush() has nothing to do. So we need to check ferror() before calling\nfclose().\n\nIf ferror() tells us there was an error, errno might contain some random\nerror code, but not necessarily the root cause. Thus we better keep\nquiet about it and only use error() to tell the user we failed to talk\nto our child but we don't know why.\n\nWe could fflush() explicitly before fclose(), but fclose() reports any\nfailure of its implicit fflush() anyway , so we don't gain anything by\ndoing so.\n\nDid I get that right?\n\n> We need this explicit test after each fprintf anyway because SIGPIPE may\n> be ignored, and then writing fails with EPIPE. On Windows, this is\n> doubly important because we do not have SIGPIPE at all (and always see\n> EPIPE), but we see EPIPE only on the first failed write; subsequent\n> writes produce EINVAL.\n\nWhy is this important? The current code doesn't care about it, at\nleast. It does care about EINTR, though.\n\nRené\n\n"},{"id":"402792","messageId":"90b839cd-29e0-3d20-8661-a2a73eff9ced@web.de","threadId":"53967","inReplyTo":"fdf66f66-b7ed-f733-22d0-23ec17209f40@web.de","subject":"Re: [PATCH] midx: use buffered I/O to talk to pack-objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-08-04T04:31:24Z","receivedAt":"2020-08-04T04:31:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.08.20 um 00:27 schrieb René Scharfe:\n> Am 03.08.20 um 20:10 schrieb Johannes Sixt:\n>> Am 02.08.20 um 18:11 schrieb Chris Torek:\n>>> On Sun, Aug 2, 2020 at 7:40 AM René Scharfe wrote:\n>>>> @@ -1443,10 +1446,15 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n>>>> continue;\n>>>>\n>>>> nth_midxed_object_oid(&oid, m, i);\n>>>> - xwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);\n>>>> - xwrite(cmd.in, \"\\n\", 1);\n>>>> + fprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n>>>> + }\n>>>> +\n>>>> + if (fclose(cmd_in)) {\n>>>> + error_errno(_(\"could not close stdin of pack-objects\"));\n>>>> + result = 1;\n>>>> + finish_command(&cmd);\n>>>> + goto cleanup;\n>>>> }\n>>>> - close(cmd.in);\n>>>>\n>>>> if (finish_command(&cmd)) {\n>>>> error(_(\"could not finish pack-objects\"));\n>>>> --\n>>>> 2.28.0\n\n>> We need this explicit test after each fprintf anyway because SIGPIPE may\n>> be ignored, and then writing fails with EPIPE. On Windows, this is\n>> doubly important because we do not have SIGPIPE at all (and always see\n>> EPIPE), but we see EPIPE only on the first failed write; subsequent\n>> writes produce EINVAL.\n>\n> Why is this important? The current code doesn't care about it, at\n> least. It does care about EINTR, though.\n\nAh, that's the point, right? You want to *ignore* EPIPE, because the\nfailed pack-objects process at the other end will have died with a\n(hopefully) useful error message already.\n\nOK, so we also need a fprintf() wrapper that retries on EINTR, ignores\nEPIPE and exits early if the error indicator is set?\n\nSomehow staying with write(2) and its friends and just adding strbuf\nbased buffering looks attractive to me now. :-/\n\nRené\n"},{"id":"402793","messageId":"xmqqd047t4ab.fsf@gitster.c.googlers.com","threadId":"53967","inReplyTo":"90b839cd-29e0-3d20-8661-a2a73eff9ced@web.de","subject":"Re: [PATCH] midx: use buffered I/O to talk to pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-04T04:37:32Z","receivedAt":"2020-08-04T04:37:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Somehow staying with write(2) and its friends and just adding strbuf\n> based buffering looks attractive to me now. :-/\n\nIndeed :-/\n"},{"id":"403364","messageId":"3655b3c8-9b60-daec-c3a2-6e3703ec5b3f@web.de","threadId":"53967","inReplyTo":"90541678-f412-89a1-2ee0-4cae30e26551@gmail.com","subject":"Re: [PATCH] midx: use buffered I/O to talk to pack-objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-08-11T16:08:29Z","receivedAt":"2020-08-11T16:08:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.08.20 um 14:39 schrieb Derrick Stolee:\n> On 8/2/2020 10:38 AM, René Scharfe wrote:\n>> Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,\n>> 2016-06-08), significantly reduce the number of system calls and\n>> simplify the code for sending object IDs to pack-objects by using\n>> stdio's buffering and handling errors after the loop.\n>\n> Good find. Thanks for doing this important cleanup.\n>\n> Outside of Chris's other feedback, this looks like an obviously\n> correct transformation.\n\nI spent a surprising amount of time trying to find a solution that is\neasy to use and allows precise error handling. But now I get second\nthoughts. The main selling point of buffering is better performance,\nwhich is achieved by reducing the number of system calls. How much\nbetter actually?\n\nSo I get this in my Git repo clone without this patch:\n\n $ strace --summary-only --trace=write git multi-pack-index repack --no-progress\n % time seconds usecs/call calls errors syscall\n ------ ----------- ----------- --------- --------- ----------------\n 100.00 2.237478 2 921650 write\n ------ ----------- ----------- --------- --------- ----------------\n 100.00 2.237478 921650 total\n\nAnd here's the same with the patch:\n\n % time seconds usecs/call calls errors syscall\n ------ ----------- ----------- --------- --------- ----------------\n 100.00 0.013293 2 4613 write\n ------ ----------- ----------- --------- --------- ----------------\n 100.00 0.013293 4613 total\n\nAwesome, right? write(2) calls are down by a factor of almost 200 and\nthe time spent on them is reduced significantly, as advertised. Let's\nask hyperfine for a second opinion though. Without this patch:\n\n Benchmark #1: git multi-pack-index repack --no-progress\n Time (mean ± σ): 1.652 s ± 0.206 s [User: 1.383 s, System: 0.317 s]\n Range (min … max): 1.426 s … 1.890 s 10 runs\n\nAnd the same with this patch applied:\n\n Time (mean ± σ): 1.635 s ± 0.199 s [User: 1.363 s, System: 0.204 s]\n Range (min … max): 1.430 s … 1.871 s 10 runs\n\nOK, so system time is down by ca. 50%, but the total duration is\nbasically unchanged. It seems strace added quite some overhead to our\nmeasurement above.\n\nAnyway, now I wonder if adding our own buffer on top if the\nOS-internal pipe buffer is actually worth it. The numbers above are\nfrom Debian testing , by the way. Perhaps buffering still pays off on\noperating systems with slower pipes..\n\nRené\n"},{"id":"403365","messageId":"bf1fcdb6-f7bb-4235-8403-a36c38180ee7@gmail.com","threadId":"53967","inReplyTo":"3655b3c8-9b60-daec-c3a2-6e3703ec5b3f@web.de","subject":"Re: [PATCH] midx: use buffered I/O to talk to pack-objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-11T17:14:03Z","receivedAt":"2020-08-11T17:14:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/11/2020 12:08 PM, René Scharfe wrote:\n> Am 03.08.20 um 14:39 schrieb Derrick Stolee:\n>> On 8/2/2020 10:38 AM, René Scharfe wrote:\n>>> Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,\n>>> 2016-06-08), significantly reduce the number of system calls and\n>>> simplify the code for sending object IDs to pack-objects by using\n>>> stdio's buffering and handling errors after the loop.\n>>\n>> Good find. Thanks for doing this important cleanup.\n>>\n>> Outside of Chris's other feedback, this looks like an obviously\n>> correct transformation.\n> \n> I spent a surprising amount of time trying to find a solution that is\n> easy to use and allows precise error handling. But now I get second\n> thoughts. The main selling point of buffering is better performance,\n> which is achieved by reducing the number of system calls. How much\n> better actually?\n> \n> So I get this in my Git repo clone without this patch:\n> \n> $ strace --summary-only --trace=write git multi-pack-index repack --no-progress\n> % time seconds usecs/call calls errors syscall\n> ------ ----------- ----------- --------- --------- ----------------\n> 100.00 2.237478 2 921650 write\n> ------ ----------- ----------- --------- --------- ----------------\n> 100.00 2.237478 921650 total\n> \n> And here's the same with the patch:\n> \n> % time seconds usecs/call calls errors syscall\n> ------ ----------- ----------- --------- --------- ----------------\n> 100.00 0.013293 2 4613 write\n> ------ ----------- ----------- --------- --------- ----------------\n> 100.00 0.013293 4613 total\n> \n> Awesome, right? write(2) calls are down by a factor of almost 200 and\n> the time spent on them is reduced significantly, as advertised. Let's\n> ask hyperfine for a second opinion though. Without this patch:\n> \n> Benchmark #1: git multi-pack-index repack --no-progress\n> Time (mean ± σ): 1.652 s ± 0.206 s [User: 1.383 s, System: 0.317 s]\n> Range (min … max): 1.426 s … 1.890 s 10 runs\n> \n> And the same with this patch applied:\n> \n> Time (mean ± σ): 1.635 s ± 0.199 s [User: 1.363 s, System: 0.204 s]\n> Range (min … max): 1.430 s … 1.871 s 10 runs\n> \n> OK, so system time is down by ca. 50%, but the total duration is\n> basically unchanged. It seems strace added quite some overhead to our\n> measurement above.\n> \n> Anyway, now I wonder if adding our own buffer on top if the\n> OS-internal pipe buffer is actually worth it. The numbers above are\n> from Debian testing , by the way. Perhaps buffering still pays off on\n> operating systems with slower pipes..\n\nFor what it's worth, I took your patch and applied it on Git for Windows\nand tested 'git multi-pack-index repack' on my copy of the Git repo\n(which includes Git for Windows and microsoft/git for a total of\n1.7 million objects) and saw the time improve from 22.3s to 16.6s!\n\nThe \"Enumerating objects\" progress bar was visibly faster when I was\nwatching the command.\n\nI was not expecting such a huge speed bump, seeing how the objects\nare being repacked, so this command includes complicated processes\nlike delta compression an zlib compression.\n\nThanks! This is definitely worth the speed boost on Windows.\n\n-Stolee\n"},{"id":"403473","messageId":"9162c1cb-36fd-3203-ec58-4bd1501938d0@web.de","threadId":"53967","inReplyTo":"c5920e08-b7dd-e870-f99e-225d0aafc663@web.de","subject":"[PATCH v2] midx: use buffered I/O to talk to pack-objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-08-12T16:52:54Z","receivedAt":"2020-08-12T16:53:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,\n2016-06-08), significantly reduce the number of system calls and\nsimplify the code for sending object IDs to pack-objects by using\nstdio's buffering.\n\nHelped-by: Chris Torek \nHelped-by: Johannes Sixt \nEncouraged-by: Derrick Stolee \nSigned-off-by: René Scharfe \n---\nChange since v1:\n- Removed error handling to match the original code.\n\n midx.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex a5fb797edee..0f95c3d8526 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1383,6 +1383,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tuint32_t i;\n \tunsigned char *include_pack;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tFILE *cmd_in;\n \tstruct strbuf base_name = STRBUF_INIT;\n \tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n\n@@ -1435,6 +1436,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t\tgoto cleanup;\n \t}\n\n+\tcmd_in = xfdopen(cmd.in, \"w\");\n+\n \tfor (i = 0; i < m->num_objects; i++) {\n \t\tstruct object_id oid;\n \t\tuint32_t pack_int_id = nth_midxed_pack_int_id(m, i);\n@@ -1443,10 +1446,9 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t\t\tcontinue;\n\n \t\tnth_midxed_object_oid(&oid, m, i);\n-\t\txwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);\n-\t\txwrite(cmd.in, \"\\n\", 1);\n+\t\tfprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n \t}\n-\tclose(cmd.in);\n+\tfclose(cmd_in);\n\n \tif (finish_command(&cmd)) {\n \t\terror(_(\"could not finish pack-objects\"));\n--\n2.28.0\n"},{"id":"403503","messageId":"xmqqh7t7623d.fsf@gitster.c.googlers.com","threadId":"53967","inReplyTo":"9162c1cb-36fd-3203-ec58-4bd1501938d0@web.de","subject":"Re: [PATCH v2] midx: use buffered I/O to talk to pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T20:28:22Z","receivedAt":"2020-08-12T20:28:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> \t\tnth_midxed_object_oid(&oid, m, i);\n> -\t\txwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);\n> -\t\txwrite(cmd.in, \"\\n\", 1);\n> +\t\tfprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n\nI do think it is silly to send an object name and terminating LF in\ntwo different system calls per object.\n\nThe original uses xwrite() so that it does not have to worry about\nhaving to restart interrupted system calls and such. Do we need to\ndo that ourselves now or does the stdio layer take care of it for\nus?\n\n> \t}\n> -\tclose(cmd.in);\n> +\tfclose(cmd_in);\n>\n> \tif (finish_command(&cmd)) {\n> \t\terror(_(\"could not finish pack-objects\"));\n> --\n> 2.28.0\n"},{"id":"403504","messageId":"xmqqd03v61xd.fsf@gitster.c.googlers.com","threadId":"53967","inReplyTo":"xmqqh7t7623d.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] midx: use buffered I/O to talk to pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T20:31:58Z","receivedAt":"2020-08-12T20:32:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> René Scharfe writes:\n>\n>> \t\tnth_midxed_object_oid(&oid, m, i);\n>> -\t\txwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);\n>> -\t\txwrite(cmd.in, \"\\n\", 1);\n>> +\t\tfprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n>\n> I do think it is silly to send an object name and terminating LF in\n> two different system calls per object.\n>\n> The original uses xwrite() so that it does not have to worry about\n> having to restart interrupted system calls and such.\n\nOops. There is not much in \"and such\". xwrite() only restarts but\nthe caller must deal with truncated write, which the original does\nnot do.\n\n"},{"id":"403534","messageId":"20200813090635.GC3092220@coredump.intra.peff.net","threadId":"53967","inReplyTo":"xmqqh7t7623d.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] midx: use buffered I/O to talk to pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-13T09:06:35Z","receivedAt":"2020-08-13T09:06:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 12, 2020 at 01:28:22PM -0700, Junio C Hamano wrote:\n\n> René Scharfe writes:\n> \n> > \t\tnth_midxed_object_oid(&oid, m, i);\n> > -\t\txwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);\n> > -\t\txwrite(cmd.in, \"\\n\", 1);\n> > +\t\tfprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n> \n> I do think it is silly to send an object name and terminating LF in\n> two different system calls per object.\n> \n> The original uses xwrite() so that it does not have to worry about\n> having to restart interrupted system calls and such. Do we need to\n> do that ourselves now or does the stdio layer take care of it for\n> us?\n\nI think we're OK in this instance because we are not expecting writes to\nhappen at any given moment. We might do a partial write() for any given\nfprintf(), but we're OK as long as by the fflush() at the end everything\nis written.\n\nFor more general cases where we do care about ordering (e.g., if we were\ninterleaving writes and reads with something like cat-file), we'd need\nto fflush() after each write. And I'd expect fflush() to retry across\npartial write() or EINTR anyway as a quality-of-implementation thing.\nWhich doesn't make it true, but glibc empirically seems to behave that\nway, and I think we'd be better off adding a transparent wrapper to\nfflush() on any system that doesn't.\n\n-Peff\n"},{"id":"403535","messageId":"20200813091103.GD3092220@coredump.intra.peff.net","threadId":"53967","inReplyTo":"xmqqd03v61xd.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] midx: use buffered I/O to talk to pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-13T09:11:03Z","receivedAt":"2020-08-13T09:11:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 12, 2020 at 01:31:58PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > René Scharfe writes:\n> >\n> >> \t\tnth_midxed_object_oid(&oid, m, i);\n> >> -\t\txwrite(cmd.in, oid_to_hex(&oid), the_hash_algo->hexsz);\n> >> -\t\txwrite(cmd.in, \"\\n\", 1);\n> >> +\t\tfprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n> >\n> > I do think it is silly to send an object name and terminating LF in\n> > two different system calls per object.\n> >\n> > The original uses xwrite() so that it does not have to worry about\n> > having to restart interrupted system calls and such.\n> \n> Oops. There is not much in \"and such\". xwrite() only restarts but\n> the caller must deal with truncated write, which the original does\n> not do.\n\nYeah, so I think the new code should be an improvement there, because it\nwill eventually retry the write when the buffer fills, or when we\neventually flush on close (my \"everything written by the time we hit\nfflush()\" in the previous mail really should have been \"...by the time\nwe hit fclose()\"). Maybe another reason to use stdio. :)\n\nThe original also doesn't handle \"real\" errors at all (stuff besides\nEINTR, etc). Nor does the rewrite. I guess it's unlikely to see a write\nerror here that doesn't also involve pack-objects returning a non-zero\nexit code.\n\n-Peff\n"}]}