{"thread":{"id":"41525","subject":"[PATCH] strbuf_write: omit system call when length is zero","startedAt":"2016-02-25T22:34:38Z","lastAt":"2016-02-26T17:09:50Z","messageCount":5,"participants":["Stefan Beller","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"279395","messageId":"1456439678-5433-1-git-send-email-sbeller@google.com","threadId":"41525","inReplyTo":null,"subject":"[PATCH] strbuf_write: omit system call when length is zero","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-25T22:34:38Z","receivedAt":"2016-02-25T22:34:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In case the length of the buffer is zero, we do not need to call the\nfwrite system call as a performance improvement.\n\nSigned-off-by: Stefan Beller \n---\n\n This applies on top of v17 for origin/sb/submodule-parallel-update.\n \n In case there are other reasons for origin/sb/submodule-parallel-update\n to need a reroll I'll squash it in. But as this is a pure performance\n optimization in a case we are not running into with that series and that\n series is clashing with Davids refs backend series, I figure we may not\n want to have a reroll for this fix alone.\n \n Thanks,\n Stefan\n \n\n strbuf.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 71345cd..5f6da82 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -397,7 +397,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n \n ssize_t strbuf_write(struct strbuf *sb, FILE *f)\n {\n-\treturn fwrite(sb->buf, 1, sb->len, f);\n+\treturn sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;\n }\n \n \n-- \n2.7.2.374.ga5f0819.dirty\n"},{"id":"279418","messageId":"xmqqk2lsmlwp.fsf@gitster.mtv.corp.google.com","threadId":"41525","inReplyTo":"1456439678-5433-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] strbuf_write: omit system call when length is zero","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-25T23:04:54Z","receivedAt":"2016-02-25T23:04:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> In case the length of the buffer is zero, we do not need to call the\n> fwrite system call as a performance improvement.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> This applies on top of v17 for origin/sb/submodule-parallel-update.\n> \n> In case there are other reasons for origin/sb/submodule-parallel-update\n> to need a reroll I'll squash it in. But as this is a pure performance\n> optimization in a case we are not running into with that series and that\n> series is clashing with Davids refs backend series, I figure we may not\n> want to have a reroll for this fix alone.\n\nYeah, I tend to agree that this change by itself is probably not\nworth much.\n\nWhile I agree with the idea of passing the output from children thru\nwith as little modification as possible, I think that goal does not\nhave to be incompatible with the idea you and Jonathan had to ensure\nthat the output is safe in the presence of incomplete last line.\n\nI think the output made from pp_collect_finished() for \"other\nfinished processes\" is known to have the very last part from\nfinished child, so it would be easy to make sure by calling\nstrbuf_complete_line() that pp->children[i].err is terminated.\n\nI haven't thought it through how the output from CP_WORKING process\nin pp_output() should be kept track of, though.\n\nIt might be just a matter of adding a bit \"incomplete_last_line\" to\nthe parallel_process.children struct and maintain it every time\npp_output() writes something out (we cannot add an extra newline in\npp_output() because we don't know if the process will have further\noutput, but we can keep track of the fact that the last output did\nnot have the final newline). In pp_collect_finished(), we can use\nthe bit and the contents of pp->children[i].err to decide if we need\nto call strbuf_complete_line() if we did so.\n\nOr something like that ;-)\n"},{"id":"279457","messageId":"CACsJy8DgjmdX681fRwB-JajPBRN+cNy=jFwEDW6D-dXhNi=_6g@mail.gmail.com","threadId":"41525","inReplyTo":"1456439678-5433-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] strbuf_write: omit system call when length is zero","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-26T00:47:44Z","receivedAt":"2016-02-26T00:47:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 26, 2016 at 5:34 AM, Stefan Beller wrote:\n> In case the length of the buffer is zero, we do not need to call the\n> fwrite system call as a performance improvement.\n\nfwrite is a libc call, not system call. Are you sure it always calls\nwrite() (assuming buffering is off)?\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> This applies on top of v17 for origin/sb/submodule-parallel-update.\n>\n> In case there are other reasons for origin/sb/submodule-parallel-update\n> to need a reroll I'll squash it in. But as this is a pure performance\n> optimization in a case we are not running into with that series and that\n> series is clashing with Davids refs backend series, I figure we may not\n> want to have a reroll for this fix alone.\n>\n> Thanks,\n> Stefan\n>\n>\n> strbuf.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index 71345cd..5f6da82 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -397,7 +397,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n>\n> ssize_t strbuf_write(struct strbuf *sb, FILE *f)\n> {\n> - return fwrite(sb->buf, 1, sb->len, f);\n> + return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;\n> }\n>\n>\n> --\n> 2.7.2.374.ga5f0819.dirty\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n\n\n-- \nDuy\n"},{"id":"279458","messageId":"xmqqy4a8l04f.fsf@gitster.mtv.corp.google.com","threadId":"41525","inReplyTo":"CACsJy8DgjmdX681fRwB-JajPBRN+cNy=jFwEDW6D-dXhNi=_6g@mail.gmail.com","subject":"Re: [PATCH] strbuf_write: omit system call when length is zero","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-26T01:40:48Z","receivedAt":"2016-02-26T01:40:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Feb 26, 2016 at 5:34 AM, Stefan Beller wrote:\n>> In case the length of the buffer is zero, we do not need to call the\n>> fwrite system call as a performance improvement.\n>\n> fwrite is a libc call, not system call. Are you sure it always calls\n> write() (assuming buffering is off)?\n\nI do not think so, but I suspect that the patch misstates its\nrationale (I said I get uncomfortable every time I see a function\nthat takes size and nelem separately used by a caller that can\npotentially pass nleme=0, when I wondered it it is OK that no caller\nof this funtion checks its return value).\n\n\n>\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>>\n>> This applies on top of v17 for origin/sb/submodule-parallel-update.\n>>\n>> In case there are other reasons for origin/sb/submodule-parallel-update\n>> to need a reroll I'll squash it in. But as this is a pure performance\n>> optimization in a case we are not running into with that series and that\n>> series is clashing with Davids refs backend series, I figure we may not\n>> want to have a reroll for this fix alone.\n>>\n>> Thanks,\n>> Stefan\n>>\n>>\n>> strbuf.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/strbuf.c b/strbuf.c\n>> index 71345cd..5f6da82 100644\n>> --- a/strbuf.c\n>> +++ b/strbuf.c\n>> @@ -397,7 +397,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n>>\n>> ssize_t strbuf_write(struct strbuf *sb, FILE *f)\n>> {\n>> - return fwrite(sb->buf, 1, sb->len, f);\n>> + return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;\n>> }\n>>\n>>\n>> --\n>> 2.7.2.374.ga5f0819.dirty\n>>\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"279537","messageId":"CAGZ79kZYqf3K5mR0_1eeQHO-n675BRfEQvC_-YCb+gOPTJ5xXA@mail.gmail.com","threadId":"41525","inReplyTo":"xmqqy4a8l04f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] strbuf_write: omit system call when length is zero","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-26T17:09:50Z","receivedAt":"2016-02-26T17:09:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 25, 2016 at 5:40 PM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> On Fri, Feb 26, 2016 at 5:34 AM, Stefan Beller wrote:\n>>> In case the length of the buffer is zero, we do not need to call the\n>>> fwrite system call as a performance improvement.\n>>\n>> fwrite is a libc call, not system call. Are you sure it always calls\n>> write() (assuming buffering is off)?\n>\n> I do not think so, but I suspect that the patch misstates its\n> rationale (I said I get uncomfortable every time I see a function\n> that takes size and nelem separately used by a caller that can\n> potentially pass nleme=0, when I wondered it it is OK that no caller\n> of this funtion checks its return value).\n>\n\nThis is exactly what happened. I assumed fwrite would be a system call\neventually.\n"}]}