{"thread":{"id":"42372","subject":"[PATCH 0/2] minor cat-file optimizations","startedAt":"2016-05-18T16:52:53Z","lastAt":"2016-05-18T16:56:14Z","messageCount":4,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"286897","messageId":"20160518165253.GA5379@sigill.intra.peff.net","threadId":"42372","inReplyTo":null,"subject":"[PATCH 0/2] minor cat-file optimizations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T16:52:53Z","receivedAt":"2016-05-18T16:52:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"At Git Merge, Charles suggested that \"--buffer\" should be the default\nfor \"--batch-all-objects\". This bit me again yesterday, so I thought I'd\nactually implement it. :)\n\nThat's in the second patch. The first one is another micro-optimization\nthat happens to help the use case I was working on (the speedup there is\n\"only\" 40%, but the case I was looking at had a pathological number of\npacks, and so was even slower).\n\n  [1/2]: cat-file: avoid noop calls to sha1_object_info_extended\n  [2/2]: cat-file: default to --buffer when --batch-all-objects is used\n\n-Peff\n"},{"id":"286898","messageId":"20160518165411.GA5563@sigill.intra.peff.net","threadId":"42372","inReplyTo":"20160518165253.GA5379@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] minor cat-file optimizations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T16:54:11Z","receivedAt":"2016-05-18T16:54:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 18, 2016 at 12:52:53PM -0400, Jeff King wrote:\n\n> At Git Merge, Charles suggested that \"--buffer\" should be the default\n> for \"--batch-all-objects\". This bit me again yesterday, so I thought I'd\n> actually implement it. :)\n> \n> That's in the second patch. The first one is another micro-optimization\n> that happens to help the use case I was working on (the speedup there is\n> \"only\" 40%, but the case I was looking at had a pathological number of\n> packs, and so was even slower).\n> \n>   [1/2]: cat-file: avoid noop calls to sha1_object_info_extended\n>   [2/2]: cat-file: default to --buffer when --batch-all-objects is used\n\nWhoops, meant to actually cc Charles.\n\n-Peff\n"},{"id":"286899","messageId":"20160518165522.GA5607@sigill.intra.peff.net","threadId":"42372","inReplyTo":"20160518165411.GA5563@sigill.intra.peff.net","subject":"[PATCH 1/2] cat-file: avoid noop calls to sha1_object_info_extended","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T16:55:23Z","receivedAt":"2016-05-18T16:55:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It is not unreasonable to ask cat-file for a batch-check\nformat of simply \"%(objectname)\". At first glance this seems\nlike a noop (you are generally already feeding the object\nnames on stdin!), but it has a few uses:\n\n  1. With --batch-all-objects, you can generate a listing of\n     the sha1s present in the repository, without any input.\n\n  2. You do not have to feed sha1s; you can feed arbitrary\n     sha1 expressions and have git resolve them en masse.\n\n  3. You can even feed a raw sha1, with the result that git\n     will tell you whether we actually have the object or\n     not.\n\nIn case 3, the call to sha1_object_info is useful; it tells\nus whether the object exists or not (technically we could\nswap this out for has_sha1_file, but the cost is roughly the\nsame).\n\nIn case 2, the existence check is of debatable value. A\nmass-resolution might prefer performance to safety (against\noutputting a value for a corrupted ref, for example).\nHowever, the object lookup cost is likely not as noticeable\ncompared to the resolution cost. And since we have provided\nthat safety in the past, the conservative choice is to keep\nit.\n\nIn case 1, though, the object lookup is a definite noop; we\nknow about the object because we found it in the object\ndatabase. There is no new information gained by making the\ncall.\n\nThis patch detects that case and optimizes out the call.\nHere are best-of-five timings for linux.git:\n\n  [before]\n  $ time git cat-file --buffer \\\n                      --batch-all-objects \\\n                      --batch-check='%(objectname)'\n  real    0m2.117s\n  user    0m2.044s\n  sys     0m0.072s\n\n  [after]\n  $ time git cat-file --buffer \\\n                      --batch-all-objects \\\n                      --batch-check='%(objectname)'\n  real    0m1.230s\n  user    0m1.176s\n  sys     0m0.052s\n\nThere are two implementation details to note here.\n\nOne is that we detect the noop case by seeing that \"struct\nobject_info\" does not request any information. But besides\nobject existence, there is one other piece of information\nwhich sha1_object_info may fill in: whether the object is\ncached, loose, or packed. We don't currently provide that\ninformation in the output, but if we were to do so later,\nwe'd need to take note and disable the optimization in that\ncase.\n\nAnd that leads to the second note. If we were to output\nthat information, a better implementation would be to\nremember where we saw the object in --batch-all-objects in\nthe first place, and avoid looking it up again by sha1.\n\nIn fact, we could probably squeeze out some extra\nperformance for less-trivial cases, too, by remembering the\npack location where we saw the object, and going directly\nthere to find its information (like type, size, etc). That\nwould in theory make this optimization unnecessary.\n\nI didn't pursue that path here for two reasons:\n\n  1. It's non-trivial to implement, and has memory\n     implications. Because we sort and de-dup the list of\n     output sha1s, we'd have to record the pack information\n     for each object, too.\n\n  2. It doesn't save as much as you might hope. It saves the\n     find_pack_entry() call, but getting the size and type\n     for deltified objects requires walking down the delta\n     chain (for the real type) or reading the delta data\n     header (for the size). These costs tend to dominate the\n     non-trivial cases.\n\nBy contrast, this optimization is easy and self-contained,\nand speeds up a real-world case I've used.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/cat-file.c | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 54db118..5d3180e 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -154,6 +154,13 @@ struct expand_data {\n \t * elements above, so you can retrieve the response from there.\n \t */\n \tstruct object_info info;\n+\n+\t/*\n+\t * This flag will be true if the requested batch format and options\n+\t * don't require us to call sha1_object_info, which can then be\n+\t * optimized out.\n+\t */\n+\tunsigned skip_object_info : 1;\n };\n \n static int is_atom(const char *atom, const char *s, int slen)\n@@ -258,7 +265,8 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (sha1_object_info_extended(data->sha1, &data->info, LOOKUP_REPLACE_OBJECT) < 0) {\n+\tif (!data->skip_object_info &&\n+\t    sha1_object_info_extended(data->sha1, &data->info, LOOKUP_REPLACE_OBJECT) < 0) {\n \t\tprintf(\"%s missing\\n\", obj_name ? obj_name : sha1_to_hex(data->sha1));\n \t\tfflush(stdout);\n \t\treturn;\n@@ -369,6 +377,13 @@ static int batch_objects(struct batch_options *opt)\n \tstrbuf_expand(&buf, opt->format, expand_format, &data);\n \tdata.mark_query = 0;\n \n+\tif (opt->all_objects) {\n+\t\tstruct object_info empty;\n+\t\tmemset(&empty, 0, sizeof(empty));\n+\t\tif (!memcmp(&data.info, &empty, sizeof(empty)))\n+\t\t\tdata.skip_object_info = 1;\n+\t}\n+\n \t/*\n \t * If we are printing out the object, then always fill in the type,\n \t * since we will want to decide whether or not to stream.\n-- \n2.8.2.888.gecb1fe3\n"},{"id":"286900","messageId":"20160518165613.GB5607@sigill.intra.peff.net","threadId":"42372","inReplyTo":"20160518165411.GA5563@sigill.intra.peff.net","subject":"[PATCH 2/2] cat-file: default to --buffer when --batch-all-objects is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T16:56:14Z","receivedAt":"2016-05-18T16:56:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Traditionally cat-file's batch-mode does not do any output\nbuffering. The reason is that a caller may have pipes\nconnected to its input and output, and would want to use\ncat-file interactively, getting output immediately for each\ninput it sends.\n\nThis may involve a lot of small write() calls, which can be\nslow. So we introduced --buffer to improve this, but we\ncan't turn it on by default, as it would break the\ninteractive case above.\n\nHowever, when --batch-all-objects is used, we do not read\nstdin at all. We generate the output ourselves as quickly as\npossible, and then exit. In this case buffering is a strict\nwin, and it is simply a hassle for the user to have to\nremember to specify --buffer.\n\nThis patch makes --buffer the default when --batch-all-objects\nis used. Specifying \"--buffer\" manually is still OK, and you\ncan even override it with \"--no-buffer\" if you're a\nmasochist (or debugging).\n\nFor some real numbers, running:\n\n  git cat-file --batch-all-objects --batch-check='%(objectname)'\n\non torvalds/linux goes from:\n\n  real    0m1.464s\n  user    0m1.208s\n  sys     0m0.252s\n\nto:\n\n  real    0m1.230s\n  user    0m1.172s\n  sys     0m0.056s\n\nfor a 16% speedup.\n\nSuggested-by: Charles Bailey <charles@hashpling.org>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/cat-file.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 5d3180e..618103f 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -504,6 +504,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_cat_file_config, NULL);\n \n+\tbatch.buffer_output = -1;\n \targc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);\n \n \tif (opt) {\n@@ -527,6 +528,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(cat_file_usage, options);\n \t}\n \n+\tif (batch.buffer_output < 0)\n+\t\tbatch.buffer_output = batch.all_objects;\n+\n \tif (batch.enabled)\n \t\treturn batch_objects(&batch);\n \n-- \n2.8.2.888.gecb1fe3\n"}]}