{"thread":{"id":"56072","subject":"[PATCH 0/3] strbuf.[ch]: add STRBUF_HINT_SIZE, don't hardcode 8192","startedAt":"2021-07-07T10:38:52Z","lastAt":"2021-07-12T17:58:52Z","messageCount":12,"participants":["Ævar Arnfjörð Bjarmason","Han-Wen Nienhuys","Eric Sunshine","Junio C Hamano","Randall S. Becker","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"429414","messageId":"cover-0.3-00000000000-20210707T103712Z-avarab@gmail.com","threadId":"56072","inReplyTo":null,"subject":"[PATCH 0/3] strbuf.[ch]: add STRBUF_HINT_SIZE, don't hardcode 8192","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-07T10:38:39Z","receivedAt":"2021-07-07T10:38:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A rather trivial cleanup series to avoid hardcoding 8192 in N places\nin strbuf.[ch], instead we add a STRBUF_HINT_SIZE macro. Then make\nstrbuf_fread() take a \"hint\" instead of \"size\" (like most similar\nfunctions) so we can pass a size of \"0\" meaning the default hint to\nit.\n\nÆvar Arnfjörð Bjarmason (3):\n  strbuf.[ch]: add STRBUF_HINT_SIZE macro = 8192\n  strbuf.h API users: don't hardcode 8192, use STRBUF_HINT_SIZE\n  strbuf.[ch]: make strbuf_fread() take hint, not size\n\n builtin/am.c |  4 ++--\n cache-tree.c |  2 +-\n commit.c     |  2 +-\n strbuf.c     |  9 +++++----\n strbuf.h     | 14 +++++++++++++-\n 5 files changed, 22 insertions(+), 9 deletions(-)\n\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429415","messageId":"patch-1.3-f5a6c4a2720-20210707T103712Z-avarab@gmail.com","threadId":"56072","inReplyTo":"cover-0.3-00000000000-20210707T103712Z-avarab@gmail.com","subject":"[PATCH 1/3] strbuf.[ch]: add STRBUF_HINT_SIZE macro = 8192","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-07T10:38:40Z","receivedAt":"2021-07-07T10:38:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In b449f4cfc97 (Rework strbuf API and semantics., 2007-09-06) the\nfirst hardcoding of 8192 appeared in strbuf.[ch], then in\nf1696ee398e (Strbuf API extensions and fixes., 2007-09-10) another one\nwas added, and in b4e04fb66e8 (strbuf: add strbuf_read_once to read\nwithout blocking, 2015-12-15) a third.\n\nLet's factor that out into a STRBUF_HINT_SIZE macro, and add a\nstrbuf_hint() helper macro for \"hint ? hint : STRBUF_HINT_SIZE\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n strbuf.c |  6 +++---\n strbuf.h | 11 +++++++++++\n 2 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 4df30b45494..7e9f5fdc4de 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -517,7 +517,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \tsize_t oldlen = sb->len;\n \tsize_t oldalloc = sb->alloc;\n \n-\tstrbuf_grow(sb, hint ? hint : 8192);\n+\tstrbuf_grow(sb, strbuf_hint(hint));\n \tfor (;;) {\n \t\tssize_t want = sb->alloc - sb->len - 1;\n \t\tssize_t got = read_in_full(fd, sb->buf + sb->len, want);\n@@ -532,7 +532,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \t\tsb->len += got;\n \t\tif (got < want)\n \t\t\tbreak;\n-\t\tstrbuf_grow(sb, 8192);\n+\t\tstrbuf_grow(sb, STRBUF_HINT_SIZE);\n \t}\n \n \tsb->buf[sb->len] = '\\0';\n@@ -544,7 +544,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n \tsize_t oldalloc = sb->alloc;\n \tssize_t cnt;\n \n-\tstrbuf_grow(sb, hint ? hint : 8192);\n+\tstrbuf_grow(sb, strbuf_hint(hint));\n \tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n \tif (cnt > 0)\n \t\tstrbuf_setlen(sb, sb->len + cnt);\ndiff --git a/strbuf.h b/strbuf.h\nindex 223ee2094af..ca3c47966a0 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -72,6 +72,17 @@ struct strbuf {\n extern char strbuf_slopbuf[];\n #define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }\n \n+/**\n+ * Various functions take a `size_t hint` to give a hint about the\n+ * file size, to avoid reallocs. This is the default hint size when\n+ * `0` is given. \n+ *\n+ * The strbuf_hint() convenience macro is used internally in the\n+ * API. DO NOT USE any expression with side-effect for 'size'.\n+ */\n+#define STRBUF_HINT_SIZE 8192\n+#define strbuf_hint(size) ((size) ? (size) : STRBUF_HINT_SIZE)\n+\n /*\n  * Predeclare this here, since cache.h includes this file before it defines the\n  * struct.\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429416","messageId":"patch-2.3-a920a9971e8-20210707T103712Z-avarab@gmail.com","threadId":"56072","inReplyTo":"cover-0.3-00000000000-20210707T103712Z-avarab@gmail.com","subject":"[PATCH 2/3] strbuf.h API users: don't hardcode 8192, use STRBUF_HINT_SIZE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-07T10:38:41Z","receivedAt":"2021-07-07T10:38:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change a couple of users of strbuf_init() that pass a hint of 8192 to\npass STRBUF_HINT_SIZE instead.\n\nBoth of these hardcoded occurrences pre-date the use of the strbuf\nAPI. See 5242bcbb638 (Use strbuf API in cache-tree.c, 2007-09-06) and\naf6eb82262e (Use strbuf API in apply, blame, commit-tree and diff,\n2007-09-06).\n\nIn both cases the exact choice of 8192 is rather arbitrary, e.g. for\ncommit buffers I think 1024 or 2048 would probably be a better\ndefault (this commit message is getting this commit close to the\nformer, but I daresay it's already way above the average for git\ncommits).\n\nIn any case, if we ever tweak STRBUF_HINT_SIZE we'll probably do so on\nthe basis of some codease-wide performance tests, so replacing the\nhardcoded value with STRBUF_HINT_SIZE should be safe, they're the same\nnow, and if we change STRBUF_HINT_SIZE in the future this is one of\nthe main codepaths we'll be testing.\n\nAside: It's unfortunate that we don't take a \"hint\" of \"0\" in\nstrbuf_init() to mean \"default\" and e.g. \"-1\" to mean the\nstrbuf_slopbuf (a in STRBUF_INIT). I considered adding that, or\nsplitting them up so you'd do strbuf_init(&buf) for the\nstrbuf_init(&buf, 0) case, or strbuf_init_alloc() for the\nstrbuf_init(&buf, N) case, where N > 0. But that would be a big change\nacross the codebase, so let's punt on that for now.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n cache-tree.c | 2 +-\n commit.c     | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 45e58666afc..d69f6d1c66f 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -335,7 +335,7 @@ static int update_one(struct cache_tree *it,\n \t/*\n \t * Then write out the tree object for this level.\n \t */\n-\tstrbuf_init(&buffer, 8192);\n+\tstrbuf_init(&buffer, STRBUF_HINT_SIZE);\n \n \ti = 0;\n \twhile (i < entries) {\ndiff --git a/commit.c b/commit.c\nindex 8ea55a447fa..b3aab46072a 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1521,7 +1521,7 @@ int commit_tree_extended(const char *msg, size_t msg_len,\n \t/* Not having i18n.commitencoding is the same as having utf-8 */\n \tencoding_is_utf8 = is_encoding_utf8(git_commit_encoding);\n \n-\tstrbuf_init(&buffer, 8192); /* should avoid reallocs for the headers */\n+\tstrbuf_init(&buffer, STRBUF_HINT_SIZE); /* should avoid reallocs for the headers */\n \tstrbuf_addf(&buffer, \"tree %s\\n\", oid_to_hex(tree));\n \n \t/*\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429417","messageId":"patch-3.3-46c65a7ae12-20210707T103712Z-avarab@gmail.com","threadId":"56072","inReplyTo":"cover-0.3-00000000000-20210707T103712Z-avarab@gmail.com","subject":"[PATCH 3/3] strbuf.[ch]: make strbuf_fread() take hint, not size","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-07T10:38:42Z","receivedAt":"2021-07-07T10:38:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the strbuf_fread() function to take a \"size_t hint\" instead of\na \"size_t size\", for consistency with e.g. strbuf_read(). We can then\nchange callers that explicitly passed in our default hint of 8192.\n\nThe strbuf_fread() has not taken a hint ever since it was added in\nb449f4cfc97 (Rework strbuf API and semantics., 2007-09-06), it was\nleft out when strbuf_read() learned to do it in f1696ee398e (Strbuf\nAPI extensions and fixes., 2007-09-10).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/am.c | 4 ++--\n strbuf.c     | 3 ++-\n strbuf.h     | 3 ++-\n 3 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0b2d886c81b..295b9c4080c 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -781,7 +781,7 @@ static int stgit_patch_to_mail(FILE *out, FILE *in, int keep_cr)\n \t}\n \n \tstrbuf_reset(&sb);\n-\twhile (strbuf_fread(&sb, 8192, in) > 0) {\n+\twhile (strbuf_fread(&sb, 0, in) > 0) {\n \t\tfwrite(sb.buf, 1, sb.len, out);\n \t\tstrbuf_reset(&sb);\n \t}\n@@ -898,7 +898,7 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)\n \t}\n \n \tstrbuf_reset(&sb);\n-\twhile (strbuf_fread(&sb, 8192, in) > 0) {\n+\twhile (strbuf_fread(&sb, 0, in) > 0) {\n \t\tfwrite(sb.buf, 1, sb.len, out);\n \t\tstrbuf_reset(&sb);\n \t}\ndiff --git a/strbuf.c b/strbuf.c\nindex 7e9f5fdc4de..af3af7622d1 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -498,10 +498,11 @@ void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags)\n \t}\n }\n \n-size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)\n+size_t strbuf_fread(struct strbuf *sb, size_t hint, FILE *f)\n {\n \tsize_t res;\n \tsize_t oldalloc = sb->alloc;\n+\tsize_t size = strbuf_hint(hint);\n \n \tstrbuf_grow(sb, size);\n \tres = fread(sb->buf + sb->len, 1, size, f);\ndiff --git a/strbuf.h b/strbuf.h\nindex ca3c47966a0..7d178e3c8de 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -442,13 +442,14 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt,\n \n /**\n  * Read a given size of data from a FILE* pointer to the buffer.\n+ * if the size is 0 the default hint is used.\n  *\n  * NOTE: The buffer is rewound if the read fails. If -1 is returned,\n  * `errno` must be consulted, like you would do for `read(3)`.\n  * `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline_*()`\n  * family of functions have the same behaviour as well.\n  */\n-size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *file);\n+size_t strbuf_fread(struct strbuf *sb, size_t hint, FILE *file);\n \n /**\n  * Read the contents of a given file descriptor. The third argument can be\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429419","messageId":"CAFQ2z_MWMokX8hXdOJfiL9Yn=F-QUWe5VHXFEF5DoSBBJu=dmw@mail.gmail.com","threadId":"56072","inReplyTo":"patch-3.3-46c65a7ae12-20210707T103712Z-avarab@gmail.com","subject":"Re: [PATCH 3/3] strbuf.[ch]: make strbuf_fread() take hint, not size","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-07-07T11:47:00Z","receivedAt":"2021-07-07T11:47:16Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Jul 7, 2021 at 12:39 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> Change the strbuf_fread() function to take a \"size_t hint\" instead of\n> a \"size_t size\", for consistency with e.g. strbuf_read(). We can then\n\nMaking strbuf_fread and strbuf_read consistent is weird because they\ndo different things. strbuf_read reads from fd until it's exhausted,\nwhile strbuf_fread reads up to the size specified. So the argument\nisn't really a hint.\n\nshouldn't you rename it to strbuf_fread_once, analogous to strbuf_read_once ?\n\nbonus points for making the argument ordering consistent between\nstrbuf_fread and strbuf_read\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"429444","messageId":"CAPig+cSCKbcK5Dfi3CNh=kNngwG5==2afFPVxy2wA9S_dTa9oQ@mail.gmail.com","threadId":"56072","inReplyTo":"patch-2.3-a920a9971e8-20210707T103712Z-avarab@gmail.com","subject":"Re: [PATCH 2/3] strbuf.h API users: don't hardcode 8192, use STRBUF_HINT_SIZE","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-07T20:10:29Z","receivedAt":"2021-07-07T20:10:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 7, 2021 at 6:38 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> Change a couple of users of strbuf_init() that pass a hint of 8192 to\n> pass STRBUF_HINT_SIZE instead.\n>\n> Both of these hardcoded occurrences pre-date the use of the strbuf\n> API. See 5242bcbb638 (Use strbuf API in cache-tree.c, 2007-09-06) and\n> af6eb82262e (Use strbuf API in apply, blame, commit-tree and diff,\n> 2007-09-06).\n>\n> In both cases the exact choice of 8192 is rather arbitrary, e.g. for\n> commit buffers I think 1024 or 2048 would probably be a better\n> default (this commit message is getting this commit close to the\n> former, but I daresay it's already way above the average for git\n> commits).\n>\n> In any case, if we ever tweak STRBUF_HINT_SIZE we'll probably do so on\n> the basis of some codease-wide performance tests, so replacing the\n\nDid you mean? s/codease/code/\n\n> hardcoded value with STRBUF_HINT_SIZE should be safe, they're the same\n> now, and if we change STRBUF_HINT_SIZE in the future this is one of\n> the main codepaths we'll be testing.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n"},{"id":"429448","messageId":"xmqq5yxlervl.fsf@gitster.g","threadId":"56072","inReplyTo":"CAFQ2z_MWMokX8hXdOJfiL9Yn=F-QUWe5VHXFEF5DoSBBJu=dmw@mail.gmail.com","subject":"Re: [PATCH 3/3] strbuf.[ch]: make strbuf_fread() take hint, not size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-07T21:27:58Z","receivedAt":"2021-07-07T21:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> shouldn't you rename it to strbuf_fread_once, analogous to strbuf_read_once ?\n\nGood point.\n"},{"id":"429452","messageId":"xmqqsg0pda9o.fsf@gitster.g","threadId":"56072","inReplyTo":"patch-1.3-f5a6c4a2720-20210707T103712Z-avarab@gmail.com","subject":"Re: [PATCH 1/3] strbuf.[ch]: add STRBUF_HINT_SIZE macro = 8192","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-07T22:33:39Z","receivedAt":"2021-07-07T22:33:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> In b449f4cfc97 (Rework strbuf API and semantics., 2007-09-06) the\n> first hardcoding of 8192 appeared in strbuf.[ch], then in\n> f1696ee398e (Strbuf API extensions and fixes., 2007-09-10) another one\n> was added, and in b4e04fb66e8 (strbuf: add strbuf_read_once to read\n> without blocking, 2015-12-15) a third.\n>\n> Let's factor that out into a STRBUF_HINT_SIZE macro, and add a\n> strbuf_hint() helper macro for \"hint ? hint : STRBUF_HINT_SIZE\".\n\nI guess hiding 8k that appears twice in strbuf.c behind a macro or\nsymbolic constant might makes sense, but \n\n - STRBUF_HINT_SIZE is a misnomer.  It is the default when the\n   caller didn't give you a useful hint.  It is STRBUF_NO_HINT_SIZE,\n   because it is the size that is used when there is no hint, or\n   better yet, call it STRBUF_DEFAULT_READ_SIZE, perhaps?\n\n - strbuf_hint() is a misnomer for the same reason.\n   strbuf_read_size(hint) perhaps, but for two or three callers,\n   spelling out (hint ? hint : STRBUF_DEFAULT_READ_SIZE) is easier\n   to understand.\n\n - Both STRBUF_HINT_SIZE and strbuf_hint() have no reason to be\n   visible outside the implementation of strbuf.c; the whole reason\n   why such a \"the caller does not care, so let's use this logic to\n   come up with the default value\" exists is so that caller does not\n   have to know and instead pass \"0\", which has nothing to do with\n   the actual value used.\n\n - Also, there is no reason why strbuf_read() and strbuf_read_once()\n   must share the same default when the caller did not give any\n   hint; replacing these occurrences of 8k with a single constant\n   may give a false impression that they must match.  This is merely\n   an observation and not an objection, because the caller by\n   passing 0 is saying they do not care at all, so while we can use\n   arbitrary and different random values in these two functions, we\n   can use the same arbitrary value as well.\n\n"},{"id":"429454","messageId":"xmqqo8bdda2j.fsf@gitster.g","threadId":"56072","inReplyTo":"patch-2.3-a920a9971e8-20210707T103712Z-avarab@gmail.com","subject":"Re: [PATCH 2/3] strbuf.h API users: don't hardcode 8192, use STRBUF_HINT_SIZE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-07T22:37:56Z","receivedAt":"2021-07-07T22:38:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Change a couple of users of strbuf_init() that pass a hint of 8192 to\n> pass STRBUF_HINT_SIZE instead.\n>\n> Both of these hardcoded occurrences pre-date the use of the strbuf\n> API. See 5242bcbb638 (Use strbuf API in cache-tree.c, 2007-09-06) and\n> af6eb82262e (Use strbuf API in apply, blame, commit-tree and diff,\n> 2007-09-06).\n>\n> In both cases the exact choice of 8192 is rather arbitrary, e.g. for\n> commit buffers I think 1024 or 2048 would probably be a better\n> default (this commit message is getting this commit close to the\n> former, but I daresay it's already way above the average for git\n> commits).\n\nYes, they are arbitrary within the context of these callers.\n\nI do not think using STRBUF_HINT_SIZE macro in them is the right\nthing to do at all, as there is no reason to think that the best\nvalue for the write chunk sizes in these codepath has any linkage to\nthe best value for the read chunk sizes used by strbuf_read() at\nall.  When benchmarking reveals that the best default size for\nstrbuf_read() is 16k, you'd update STRBUF_HINT_SIZE to 16k, but how\ndo you tell that it also happens to be the best write buffer size\nfor the cache-tree writeout codepath (answer: you don't)?\n\n\n\n"},{"id":"429456","messageId":"xmqqim1ld8lj.fsf@gitster.g","threadId":"56072","inReplyTo":"xmqqo8bdda2j.fsf@gitster.g","subject":"Re: [PATCH 2/3] strbuf.h API users: don't hardcode 8192, use STRBUF_HINT_SIZE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-07T23:09:44Z","receivedAt":"2021-07-07T23:09:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Change a couple of users of strbuf_init() that pass a hint of 8192 to\n>> pass STRBUF_HINT_SIZE instead.\n>>\n>> Both of these hardcoded occurrences pre-date the use of the strbuf\n>> API. See 5242bcbb638 (Use strbuf API in cache-tree.c, 2007-09-06) and\n>> af6eb82262e (Use strbuf API in apply, blame, commit-tree and diff,\n>> 2007-09-06).\n>>\n>> In both cases the exact choice of 8192 is rather arbitrary, e.g. for\n>> commit buffers I think 1024 or 2048 would probably be a better\n>> default (this commit message is getting this commit close to the\n>> former, but I daresay it's already way above the average for git\n>> commits).\n>\n> Yes, they are arbitrary within the context of these callers.\n>\n> I do not think using STRBUF_HINT_SIZE macro in them is the right\n> thing to do at all, as there is no reason to think that the best\n> value for the write chunk sizes in these codepath has any linkage to\n> the best value for the read chunk sizes used by strbuf_read() at\n> all.  When benchmarking reveals that the best default size for\n> strbuf_read() is 16k, you'd update STRBUF_HINT_SIZE to 16k, but how\n> do you tell that it also happens to be the best write buffer size\n> for the cache-tree writeout codepath (answer: you don't)?\n\nHaving said all that, I wouldn't be so opposed to an approach that\n\n - declares that we need only one \"default I/O buffer size\";\n\n - declares that the appropriate size for it is 8192;\n\n - #define DEFAULT_IO_SIZE 8192;\n\n - does something like your [PATCH 1/3], but not limited to strbuf\n   API, and\n\n - covers also the writeout codepath of cache-tree, etc. that uses\n   hardcoded I/O buffer size.\n\nThe biggest trouble I had with the posted patches, especially the\n[PATCH 2/3], was that I am quite sure that you wouldn't have used\nSTRBUF_HINT_SIZE in the cache-tree writeout codepath or commit-tree\ncodepath if they didn't use strbuf as a convenient way to get an\nelastic buffer.  The more relevant commonality across codepaths that\nuse 8192 is that the constant is used for sizing the I/O buffer, and\nI got an impression that the 3-patch series posted did an incomplete\njob of touching some that happen to use strbuf.\n\nAn approach that concentrated on the \"right\" commonality, i.e. we\nhave hardcoded magic constants for I/O buffer sizing, would have\ncovered copy.c, diff-delta.c, http-backend.c etc. that do not use\nstrbuf API where they have hardcoded 8k constants.\n\nThanks.\n\n"},{"id":"429465","messageId":"057901d77386$f9496e30$ebdc4a90$@nexbridge.com","threadId":"56072","inReplyTo":"xmqqo8bdda2j.fsf@gitster.g","subject":"RE: [PATCH 2/3] strbuf.h API users: don't hardcode 8192, use STRBUF_HINT_SIZE","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-07-07T23:22:32Z","receivedAt":"2021-07-07T23:22:51Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On July 7, 2021 6:38 PM, Junio C Hamano wrote:\n>To: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>Cc: git@vger.kernel.org; Jeff King <peff@peff.net>\n>Subject: Re: [PATCH 2/3] strbuf.h API users: don't hardcode 8192, use STRBUF_HINT_SIZE\n>\n>Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Change a couple of users of strbuf_init() that pass a hint of 8192 to\n>> pass STRBUF_HINT_SIZE instead.\n>>\n>> Both of these hardcoded occurrences pre-date the use of the strbuf\n>> API. See 5242bcbb638 (Use strbuf API in cache-tree.c, 2007-09-06) and\n>> af6eb82262e (Use strbuf API in apply, blame, commit-tree and diff,\n>> 2007-09-06).\n>>\n>> In both cases the exact choice of 8192 is rather arbitrary, e.g. for\n>> commit buffers I think 1024 or 2048 would probably be a better default\n>> (this commit message is getting this commit close to the former, but I\n>> daresay it's already way above the average for git commits).\n>\n>Yes, they are arbitrary within the context of these callers.\n>\n>I do not think using STRBUF_HINT_SIZE macro in them is the right thing to do at all, as there is no reason to think that the best value for\n>the write chunk sizes in these codepath has any linkage to the best value for the read chunk sizes used by strbuf_read() at all.  When\n>benchmarking reveals that the best default size for\n>strbuf_read() is 16k, you'd update STRBUF_HINT_SIZE to 16k, but how do you tell that it also happens to be the best write buffer size for\n>the cache-tree writeout codepath (answer: you don't)?\n\nAnd benchmark results are going to be highly platform dependent, as we have seen with our exotic platform.\n-Randall\n\n\n"},{"id":"429806","messageId":"YOyC2gJ4PWCTepKn@coredump.intra.peff.net","threadId":"56072","inReplyTo":"xmqqo8bdda2j.fsf@gitster.g","subject":"Re: [PATCH 2/3] strbuf.h API users: don't hardcode 8192, use STRBUF_HINT_SIZE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-12T17:58:50Z","receivedAt":"2021-07-12T17:58:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 07, 2021 at 03:37:56PM -0700, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n> \n> > Change a couple of users of strbuf_init() that pass a hint of 8192 to\n> > pass STRBUF_HINT_SIZE instead.\n> >\n> > Both of these hardcoded occurrences pre-date the use of the strbuf\n> > API. See 5242bcbb638 (Use strbuf API in cache-tree.c, 2007-09-06) and\n> > af6eb82262e (Use strbuf API in apply, blame, commit-tree and diff,\n> > 2007-09-06).\n> >\n> > In both cases the exact choice of 8192 is rather arbitrary, e.g. for\n> > commit buffers I think 1024 or 2048 would probably be a better\n> > default (this commit message is getting this commit close to the\n> > former, but I daresay it's already way above the average for git\n> > commits).\n> \n> Yes, they are arbitrary within the context of these callers.\n> \n> I do not think using STRBUF_HINT_SIZE macro in them is the right\n> thing to do at all, as there is no reason to think that the best\n> value for the write chunk sizes in these codepath has any linkage to\n> the best value for the read chunk sizes used by strbuf_read() at\n> all.  When benchmarking reveals that the best default size for\n> strbuf_read() is 16k, you'd update STRBUF_HINT_SIZE to 16k, but how\n> do you tell that it also happens to be the best write buffer size\n> for the cache-tree writeout codepath (answer: you don't)?\n\nBeing cc'd on this series, I feel compelled to respond with some review.\nBut I'm in such agreement with what you said here (and downthread, and\nalso in your response to patch 1) that I can only add a lame \"me too\". :)\n\n-Peff\n"}]}