{"thread":{"id":"61661","subject":"[PATCH] cat-file: reduce write calls for unfiltered blobs","startedAt":"2024-06-21T02:05:04Z","lastAt":"2024-06-21T19:45:36Z","messageCount":6,"participants":["Eric Wong","Jeff King","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"497445","messageId":"20240621020457.1081233-1-e@80x24.org","threadId":"61661","inReplyTo":null,"subject":"[PATCH] cat-file: reduce write calls for unfiltered blobs","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-06-21T02:04:57Z","receivedAt":"2024-06-21T02:05:04Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"While the --buffer switch is useful for non-interactive batch use,\nbuffering doesn't work with processes using request-response loops since\nidle times are unpredictable between requests.\n\nFor unfiltered blobs, our streaming interface now appends the initial\nblob data directly into the scratch buffer used for object info.\nFurthermore, the final blob chunk can hold the output delimiter before\nmaking the final write(2).\n\nWhile the same syscall reduction can be done with stdio buffering by\nadding fflush(3) after writing the output delimiter, stdio use requires\nadditional memory copies for the blob contents since it's not possible\nfor our streaming interface to write directly to stdio internal buffers.\n\nFor the reader process, this reduces read(2) syscalls by up to 67% in\nthe best case for small blobs.  Unfortunately my real-world tests on\nnormal data only showed only a ~20% reduction in read(2) syscalls on the\nreader side due to larger blobs and scheduler unpredictability.  Time\nimprovements only came out to roughly 0.5% on my laptop, but this may be\nmore noticeable on systems where syscalls are more expensive.\n\nwritev(2) was also considered, but it is not portable and detrimental\ngiven the way our streaming API works.  writev(2) might make more sense\nfor filtered outputs or reading non-blob data.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n builtin/cat-file.c | 36 +++++++++++++++++++++------------\n streaming.c        | 50 ++++++++++++++++++++++++++++++++++++++++++++++\n streaming.h        |  1 +\n 3 files changed, 74 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 43a1d7ac49..23db5c6a7a 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -87,9 +87,10 @@ static int filter_object(const char *path, unsigned mode,\n \treturn 0;\n }\n \n-static int stream_blob(const struct object_id *oid)\n+static int stream_blob(struct strbuf *scratch, const struct object_id *oid)\n {\n-\tif (stream_blob_to_fd(1, oid, NULL, 0))\n+\tif (scratch ? stream_blob_to_strbuf_fd(1, scratch, oid) :\n+\t\t\tstream_blob_to_fd(1, oid, NULL, 0))\n \t\tdie(\"unable to stream %s to stdout\", oid_to_hex(oid));\n \treturn 0;\n }\n@@ -195,7 +196,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \t\t}\n \n \t\tif (type == OBJ_BLOB) {\n-\t\t\tret = stream_blob(&oid);\n+\t\t\tret = stream_blob(NULL, &oid);\n \t\t\tgoto cleanup;\n \t\t}\n \t\tbuf = repo_read_object_file(the_repository, &oid, &type,\n@@ -237,7 +238,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \t\t\t\toidcpy(&blob_oid, &oid);\n \n \t\t\tif (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB) {\n-\t\t\t\tret = stream_blob(&blob_oid);\n+\t\t\t\tret = stream_blob(NULL, &blob_oid);\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \t\t\t/*\n@@ -376,7 +377,15 @@ static void batch_write(struct batch_options *opt, const void *data, int len)\n \t\twrite_or_die(1, data, len);\n }\n \n-static void print_object_or_die(struct batch_options *opt, struct expand_data *data)\n+static void flush_scratch(struct batch_options *opt, struct strbuf *scratch)\n+{\n+\tbatch_write(opt, scratch->buf, scratch->len);\n+\tstrbuf_reset(scratch);\n+}\n+\n+static void print_object_or_die(struct strbuf *scratch,\n+\t\t\t\tstruct batch_options *opt,\n+\t\t\t\tstruct expand_data *data)\n {\n \tconst struct object_id *oid = &data->oid;\n \n@@ -389,6 +398,8 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\tchar *contents;\n \t\t\tunsigned long size;\n \n+\t\t\tflush_scratch(opt, scratch);\n+\n \t\t\tif (!data->rest)\n \t\t\t\tdie(\"missing path for '%s'\", oid_to_hex(oid));\n \n@@ -414,7 +425,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\tbatch_write(opt, contents, size);\n \t\t\tfree(contents);\n \t\t} else {\n-\t\t\tstream_blob(oid);\n+\t\t\tstream_blob(scratch, oid);\n \t\t}\n \t}\n \telse {\n@@ -422,6 +433,8 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\tunsigned long size;\n \t\tvoid *contents;\n \n+\t\tflush_scratch(opt, scratch);\n+\n \t\tcontents = repo_read_object_file(the_repository, oid, &type,\n \t\t\t\t\t\t &size);\n \t\tif (!contents)\n@@ -498,8 +511,6 @@ static void batch_object_write(const char *obj_name,\n \t\t}\n \t}\n \n-\tstrbuf_reset(scratch);\n-\n \tif (!opt->format) {\n \t\tprint_default_format(scratch, data, opt);\n \t} else {\n@@ -507,12 +518,11 @@ static void batch_object_write(const char *obj_name,\n \t\tstrbuf_addch(scratch, opt->output_delim);\n \t}\n \n-\tbatch_write(opt, scratch->buf, scratch->len);\n-\n \tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n-\t\tprint_object_or_die(opt, data);\n-\t\tbatch_write(opt, &opt->output_delim, 1);\n+\t\tprint_object_or_die(scratch, opt, data);\n+\t\tstrbuf_addch(scratch, opt->output_delim);\n \t}\n+\tflush_scratch(opt, scratch);\n }\n \n static void batch_one_object(const char *obj_name,\n@@ -787,7 +797,7 @@ static int batch_objects(struct batch_options *opt)\n \t\t      opt->format ? opt->format : DEFAULT_FORMAT,\n \t\t      &data);\n \tdata.mark_query = 0;\n-\tstrbuf_release(&output);\n+\tstrbuf_reset(&output);\n \tif (opt->transform_mode)\n \t\tdata.split_on_whitespace = 1;\n \ndiff --git a/streaming.c b/streaming.c\nindex 10adf625b2..9787449c50 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -10,6 +10,8 @@\n #include \"object-store-ll.h\"\n #include \"replace-object.h\"\n #include \"packfile.h\"\n+#include \"strbuf.h\"\n+#include \"hex.h\"\n \n typedef int (*open_istream_fn)(struct git_istream *,\n \t\t\t       struct repository *,\n@@ -546,3 +548,51 @@ int stream_blob_to_fd(int fd, const struct object_id *oid, struct stream_filter\n \tclose_istream(st);\n \treturn result;\n }\n+\n+/*\n+ * stdio buffering requires extra data copies, using strbuf\n+ * allows us to read_istream directly into a scratch buffer\n+ */\n+int stream_blob_to_strbuf_fd(int fd, struct strbuf *sb,\n+\t\t\t\tconst struct object_id *oid)\n+{\n+\tsize_t bufsz = 16 * 1024;\n+\tstruct git_istream *st;\n+\tenum object_type type;\n+\tunsigned long sz;\n+\tint result = -1;\n+\n+\tst = open_istream(the_repository, oid, &type, &sz, NULL);\n+\tif (!st)\n+\t\treturn result;\n+\tif (type != OBJ_BLOB)\n+\t\tgoto close_and_exit;\n+\tif (bufsz > sz)\n+\t\tbufsz = sz;\n+\tstrbuf_grow(sb, bufsz + 1); /* extra byte for output_delim */\n+\twhile (sz) {\n+\t\tssize_t readlen = read_istream(st, sb->buf + sb->len, bufsz);\n+\n+\t\tif (readlen < 0)\n+\t\t\tgoto close_and_exit;\n+\t\tif (readlen == 0)\n+\t\t\tdie(\"unexpected EOF from %s\\n\", oid_to_hex(oid));\n+\t\tsz -= readlen;\n+\t\tif (!sz) {\n+\t\t\t/*\n+\t\t\t * done, keep the last bit buffered for caller to\n+\t\t\t * append output_delim\n+\t\t\t */\n+\t\t\tstrbuf_setlen(sb, sb->len + readlen);\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (write_in_full(fd, sb->buf, sb->len + readlen) < 0)\n+\t\t\tgoto close_and_exit;\n+\t\tstrbuf_reset(sb);\n+\t}\n+\tresult = 0;\n+\n+ close_and_exit:\n+\tclose_istream(st);\n+\treturn result;\n+}\ndiff --git a/streaming.h b/streaming.h\nindex bd27f59e57..3cba4fe016 100644\n--- a/streaming.h\n+++ b/streaming.h\n@@ -17,5 +17,6 @@ int close_istream(struct git_istream *);\n ssize_t read_istream(struct git_istream *, void *, size_t);\n \n int stream_blob_to_fd(int fd, const struct object_id *, struct stream_filter *, int can_seek);\n+int stream_blob_to_strbuf_fd(int fd, struct strbuf *, const struct object_id *);\n \n #endif /* STREAMING_H */\n"},{"id":"497464","messageId":"20240621062915.GA2105230@coredump.intra.peff.net","threadId":"61661","inReplyTo":"20240621020457.1081233-1-e@80x24.org","subject":"Re: [PATCH] cat-file: reduce write calls for unfiltered blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-21T06:29:15Z","receivedAt":"2024-06-21T06:29:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 21, 2024 at 02:04:57AM +0000, Eric Wong wrote:\n\n> While the --buffer switch is useful for non-interactive batch use,\n> buffering doesn't work with processes using request-response loops since\n> idle times are unpredictable between requests.\n> \n> For unfiltered blobs, our streaming interface now appends the initial\n> blob data directly into the scratch buffer used for object info.\n> Furthermore, the final blob chunk can hold the output delimiter before\n> making the final write(2).\n\nSo we're basically saving one write() per object. I'm not that surprised\nyou didn't see a huge time improvement. I'd think most of the effort is\nspend zlib decompressing the object contents.\n\n> +\n> +/*\n> + * stdio buffering requires extra data copies, using strbuf\n> + * allows us to read_istream directly into a scratch buffer\n> + */\n> +int stream_blob_to_strbuf_fd(int fd, struct strbuf *sb,\n> +\t\t\t\tconst struct object_id *oid)\n> +{\n\nThis is a pretty convoluted interface. Did you measure that avoiding\nstdio actually provides a noticeable improvement?\n\nThis function seems to mostly duplicate stream_blob_to_fd(). If we do\nwant to go this route, it feels like we should be able to implement the\nexisting function in terms of this one, just by passing in an empty\nstrbuf?\n\nAll that said, I think there's another approach that will yield much\nbigger rewards. The call to _get_ the object-info line is separate from\nthe streaming code. So we end up finding and accessing each object\ntwice, which is wasteful, especially since most objects aren't big\nenough that streaming is useful.\n\nIf we could instead tell oid_object_info_extended() to just pass back\nthe content when it's not huge, we could output it directly. I have a\npatch that does this. You can fetch it from https://github.com/peff/git,\non the branch jk/object-info-round-trip. It drops the time to run\n\"cat-file --batch-all-objects --unordered --batch\" on git.git from ~7.1s\nto ~6.1s on my machine.\n\nI don't remember all the details of why I didn't polish up the patch. I\nthink there was some refactoring needed in packed_object_info(), and I\nnever got around to cleaning it up.\n\nBut anyway, that's a much bigger improvement than what you've got here.\nIt does still require two write() calls, since you'll get the object\ncontents as a separate buffer. But it might be possible to teach\nobject_oid_info_extended() to write into a buffer of your choice (so you\ncould reserve some space at the front to format the metadata into, and\nlikewise you could reuse the buffer to avoid malloc/free for each).\n\nI don't know that I'll have time to revisit it in the near future, but\nif you like the direction feel free to take a look at the patch and see\nif you can clean it up. (It was written years ago, but I rebase my\ntopics forward regularly and merge them into a daily driver, so it\nshould be in good working order).\n\n-Peff\n"},{"id":"497494","messageId":"3d43023c-ceb8-4e5c-9607-8448509fb599@gmail.com","threadId":"61661","inReplyTo":"20240621062915.GA2105230@coredump.intra.peff.net","subject":"Re: [PATCH] cat-file: reduce write calls for unfiltered blobs","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-21T13:24:05Z","receivedAt":"2024-06-21T13:24:07Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric and Peff\n\nOn 21/06/2024 07:29, Jeff King wrote:\n> On Fri, Jun 21, 2024 at 02:04:57AM +0000, Eric Wong wrote:\n> \n>> While the --buffer switch is useful for non-interactive batch use,\n>> buffering doesn't work with processes using request-response loops since\n>> idle times are unpredictable between requests.\n>>\n>> For unfiltered blobs, our streaming interface now appends the initial\n>> blob data directly into the scratch buffer used for object info.\n>> Furthermore, the final blob chunk can hold the output delimiter before\n>> making the final write(2).\n> \n> So we're basically saving one write() per object. I'm not that surprised\n> you didn't see a huge time improvement. I'd think most of the effort is\n> spend zlib decompressing the object contents.\n\nIf I'm reading the changes correctly then I think we may be saving more \nthan one write far large objects we now seem to allocate a buffer large \nenough to hold the whole object rather than using a fixed 16KB buffer. \nThe streaming read functions seem to try to fill the whole buffer before \nreturning so I think we'll try and write the whole object at once. I'm \nnot sure that approach is sensible for large blobs due to the extra \nmemory consumption and it does not seem to fit the behavior of the other \nstreaming functions.\n\nIf the reason for this change is to reduce the number of read() calls \nthe consumer has to make isn't that going to be limited by the capacity \nof the pipe? Does git to writing more than PIPE_BUF data at a time \nreally reduce the number of reads on the other side of the pipe?\n\n>> +\n>> +/*\n>> + * stdio buffering requires extra data copies, using strbuf\n>> + * allows us to read_istream directly into a scratch buffer\n>> + */\n>> +int stream_blob_to_strbuf_fd(int fd, struct strbuf *sb,\n>> +\t\t\t\tconst struct object_id *oid)\n>> +{\n> \n> This is a pretty convoluted interface. Did you measure that avoiding\n> stdio actually provides a noticeable improvement?\n\nYes this looks nasty especially as the gotcha of the caller being \nresponsible for writing any data left in the buffer when the function \nreturns is undocumented.\n\nYour suggestion below to avoid looking up the object twice sounds like a \nnicer and hopefully more effective way of trying to improve the \nperformance of \"git cat-file\".\n\nBest Wishes\n\nPhillip\n\n\n> This function seems to mostly duplicate stream_blob_to_fd(). If we do\n> want to go this route, it feels like we should be able to implement the\n> existing function in terms of this one, just by passing in an empty\n> strbuf?\n> \n> All that said, I think there's another approach that will yield much\n> bigger rewards. The call to _get_ the object-info line is separate from\n> the streaming code. So we end up finding and accessing each object\n> twice, which is wasteful, especially since most objects aren't big\n> enough that streaming is useful.\n> \n> If we could instead tell oid_object_info_extended() to just pass back\n> the content when it's not huge, we could output it directly. I have a\n> patch that does this. You can fetch it from https://github.com/peff/git,\n> on the branch jk/object-info-round-trip. It drops the time to run\n> \"cat-file --batch-all-objects --unordered --batch\" on git.git from ~7.1s\n> to ~6.1s on my machine.\n> \n> I don't remember all the details of why I didn't polish up the patch. I\n> think there was some refactoring needed in packed_object_info(), and I\n> never got around to cleaning it up.\n> \n> But anyway, that's a much bigger improvement than what you've got here.\n> It does still require two write() calls, since you'll get the object\n> contents as a separate buffer. But it might be possible to teach\n> object_oid_info_extended() to write into a buffer of your choice (so you\n> could reserve some space at the front to format the metadata into, and\n> likewise you could reuse the buffer to avoid malloc/free for each).\n> \n> I don't know that I'll have time to revisit it in the near future, but\n> if you like the direction feel free to take a look at the patch and see\n> if you can clean it up. (It was written years ago, but I rebase my\n> topics forward regularly and merge them into a daily driver, so it\n> should be in good working order).\n> \n> -Peff\n> \n\n"},{"id":"497496","messageId":"3dff8e61-1474-425a-8454-4d729b62ef83@gmail.com","threadId":"61661","inReplyTo":"3d43023c-ceb8-4e5c-9607-8448509fb599@gmail.com","subject":"Re: [PATCH] cat-file: reduce write calls for unfiltered blobs","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-21T15:25:32Z","receivedAt":"2024-06-21T15:25:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/06/2024 14:24, Phillip Wood wrote:\n> Hi Eric and Peff\n> \n> On 21/06/2024 07:29, Jeff King wrote:\n>> On Fri, Jun 21, 2024 at 02:04:57AM +0000, Eric Wong wrote:\n>>\n>>> While the --buffer switch is useful for non-interactive batch use,\n>>> buffering doesn't work with processes using request-response loops since\n>>> idle times are unpredictable between requests.\n>>>\n>>> For unfiltered blobs, our streaming interface now appends the initial\n>>> blob data directly into the scratch buffer used for object info.\n>>> Furthermore, the final blob chunk can hold the output delimiter before\n>>> making the final write(2).\n>>\n>> So we're basically saving one write() per object. I'm not that surprised\n>> you didn't see a huge time improvement. I'd think most of the effort is\n>> spend zlib decompressing the object contents.\n> \n> If I'm reading the changes correctly\n\nLooking at the patch again I had misread it - the buffer is the same \nsize and so the rest of this paragraph is nonsense.\n\nSorry for the noise\n\nPhillip\n\n> then I think we may be saving more \n> than one write far large objects we now seem to allocate a buffer large \n> enough to hold the whole object rather than using a fixed 16KB buffer. \n> The streaming read functions seem to try to fill the whole buffer before \n> returning so I think we'll try and write the whole object at once. I'm \n> not sure that approach is sensible for large blobs due to the extra \n> memory consumption and it does not seem to fit the behavior of the other \n> streaming functions.\n> \n> If the reason for this change is to reduce the number of read() calls \n> the consumer has to make isn't that going to be limited by the capacity \n> of the pipe? Does git to writing more than PIPE_BUF data at a time \n> really reduce the number of reads on the other side of the pipe?\n> \n>>> +\n>>> +/*\n>>> + * stdio buffering requires extra data copies, using strbuf\n>>> + * allows us to read_istream directly into a scratch buffer\n>>> + */\n>>> +int stream_blob_to_strbuf_fd(int fd, struct strbuf *sb,\n>>> +                const struct object_id *oid)\n>>> +{\n>>\n>> This is a pretty convoluted interface. Did you measure that avoiding\n>> stdio actually provides a noticeable improvement?\n> \n> Yes this looks nasty especially as the gotcha of the caller being \n> responsible for writing any data left in the buffer when the function \n> returns is undocumented.\n> \n> Your suggestion below to avoid looking up the object twice sounds like a \n> nicer and hopefully more effective way of trying to improve the \n> performance of \"git cat-file\".\n> \n> Best Wishes\n> \n> Phillip\n> \n> \n>> This function seems to mostly duplicate stream_blob_to_fd(). If we do\n>> want to go this route, it feels like we should be able to implement the\n>> existing function in terms of this one, just by passing in an empty\n>> strbuf?\n>>\n>> All that said, I think there's another approach that will yield much\n>> bigger rewards. The call to _get_ the object-info line is separate from\n>> the streaming code. So we end up finding and accessing each object\n>> twice, which is wasteful, especially since most objects aren't big\n>> enough that streaming is useful.\n>>\n>> If we could instead tell oid_object_info_extended() to just pass back\n>> the content when it's not huge, we could output it directly. I have a\n>> patch that does this. You can fetch it from https://github.com/peff/git,\n>> on the branch jk/object-info-round-trip. It drops the time to run\n>> \"cat-file --batch-all-objects --unordered --batch\" on git.git from ~7.1s\n>> to ~6.1s on my machine.\n>>\n>> I don't remember all the details of why I didn't polish up the patch. I\n>> think there was some refactoring needed in packed_object_info(), and I\n>> never got around to cleaning it up.\n>>\n>> But anyway, that's a much bigger improvement than what you've got here.\n>> It does still require two write() calls, since you'll get the object\n>> contents as a separate buffer. But it might be possible to teach\n>> object_oid_info_extended() to write into a buffer of your choice (so you\n>> could reserve some space at the front to format the metadata into, and\n>> likewise you could reuse the buffer to avoid malloc/free for each).\n>>\n>> I don't know that I'll have time to revisit it in the near future, but\n>> if you like the direction feel free to take a look at the patch and see\n>> if you can clean it up. (It was written years ago, but I rebase my\n>> topics forward regularly and merge them into a daily driver, so it\n>> should be in good working order).\n>>\n>> -Peff\n>>\n> \n> \n"},{"id":"497520","messageId":"20240621194221.M879537@dcvr","threadId":"61661","inReplyTo":"20240621062915.GA2105230@coredump.intra.peff.net","subject":"Re: [PATCH] cat-file: reduce write calls for unfiltered blobs","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-06-21T19:42:21Z","receivedAt":"2024-06-21T19:42:22Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King <peff@peff.net> wrote:\n> On Fri, Jun 21, 2024 at 02:04:57AM +0000, Eric Wong wrote:\n> \n> > While the --buffer switch is useful for non-interactive batch use,\n> > buffering doesn't work with processes using request-response loops since\n> > idle times are unpredictable between requests.\n> > \n> > For unfiltered blobs, our streaming interface now appends the initial\n> > blob data directly into the scratch buffer used for object info.\n> > Furthermore, the final blob chunk can hold the output delimiter before\n> > making the final write(2).\n> \n> So we're basically saving one write() per object. I'm not that surprised\n> you didn't see a huge time improvement. I'd think most of the effort is\n> spend zlib decompressing the object contents.\n\n3 writes down to 1 for small objects, actually: header + blob + delimiter\n\nI was mainly annoyed to strace my reader process and find 3 reads,\nor even more for non-blocking sockets, worst case (after initial\nwakeup via epoll_wait) is:\n\n  read, read (EAGAIN), poll, read, read (EAGAIN), poll, read\n\nBut yeah, scheduler behavior is unpredictable on complex modern\nsystems.\n\n> > +\n> > +/*\n> > + * stdio buffering requires extra data copies, using strbuf\n> > + * allows us to read_istream directly into a scratch buffer\n> > + */\n> > +int stream_blob_to_strbuf_fd(int fd, struct strbuf *sb,\n> > +\t\t\t\tconst struct object_id *oid)\n> > +{\n> \n> This is a pretty convoluted interface. Did you measure that avoiding\n> stdio actually provides a noticeable improvement?\n\nYeah, I didn't get any improvements with stdio I could measure;\nbut my measurements included AGPL Perl code on the reader side.\n\n> This function seems to mostly duplicate stream_blob_to_fd(). If we do\n> want to go this route, it feels like we should be able to implement the\n> existing function in terms of this one, just by passing in an empty\n> strbuf?\n\nI didn't want to stuff too much into the loop given the hole\nseeking optimization logic for regular files in\nstream_blob_to_fd.\n\n> All that said, I think there's another approach that will yield much\n> bigger rewards. The call to _get_ the object-info line is separate from\n> the streaming code. So we end up finding and accessing each object\n> twice, which is wasteful, especially since most objects aren't big\n> enough that streaming is useful.\n\nYeah, I noticed that and got confused, actually.\n\n> If we could instead tell oid_object_info_extended() to just pass back\n> the content when it's not huge, we could output it directly. I have a\n> patch that does this. You can fetch it from https://github.com/peff/git,\n> on the branch jk/object-info-round-trip. It drops the time to run\n> \"cat-file --batch-all-objects --unordered --batch\" on git.git from ~7.1s\n> to ~6.1s on my machine.\n\nCool, I'll look into it and probably combining the approaches.\nOptimizations often have a snowballing effect :)\n\n> But anyway, that's a much bigger improvement than what you've got here.\n> It does still require two write() calls, since you'll get the object\n> contents as a separate buffer. But it might be possible to teach\n> object_oid_info_extended() to write into a buffer of your choice (so you\n> could reserve some space at the front to format the metadata into, and\n> likewise you could reuse the buffer to avoid malloc/free for each).\n\nYeah, that sounds like a good idea.\n\n> I don't know that I'll have time to revisit it in the near future, but\n> if you like the direction feel free to take a look at the patch and see\n> if you can clean it up. (It was written years ago, but I rebase my\n> topics forward regularly and merge them into a daily driver, so it\n> should be in good working order).\n\nThanks.  I'll try to take a look at it soon.\n"},{"id":"497521","messageId":"xmqqiky2jd0n.fsf@gitster.g","threadId":"61661","inReplyTo":"20240621194221.M879537@dcvr","subject":"Re: [PATCH] cat-file: reduce write calls for unfiltered blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T19:45:28Z","receivedAt":"2024-06-21T19:45:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> Cool, I'll look into it and probably combining the approaches.\n> Optimizations often have a snowballing effect :)\n>\n>> But anyway, that's a much bigger improvement than what you've got here.\n>> It does still require two write() calls, since you'll get the object\n>> contents as a separate buffer. But it might be possible to teach\n>> object_oid_info_extended() to write into a buffer of your choice (so you\n>> could reserve some space at the front to format the metadata into, and\n>> likewise you could reuse the buffer to avoid malloc/free for each).\n>\n> Yeah, that sounds like a good idea.\n>\n>> I don't know that I'll have time to revisit it in the near future, but\n>> if you like the direction feel free to take a look at the patch and see\n>> if you can clean it up. (It was written years ago, but I rebase my\n>> topics forward regularly and merge them into a daily driver, so it\n>> should be in good working order).\n>\n> Thanks.  I'll try to take a look at it soon.\n\nThanks, that's an exciting direction to go in.\n\n"}]}