{"thread":{"id":"34413","subject":"[PATCH 0/7] cat-file --batch-check performance improvements","startedAt":"2013-07-12T06:15:34Z","lastAt":"2013-07-15T04:23:48Z","messageCount":16,"participants":["Jeff King","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"223131","messageId":"20130712061533.GA11297@sigill.intra.peff.net","threadId":"34413","inReplyTo":null,"subject":"[PATCH 0/7] cat-file --batch-check performance improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T06:15:34Z","receivedAt":"2013-07-12T06:15:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In my earlier series introducing \"git cat-file --batch-check=<format>\",\nfound here:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/229761/focus=230041\n\nI spent a little time optimizing revindex generation, and measured by\nrequesting information on a single object from a large repository. This\nseries takes the next logical step: requesting a large number of objects\nfrom a large repository.\n\nThere are two major optimizations here:\n\n  1. Avoiding extra ref lookups due to the warning in 798c35f (get_sha1:\n     warn about full or short object names that look like refs,\n     2013-05-29).\n\n  2. Avoiding extra work for delta type resolution when the user has not\n     asked for %(objecttype).\n\nI prepared the series on top of jk/in-pack-size-measurement, and\ncertainly optimization 2 is pointless without it (before that topic,\n--batch-check always printed the type).\n\nHowever, the first optimization affects regular --batch-check, and\nrepresents a much more serious performance regression. It looks like\n798c35f is in master, but hasn't been released yet, so assuming these\ntopics graduate before the next release, it should be OK. But if not, we\nshould consider pulling the first patch out and applying it (or\nsomething like it) separately.\n\nThe results for running (in linux.git):\n\n  $ git rev-list --objects --all >objects\n  $ git cat-file --batch-check='%(objectsize:disk)' <objects >/dev/null\n\nare:\n\n        before     after\n  real  1m17.143s  0m7.205s\n  user  0m27.684s  0m6.580s\n  sys   0m49.320s  0m0.608s\n\nNow, _most_ of that speedup is coming from the first patch, and it's\nquite trivial. The rest of the patches involve a lot of refactoring, and\nonly manage to eke out one more second of performance, so it may not be\nworth it (though I think the result actually cleans up the\nsha1_object_info_extended interface a bit, and is worth it). Individual\ntimings are in the commit messages.\n\nThe patches are:\n\n  [1/7]: cat-file: disable object/refname ambiguity check for batch mode\n\nOptimization 1.\n\n  [2/7]: sha1_object_info_extended: rename \"status\" to \"type\"\n  [3/7]: sha1_loose_object_info: make type lookup optional\n  [4/7]: packed_object_info: hoist delta type resolution to helper\n  [5/7]: packed_object_info: make type lookup optional\n  [6/7]: sha1_object_info_extended: make type calculation optional\n\nOptimization 2.\n\n  [7/7]: sha1_object_info_extended: pass object_info to helpers\n\nOptional cleanup.\n\n-Peff\n"},{"id":"223137","messageId":"20130712062004.GA15572@sigill.intra.peff.net","threadId":"34413","inReplyTo":"20130712061533.GA11297@sigill.intra.peff.net","subject":"[PATCH 1/7] cat-file: disable object/refname ambiguity check for batch mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T06:20:05Z","receivedAt":"2013-07-12T06:20:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A common use of \"cat-file --batch-check\" is to feed a list\nof objects from \"rev-list --objects\" or a similar command.\nIn this instance, all of our input objects are 40-byte sha1\nids. However, cat-file has always allowed arbitrary revision\nspecifiers, and feeds the result to get_sha1().\n\nFortunately, get_sha1() recognizes a 40-byte sha1 before\ndoing any hard work trying to look up refs, meaning this\nscenario should end up spending very little time converting\nthe input into an object sha1. However, since 798c35f\n(get_sha1: warn about full or short object names that look\nlike refs, 2013-05-29), when we encounter this case, we\nspend the extra effort to do a refname lookup anyway, just\nto print a warning. This is further exacerbated by ca91993\n(get_packed_ref_cache: reload packed-refs file when it\nchanges, 2013-06-20), which makes individual ref lookup more\nexpensive by requiring a stat() of the packed-refs file for\neach missing ref.\n\nWith no patches, this is the time it takes to run:\n\n  $ git rev-list --objects --all >objects\n  $ time git cat-file --batch-check='%(objectname)' <objects\n\non the linux.git repository:\n\n  real    1m13.494s\n  user    0m25.924s\n  sys     0m47.532s\n\nIf we revert ca91993, the packed-refs up-to-date check, it\ngets a little better:\n\n  real    0m54.697s\n  user    0m21.692s\n  sys     0m32.916s\n\nbut we are still spending quite a bit of time on ref lookup\n(and we would not want to revert that patch, anyway, which\nhas correctness issues).  If we revert 798c35f, disabling\nthe warning entirely, we get a much more reasonable time:\n\n  real    0m7.452s\n  user    0m6.836s\n  sys     0m0.608s\n\nThis patch does the moral equivalent of this final case (and\ngets similar speedups). We introduce a global flag that\ncallers of get_sha1() can use to avoid paying the price for\nthe warning.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe solution feels a little hacky, but I'm not sure there is a better\none short of reverting the warning entirely.\n\nWe could also tie it to warn_ambiguous_refs (or add another config\nvariable), but I don't think that makes sense. It is not about \"do I\ncare about ambiguities in this repository\", but rather \"I am going to\ndo a really large number of sha1 resolutions, and I do not want to pay\nthe price in this particular code path for the warning\").\n\nThere may be other sites that resolve a large number of refs and run\ninto this, but I couldn't think of any. Doing for_each_ref would not\nhave the same problem, as we already know they are refs there.\n\n builtin/cat-file.c |  9 +++++++++\n cache.h            |  1 +\n environment.c      |  1 +\n sha1_name.c        | 14 ++++++++------\n 4 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 0e64b41..fe5c77f 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -266,6 +266,15 @@ static int batch_objects(struct batch_options *opt)\n \tstrbuf_expand(&buf, opt->format, expand_format, &data);\n \tdata.mark_query = 0;\n \n+\t/*\n+\t * We are going to call get_sha1 on a potentially very large number of\n+\t * objects. In most large cases, these will be actual object sha1s. The\n+\t * cost to double-check that each one is not also a ref (just so we can\n+\t * warn) ends up dwarfing the actual cost of the object lookups\n+\t * themselves. We can work around it by just turning off the warning.\n+\t */\n+\twarn_on_object_refname_ambiguity = 0;\n+\n \twhile (strbuf_getline(&buf, stdin, '\\n') != EOF) {\n \t\tchar *p;\n \t\tint error;\ndiff --git a/cache.h b/cache.h\nindex 2d06169..c1fd82c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -575,6 +575,7 @@ extern int warn_ambiguous_refs;\n extern int prefer_symlink_refs;\n extern int log_all_ref_updates;\n extern int warn_ambiguous_refs;\n+extern int warn_on_object_refname_ambiguity;\n extern int shared_repository;\n extern const char *apply_default_whitespace;\n extern const char *apply_default_ignorewhitespace;\ndiff --git a/environment.c b/environment.c\nindex 0cb67b2..5398c36 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -22,6 +22,7 @@ int warn_ambiguous_refs = 1;\n int is_bare_repository_cfg = -1; /* unspecified */\n int log_all_ref_updates = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n+int warn_on_object_refname_ambiguity = 1;\n int repository_format_version;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 90419ef..6f8812a 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -452,13 +452,15 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \tint at, reflog_len, nth_prior = 0;\n \n \tif (len == 40 && !get_sha1_hex(str, sha1)) {\n-\t\trefs_found = dwim_ref(str, len, tmp_sha1, &real_ref);\n-\t\tif (refs_found > 0 && warn_ambiguous_refs) {\n-\t\t\twarning(warn_msg, len, str);\n-\t\t\tif (advice_object_name_warning)\n-\t\t\t\tfprintf(stderr, \"%s\\n\", _(object_name_msg));\n+\t\tif (warn_on_object_refname_ambiguity) {\n+\t\t\trefs_found = dwim_ref(str, len, tmp_sha1, &real_ref);\n+\t\t\tif (refs_found > 0 && warn_ambiguous_refs) {\n+\t\t\t\twarning(warn_msg, len, str);\n+\t\t\t\tif (advice_object_name_warning)\n+\t\t\t\t\tfprintf(stderr, \"%s\\n\", _(object_name_msg));\n+\t\t\t}\n+\t\t\tfree(real_ref);\n \t\t}\n-\t\tfree(real_ref);\n \t\treturn 0;\n \t}\n \n-- \n1.8.3.rc3.24.gec82cb9\n"},{"id":"223138","messageId":"20130712062122.GB15572@sigill.intra.peff.net","threadId":"34413","inReplyTo":"20130712061533.GA11297@sigill.intra.peff.net","subject":"[PATCH 2/7] sha1_object_info_extended: rename \"status\" to \"type\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T06:21:22Z","receivedAt":"2013-07-12T06:21:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The value we get from each low-level object_info function\n(e.g., loose, packed) is actually the object type (or -1 for\nerror). Let's explicitly call it \"type\", which will make\nfurther refactorings easier to read.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4c2365f..e826066 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2394,7 +2394,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n {\n \tstruct cached_object *co;\n \tstruct pack_entry e;\n-\tint status, rtype;\n+\tint type, rtype;\n \n \tco = find_cached_object(sha1);\n \tif (co) {\n@@ -2408,23 +2408,23 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \n \tif (!find_pack_entry(sha1, &e)) {\n \t\t/* Most likely it's a loose object. */\n-\t\tstatus = sha1_loose_object_info(sha1, oi->sizep, oi->disk_sizep);\n-\t\tif (status >= 0) {\n+\t\ttype = sha1_loose_object_info(sha1, oi->sizep, oi->disk_sizep);\n+\t\tif (type >= 0) {\n \t\t\toi->whence = OI_LOOSE;\n-\t\t\treturn status;\n+\t\t\treturn type;\n \t\t}\n \n \t\t/* Not a loose object; someone else may have just packed it. */\n \t\treprepare_packed_git();\n \t\tif (!find_pack_entry(sha1, &e))\n-\t\t\treturn status;\n+\t\t\treturn type;\n \t}\n \n-\tstatus = packed_object_info(e.p, e.offset, oi->sizep, &rtype,\n-\t\t\t\t    oi->disk_sizep);\n-\tif (status < 0) {\n+\ttype = packed_object_info(e.p, e.offset, oi->sizep, &rtype,\n+\t\t\t\t  oi->disk_sizep);\n+\tif (type < 0) {\n \t\tmark_bad_packed_object(e.p, sha1);\n-\t\tstatus = sha1_object_info_extended(sha1, oi);\n+\t\ttype = sha1_object_info_extended(sha1, oi);\n \t} else if (in_delta_base_cache(e.p, e.offset)) {\n \t\toi->whence = OI_DBCACHED;\n \t} else {\n@@ -2435,7 +2435,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \t\t\t\t\t rtype == OBJ_OFS_DELTA);\n \t}\n \n-\treturn status;\n+\treturn type;\n }\n \n int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n-- \n1.8.3.rc3.24.gec82cb9\n"},{"id":"223141","messageId":"20130712063048.GC15572@sigill.intra.peff.net","threadId":"34413","inReplyTo":"20130712061533.GA11297@sigill.intra.peff.net","subject":"[PATCH 3/7] sha1_loose_object_info: make type lookup optional","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T06:30:48Z","receivedAt":"2013-07-12T06:30:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Until recently, the only items to request from\nsha1_object_info_extended were type and size. This meant\nthat we always had to open a loose object file to determine\none or the other.  But with the addition of the disk_size\nquery, it's possible that we can fulfill the query without\neven opening the object file at all. However, since the\nfunction interface always returns the type, we have no way\nof knowing whether the caller cares about it or not.\n\nThis patch only modified sha1_loose_object_info to make type\nlookup optional using an out-parameter, similar to the way\nthe size is handled (and the return value is \"0\" or \"-1\" for\nsuccess or error, respectively).\n\nThere should be no functional change yet, though, as\nsha1_object_info_extended, the only caller, will always ask\nfor a type.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously the end goal is to have sha1_object_info_extended do this\noptionally, too (which happens in patch 6).\n\nI'm not too happy about the stat_sha1_file function, which is almost\nidentical to open_sha1_file (and which in turn is almost the same thing\nas has_loose_object). They all do:\n\n  try operation X on sha1_file_name(sha1)\n  prepare_alt_odb();\n  foreach alt_odb\n    try operation X on alt_odb/sha1_file_name(sha1)\n\nUnfortunately it's hard to do this kind of factoring out in C, because\nthe argument and return types for operation X are different in these\ncases; you are stuck with providing callback function that takes a void\npointer to some operation-specific data. The boilerplate ends up worse\nthan the repeated code.\n\nAnother solution would be to have a \"find the file for loose object Y\"\nfunction, and then just do operation X on that. But since X is a\nfilesystem operation in each case, you do not want to lose the atomicity\nof performing the operation directly (not to mention incurring the cost\nof an extra stat() on each file).\n\nSo I am open to clever refactoring suggestions.\n\n sha1_file.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 43 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex e826066..39e7313 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1306,6 +1306,26 @@ static int git_open_noatime(const char *name)\n \t}\n }\n \n+static int stat_sha1_file(const unsigned char *sha1, struct stat *st)\n+{\n+\tchar *name = sha1_file_name(sha1);\n+\tstruct alternate_object_database *alt;\n+\n+\tif (!lstat(name, st))\n+\t\treturn 0;\n+\n+\tprepare_alt_odb();\n+\terrno = ENOENT;\n+\tfor (alt = alt_odb_list; alt; alt = alt->next) {\n+\t\tname = alt->name;\n+\t\tfill_sha1_path(name, sha1);\n+\t\tif (!lstat(alt->base, st))\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn -1;\n+}\n+\n static int open_sha1_file(const unsigned char *sha1)\n {\n \tint fd;\n@@ -2363,7 +2383,9 @@ struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \n }\n \n-static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *sizep,\n+static int sha1_loose_object_info(const unsigned char *sha1,\n+\t\t\t\t  enum object_type *typep,\n+\t\t\t\t  unsigned long *sizep,\n \t\t\t\t  unsigned long *disk_sizep)\n {\n \tint status;\n@@ -2372,6 +2394,20 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size\n \tgit_zstream stream;\n \tchar hdr[32];\n \n+\t/*\n+\t * If we don't care about type or size, then we don't\n+\t * need to look inside the object at all.\n+\t */\n+\tif (!typep && !sizep) {\n+\t\tif (disk_sizep) {\n+\t\t\tstruct stat st;\n+\t\t\tif (stat_sha1_file(sha1, &st) < 0)\n+\t\t\t\treturn -1;\n+\t\t\t*disk_sizep = st.st_size;\n+\t\t}\n+\t\treturn 0;\n+\t}\n+\n \tmap = map_sha1_file(sha1, &mapsize);\n \tif (!map)\n \t\treturn -1;\n@@ -2386,7 +2422,9 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size\n \t\t*sizep = size;\n \tgit_inflate_end(&stream);\n \tmunmap(map, mapsize);\n-\treturn status;\n+\tif (typep)\n+\t\t*typep = status;\n+\treturn 0;\n }\n \n /* returns enum object_type or negative */\n@@ -2408,8 +2446,8 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \n \tif (!find_pack_entry(sha1, &e)) {\n \t\t/* Most likely it's a loose object. */\n-\t\ttype = sha1_loose_object_info(sha1, oi->sizep, oi->disk_sizep);\n-\t\tif (type >= 0) {\n+\t\tif (!sha1_loose_object_info(sha1, &type,\n+\t\t\t\t\t    oi->sizep, oi->disk_sizep)) {\n \t\t\toi->whence = OI_LOOSE;\n \t\t\treturn type;\n \t\t}\n@@ -2417,7 +2455,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \t\t/* Not a loose object; someone else may have just packed it. */\n \t\treprepare_packed_git();\n \t\tif (!find_pack_entry(sha1, &e))\n-\t\t\treturn type;\n+\t\t\treturn -1;\n \t}\n \n \ttype = packed_object_info(e.p, e.offset, oi->sizep, &rtype,\n-- \n1.8.3.rc3.24.gec82cb9\n"},{"id":"223142","messageId":"20130712063157.GD15572@sigill.intra.peff.net","threadId":"34413","inReplyTo":"20130712061533.GA11297@sigill.intra.peff.net","subject":"[PATCH 4/7] packed_object_info: hoist delta type resolution to helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T06:31:57Z","receivedAt":"2013-07-12T06:31:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To calculate the type of a packed object, we must walk down\nits delta chain until we hit a true base object with a real\ntype. Most of the code in packed_object_info is for handling\nthis case.\n\nLet's hoist it out into a separate helper function, which\nwill make it easier to make the type-lookup optional in the\nfuture (and keep our indentation level sane).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAnnoyingly, since the part we are moving comprises the bulk\nof the function, the diff tends to show the opposite of what\nactually happened: it looks like we renamed the function to its helper\nname, and moved the other parts to a new function, which happens to have\nthe same name as the old one. So read the diff with care. :)\n\n sha1_file.c | 93 +++++++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 53 insertions(+), 40 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 39e7313..6f4aff3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1713,52 +1713,21 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,\n \treturn type;\n }\n \n-\n #define POI_STACK_PREALLOC 64\n \n-static int packed_object_info(struct packed_git *p, off_t obj_offset,\n-\t\t\t      unsigned long *sizep, int *rtype,\n-\t\t\t      unsigned long *disk_sizep)\n+static enum object_type packed_to_object_type(struct packed_git *p,\n+\t\t\t\t\t      off_t obj_offset,\n+\t\t\t\t\t      enum object_type type,\n+\t\t\t\t\t      struct pack_window **w_curs,\n+\t\t\t\t\t      off_t curpos)\n {\n-\tstruct pack_window *w_curs = NULL;\n-\tunsigned long size;\n-\toff_t curpos = obj_offset;\n-\tenum object_type type;\n \toff_t small_poi_stack[POI_STACK_PREALLOC];\n \toff_t *poi_stack = small_poi_stack;\n \tint poi_stack_nr = 0, poi_stack_alloc = POI_STACK_PREALLOC;\n \n-\ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n-\n-\tif (rtype)\n-\t\t*rtype = type; /* representation type */\n-\n-\tif (sizep) {\n-\t\tif (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n-\t\t\toff_t tmp_pos = curpos;\n-\t\t\toff_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,\n-\t\t\t\t\t\t\t   type, obj_offset);\n-\t\t\tif (!base_offset) {\n-\t\t\t\ttype = OBJ_BAD;\n-\t\t\t\tgoto out;\n-\t\t\t}\n-\t\t\t*sizep = get_size_from_delta(p, &w_curs, tmp_pos);\n-\t\t\tif (*sizep == 0) {\n-\t\t\t\ttype = OBJ_BAD;\n-\t\t\t\tgoto out;\n-\t\t\t}\n-\t\t} else {\n-\t\t\t*sizep = size;\n-\t\t}\n-\t}\n-\n-\tif (disk_sizep) {\n-\t\tstruct revindex_entry *revidx = find_pack_revindex(p, obj_offset);\n-\t\t*disk_sizep = revidx[1].offset - obj_offset;\n-\t}\n-\n \twhile (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n \t\toff_t base_offset;\n+\t\tunsigned long size;\n \t\t/* Push the object we're going to leave behind */\n \t\tif (poi_stack_nr >= poi_stack_alloc && poi_stack == small_poi_stack) {\n \t\t\tpoi_stack_alloc = alloc_nr(poi_stack_nr);\n@@ -1769,11 +1738,11 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,\n \t\t}\n \t\tpoi_stack[poi_stack_nr++] = obj_offset;\n \t\t/* If parsing the base offset fails, just unwind */\n-\t\tbase_offset = get_delta_base(p, &w_curs, &curpos, type, obj_offset);\n+\t\tbase_offset = get_delta_base(p, w_curs, &curpos, type, obj_offset);\n \t\tif (!base_offset)\n \t\t\tgoto unwind;\n \t\tcurpos = obj_offset = base_offset;\n-\t\ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n+\t\ttype = unpack_object_header(p, w_curs, &curpos, &size);\n \t\tif (type <= OBJ_NONE) {\n \t\t\t/* If getting the base itself fails, we first\n \t\t\t * retry the base, otherwise unwind */\n@@ -1800,7 +1769,6 @@ out:\n out:\n \tif (poi_stack != small_poi_stack)\n \t\tfree(poi_stack);\n-\tunuse_pack(&w_curs);\n \treturn type;\n \n unwind:\n@@ -1814,6 +1782,51 @@ unwind:\n \tgoto out;\n }\n \n+static int packed_object_info(struct packed_git *p, off_t obj_offset,\n+\t\t\t      unsigned long *sizep, int *rtype,\n+\t\t\t      unsigned long *disk_sizep)\n+{\n+\tstruct pack_window *w_curs = NULL;\n+\tunsigned long size;\n+\toff_t curpos = obj_offset;\n+\tenum object_type type;\n+\n+\ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n+\n+\tif (rtype)\n+\t\t*rtype = type; /* representation type */\n+\n+\tif (sizep) {\n+\t\tif (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n+\t\t\toff_t tmp_pos = curpos;\n+\t\t\toff_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,\n+\t\t\t\t\t\t\t   type, obj_offset);\n+\t\t\tif (!base_offset) {\n+\t\t\t\ttype = OBJ_BAD;\n+\t\t\t\tgoto out;\n+\t\t\t}\n+\t\t\t*sizep = get_size_from_delta(p, &w_curs, tmp_pos);\n+\t\t\tif (*sizep == 0) {\n+\t\t\t\ttype = OBJ_BAD;\n+\t\t\t\tgoto out;\n+\t\t\t}\n+\t\t} else {\n+\t\t\t*sizep = size;\n+\t\t}\n+\t}\n+\n+\tif (disk_sizep) {\n+\t\tstruct revindex_entry *revidx = find_pack_revindex(p, obj_offset);\n+\t\t*disk_sizep = revidx[1].offset - obj_offset;\n+\t}\n+\n+\ttype = packed_to_object_type(p, obj_offset, type, &w_curs, curpos);\n+\n+out:\n+\tunuse_pack(&w_curs);\n+\treturn type;\n+}\n+\n static void *unpack_compressed_entry(struct packed_git *p,\n \t\t\t\t    struct pack_window **w_curs,\n \t\t\t\t    off_t curpos,\n-- \n1.8.3.rc3.24.gec82cb9\n"},{"id":"223143","messageId":"20130712063225.GE15572@sigill.intra.peff.net","threadId":"34413","inReplyTo":"20130712061533.GA11297@sigill.intra.peff.net","subject":"[PATCH 5/7] packed_object_info: make type lookup optional","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T06:32:25Z","receivedAt":"2013-07-12T06:32:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Currently, packed_object_info can save some work by not\ncalculating the size or disk_size of the object if the\ncaller is not interested. However, it always calculates the\ntrue object type, whether the caller cares or not, and only\noptionally returns the easy-to-get \"representation type\".\n\nLet's swap these types. The function will now return the\nrepresentation type (or OBJ_BAD on failure), and will only\noptionally fill in the true type.\n\nThere should be no behavior change yet, as the only caller,\nsha1_object_info_extended, will always feed it a type\npointer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6f4aff3..2a1e230 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1783,7 +1783,7 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,\n }\n \n static int packed_object_info(struct packed_git *p, off_t obj_offset,\n-\t\t\t      unsigned long *sizep, int *rtype,\n+\t\t\t      enum object_type *typep, unsigned long *sizep,\n \t\t\t      unsigned long *disk_sizep)\n {\n \tstruct pack_window *w_curs = NULL;\n@@ -1791,11 +1791,12 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,\n \toff_t curpos = obj_offset;\n \tenum object_type type;\n \n+\t/*\n+\t * We always get the representation type, but only convert it to\n+\t * a \"real\" type later if the caller is interested.\n+\t */\n \ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n \n-\tif (rtype)\n-\t\t*rtype = type; /* representation type */\n-\n \tif (sizep) {\n \t\tif (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n \t\t\toff_t tmp_pos = curpos;\n@@ -1820,7 +1821,13 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,\n \t\t*disk_sizep = revidx[1].offset - obj_offset;\n \t}\n \n-\ttype = packed_to_object_type(p, obj_offset, type, &w_curs, curpos);\n+\tif (typep) {\n+\t\t*typep = packed_to_object_type(p, obj_offset, type, &w_curs, curpos);\n+\t\tif (*typep < 0) {\n+\t\t\ttype = OBJ_BAD;\n+\t\t\tgoto out;\n+\t\t}\n+\t}\n \n out:\n \tunuse_pack(&w_curs);\n@@ -2471,11 +2478,11 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \t\t\treturn -1;\n \t}\n \n-\ttype = packed_object_info(e.p, e.offset, oi->sizep, &rtype,\n-\t\t\t\t  oi->disk_sizep);\n-\tif (type < 0) {\n+\trtype = packed_object_info(e.p, e.offset, &type, oi->sizep,\n+\t\t\t\t   oi->disk_sizep);\n+\tif (rtype < 0) {\n \t\tmark_bad_packed_object(e.p, sha1);\n-\t\ttype = sha1_object_info_extended(sha1, oi);\n+\t\treturn sha1_object_info_extended(sha1, oi);\n \t} else if (in_delta_base_cache(e.p, e.offset)) {\n \t\toi->whence = OI_DBCACHED;\n \t} else {\n-- \n1.8.3.rc3.24.gec82cb9\n"},{"id":"223145","messageId":"20130712063457.GF15572@sigill.intra.peff.net","threadId":"34413","inReplyTo":"20130712061533.GA11297@sigill.intra.peff.net","subject":"[PATCH 6/7] sha1_object_info_extended: make type calculation optional","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T06:34:57Z","receivedAt":"2013-07-12T06:34:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Each caller of sha1_object_info_extended sets up an\nobject_info struct to tell the function which elements of\nthe object it wants to get. Until now, getting the type of\nthe object has always been required (and it is returned via\nthe return type rather than a pointer in object_info).\n\nThis can involve actually opening a loose object file to\ndetermine its type, or following delta chains to determine a\npacked file's base type. These effects produce a measurable\nslow-down when doing a \"cat-file --batch-check\" that does\nnot include %(objecttype).\n\nThis patch adds a \"typep\" query to struct object_info, so\nthat it can be optionally queried just like size and\ndisk_size. As a result, the return type of the function is\nno longer the object type, but rather 0/-1 for success/error.\n\nAs there are only three callers total, we just fix up each\ncaller rather than keep a compatibility wrapper:\n\n  1. The simpler sha1_object_info wrapper continues to\n     always ask for and return the type field.\n\n  2. The istream_source function wants to know the type, and\n     so always asks for it.\n\n  3. The cat-file batch code asks for the type only when\n     %(objecttype) is part of the format string.\n\nOn linux.git, the best-of-five for running:\n\n  $ git rev-list --objects --all >objects\n  $ time git cat-file --batch-check='%(objectsize:disk)'\n\non a fully packed repository goes from:\n\n  real    0m8.680s\n  user    0m8.160s\n  sys     0m0.512s\n\nto:\n\n  real    0m7.205s\n  user    0m6.580s\n  sys     0m0.608s\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis ends up changing the behavior of sha1_object_info_extended without\nchanging its function signature. Given that it is a fairly inactive area\nof the code and that there are no topics in flight, I think this is OK.\nBut an alternative could be to add (yet another) wrapper to leave the\nfirst two call-sites untouched.\n\n builtin/cat-file.c |  7 ++++---\n cache.h            |  1 +\n sha1_file.c        | 20 +++++++++++++-------\n streaming.c        |  2 +-\n 4 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex fe5c77f..163ce6c 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -150,7 +150,9 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,\n \t\tif (!data->mark_query)\n \t\t\tstrbuf_addstr(sb, sha1_to_hex(data->sha1));\n \t} else if (is_atom(\"objecttype\", atom, len)) {\n-\t\tif (!data->mark_query)\n+\t\tif (data->mark_query)\n+\t\t\tdata->info.typep = &data->type;\n+\t\telse\n \t\t\tstrbuf_addstr(sb, typename(data->type));\n \t} else if (is_atom(\"objectsize\", atom, len)) {\n \t\tif (data->mark_query)\n@@ -229,8 +231,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \t\treturn 0;\n \t}\n \n-\tdata->type = sha1_object_info_extended(data->sha1, &data->info);\n-\tif (data->type <= 0) {\n+\tif (sha1_object_info_extended(data->sha1, &data->info) < 0) {\n \t\tprintf(\"%s missing\\n\", obj_name);\n \t\tfflush(stdout);\n \t\treturn 0;\ndiff --git a/cache.h b/cache.h\nindex c1fd82c..d3b770c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1130,6 +1130,7 @@ struct object_info {\n \n struct object_info {\n \t/* Request */\n+\tenum object_type *typep;\n \tunsigned long *sizep;\n \tunsigned long *disk_sizep;\n \ndiff --git a/sha1_file.c b/sha1_file.c\nindex 2a1e230..52f7a1e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2452,24 +2452,26 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n {\n \tstruct cached_object *co;\n \tstruct pack_entry e;\n-\tint type, rtype;\n+\tint rtype;\n \n \tco = find_cached_object(sha1);\n \tif (co) {\n+\t\tif (oi->typep)\n+\t\t\t*(oi->typep) = co->type;\n \t\tif (oi->sizep)\n \t\t\t*(oi->sizep) = co->size;\n \t\tif (oi->disk_sizep)\n \t\t\t*(oi->disk_sizep) = 0;\n \t\toi->whence = OI_CACHED;\n-\t\treturn co->type;\n+\t\treturn 0;\n \t}\n \n \tif (!find_pack_entry(sha1, &e)) {\n \t\t/* Most likely it's a loose object. */\n-\t\tif (!sha1_loose_object_info(sha1, &type,\n+\t\tif (!sha1_loose_object_info(sha1, oi->typep,\n \t\t\t\t\t    oi->sizep, oi->disk_sizep)) {\n \t\t\toi->whence = OI_LOOSE;\n-\t\t\treturn type;\n+\t\t\treturn 0;\n \t\t}\n \n \t\t/* Not a loose object; someone else may have just packed it. */\n@@ -2478,7 +2480,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \t\t\treturn -1;\n \t}\n \n-\trtype = packed_object_info(e.p, e.offset, &type, oi->sizep,\n+\trtype = packed_object_info(e.p, e.offset, oi->typep, oi->sizep,\n \t\t\t\t   oi->disk_sizep);\n \tif (rtype < 0) {\n \t\tmark_bad_packed_object(e.p, sha1);\n@@ -2493,15 +2495,19 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n \t\t\t\t\t rtype == OBJ_OFS_DELTA);\n \t}\n \n-\treturn type;\n+\treturn 0;\n }\n \n int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n {\n+\tenum object_type type;\n \tstruct object_info oi = {0};\n \n+\toi.typep = &type;\n \toi.sizep = sizep;\n-\treturn sha1_object_info_extended(sha1, &oi);\n+\tif (sha1_object_info_extended(sha1, &oi) < 0)\n+\t\treturn -1;\n+\treturn type;\n }\n \n static void *read_packed_sha1(const unsigned char *sha1,\ndiff --git a/streaming.c b/streaming.c\nindex cac282f..870657a 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -111,11 +111,11 @@ static enum input_source istream_source(const unsigned char *sha1,\n \tunsigned long size;\n \tint status;\n \n+\toi->typep = type;\n \toi->sizep = &size;\n \tstatus = sha1_object_info_extended(sha1, oi);\n \tif (status < 0)\n \t\treturn stream_error;\n-\t*type = status;\n \n \tswitch (oi->whence) {\n \tcase OI_LOOSE:\n-- \n1.8.3.rc3.24.gec82cb9\n"},{"id":"223146","messageId":"20130712063752.GG15572@sigill.intra.peff.net","threadId":"34413","inReplyTo":"20130712061533.GA11297@sigill.intra.peff.net","subject":"[PATCH 7/7] sha1_object_info_extended: pass object_info to helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T06:37:53Z","receivedAt":"2013-07-12T06:37:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We take in a \"struct object_info\" which contains pointers to\nstorage for items the caller cares about. But then rather\nthan pass the whole object to the low-level loose/packed\nhelper functions, we pass the individual pointers.\n\nLet's pass the whole struct instead, which will make adding\nmore items later easier.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis one is an optional cleanup. The diff is quite noisy due to all of\nthe s/foo/oi->foo/, so it is arguable whether the result is nicer or\nnot. It would make later additions to object_info nicer, but I do not\nplan to add any more.\n\nIt _would_ have been a nice cleanup to do at the beginning of the series\n(and further diffs would not have to add extra parameters to the\nfunction calls), but that would make the incremental \"learn to do type\noptionally\" patches quite awkward.\n\nSo I am on the fence over this one, and do not mind too much if it gets\ndropped.\n\n sha1_file.c | 49 ++++++++++++++++++++++---------------------------\n 1 file changed, 22 insertions(+), 27 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 52f7a1e..563f521 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1783,8 +1783,7 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,\n }\n \n static int packed_object_info(struct packed_git *p, off_t obj_offset,\n-\t\t\t      enum object_type *typep, unsigned long *sizep,\n-\t\t\t      unsigned long *disk_sizep)\n+\t\t\t      struct object_info *oi)\n {\n \tstruct pack_window *w_curs = NULL;\n \tunsigned long size;\n@@ -1797,7 +1796,7 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,\n \t */\n \ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n \n-\tif (sizep) {\n+\tif (oi->sizep) {\n \t\tif (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n \t\t\toff_t tmp_pos = curpos;\n \t\t\toff_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,\n@@ -1806,24 +1805,24 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,\n \t\t\t\ttype = OBJ_BAD;\n \t\t\t\tgoto out;\n \t\t\t}\n-\t\t\t*sizep = get_size_from_delta(p, &w_curs, tmp_pos);\n-\t\t\tif (*sizep == 0) {\n+\t\t\t*oi->sizep = get_size_from_delta(p, &w_curs, tmp_pos);\n+\t\t\tif (*oi->sizep == 0) {\n \t\t\t\ttype = OBJ_BAD;\n \t\t\t\tgoto out;\n \t\t\t}\n \t\t} else {\n-\t\t\t*sizep = size;\n+\t\t\t*oi->sizep = size;\n \t\t}\n \t}\n \n-\tif (disk_sizep) {\n+\tif (oi->disk_sizep) {\n \t\tstruct revindex_entry *revidx = find_pack_revindex(p, obj_offset);\n-\t\t*disk_sizep = revidx[1].offset - obj_offset;\n+\t\t*oi->disk_sizep = revidx[1].offset - obj_offset;\n \t}\n \n-\tif (typep) {\n-\t\t*typep = packed_to_object_type(p, obj_offset, type, &w_curs, curpos);\n-\t\tif (*typep < 0) {\n+\tif (oi->typep) {\n+\t\t*oi->typep = packed_to_object_type(p, obj_offset, type, &w_curs, curpos);\n+\t\tif (*oi->typep < 0) {\n \t\t\ttype = OBJ_BAD;\n \t\t\tgoto out;\n \t\t}\n@@ -2404,9 +2403,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n }\n \n static int sha1_loose_object_info(const unsigned char *sha1,\n-\t\t\t\t  enum object_type *typep,\n-\t\t\t\t  unsigned long *sizep,\n-\t\t\t\t  unsigned long *disk_sizep)\n+\t\t\t\t  struct object_info *oi)\n {\n \tint status;\n \tunsigned long mapsize, size;\n@@ -2418,12 +2415,12 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \t * If we don't care about type or size, then we don't\n \t * need to look inside the object at all.\n \t */\n-\tif (!typep && !sizep) {\n-\t\tif (disk_sizep) {\n+\tif (!oi->typep && !oi->sizep) {\n+\t\tif (oi->disk_sizep) {\n \t\t\tstruct stat st;\n \t\t\tif (stat_sha1_file(sha1, &st) < 0)\n \t\t\t\treturn -1;\n-\t\t\t*disk_sizep = st.st_size;\n+\t\t\t*oi->disk_sizep = st.st_size;\n \t\t}\n \t\treturn 0;\n \t}\n@@ -2431,19 +2428,19 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \tmap = map_sha1_file(sha1, &mapsize);\n \tif (!map)\n \t\treturn -1;\n-\tif (disk_sizep)\n-\t\t*disk_sizep = mapsize;\n+\tif (oi->disk_sizep)\n+\t\t*oi->disk_sizep = mapsize;\n \tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n \t\tstatus = error(\"unable to unpack %s header\",\n \t\t\t       sha1_to_hex(sha1));\n \telse if ((status = parse_sha1_header(hdr, &size)) < 0)\n \t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n-\telse if (sizep)\n-\t\t*sizep = size;\n+\telse if (oi->sizep)\n+\t\t*oi->sizep = size;\n \tgit_inflate_end(&stream);\n \tmunmap(map, mapsize);\n-\tif (typep)\n-\t\t*typep = status;\n+\tif (oi->typep)\n+\t\t*oi->typep = status;\n \treturn 0;\n }\n \n@@ -2468,8 +2465,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \n \tif (!find_pack_entry(sha1, &e)) {\n \t\t/* Most likely it's a loose object. */\n-\t\tif (!sha1_loose_object_info(sha1, oi->typep,\n-\t\t\t\t\t    oi->sizep, oi->disk_sizep)) {\n+\t\tif (!sha1_loose_object_info(sha1, oi)) {\n \t\t\toi->whence = OI_LOOSE;\n \t\t\treturn 0;\n \t\t}\n@@ -2480,8 +2476,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \t\t\treturn -1;\n \t}\n \n-\trtype = packed_object_info(e.p, e.offset, oi->typep, oi->sizep,\n-\t\t\t\t   oi->disk_sizep);\n+\trtype = packed_object_info(e.p, e.offset, oi);\n \tif (rtype < 0) {\n \t\tmark_bad_packed_object(e.p, sha1);\n \t\treturn sha1_object_info_extended(sha1, oi);\n-- \n1.8.3.rc3.24.gec82cb9\n"},{"id":"223151","messageId":"51DFC2B2.3080300@alum.mit.edu","threadId":"34413","inReplyTo":"20130712062004.GA15572@sigill.intra.peff.net","subject":"Re: [PATCH 1/7] cat-file: disable object/refname ambiguity check for batch mode","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-12T08:47:46Z","receivedAt":"2013-07-12T08:47:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/12/2013 08:20 AM, Jeff King wrote:\n> A common use of \"cat-file --batch-check\" is to feed a list\n> of objects from \"rev-list --objects\" or a similar command.\n> In this instance, all of our input objects are 40-byte sha1\n> ids. However, cat-file has always allowed arbitrary revision\n> specifiers, and feeds the result to get_sha1().\n> \n> Fortunately, get_sha1() recognizes a 40-byte sha1 before\n> doing any hard work trying to look up refs, meaning this\n> scenario should end up spending very little time converting\n> the input into an object sha1. However, since 798c35f\n> (get_sha1: warn about full or short object names that look\n> like refs, 2013-05-29), when we encounter this case, we\n> spend the extra effort to do a refname lookup anyway, just\n> to print a warning. This is further exacerbated by ca91993\n> (get_packed_ref_cache: reload packed-refs file when it\n> changes, 2013-06-20), which makes individual ref lookup more\n> expensive by requiring a stat() of the packed-refs file for\n> each missing ref.\n> \n> With no patches, this is the time it takes to run:\n> \n>   $ git rev-list --objects --all >objects\n>   $ time git cat-file --batch-check='%(objectname)' <objects\n> \n> on the linux.git repository:\n> \n>   real    1m13.494s\n>   user    0m25.924s\n>   sys     0m47.532s\n> \n> If we revert ca91993, the packed-refs up-to-date check, it\n> gets a little better:\n> \n>   real    0m54.697s\n>   user    0m21.692s\n>   sys     0m32.916s\n> \n> but we are still spending quite a bit of time on ref lookup\n> (and we would not want to revert that patch, anyway, which\n> has correctness issues).  If we revert 798c35f, disabling\n> the warning entirely, we get a much more reasonable time:\n> \n>   real    0m7.452s\n>   user    0m6.836s\n>   sys     0m0.608s\n> \n> This patch does the moral equivalent of this final case (and\n> gets similar speedups). We introduce a global flag that\n> callers of get_sha1() can use to avoid paying the price for\n> the warning.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> The solution feels a little hacky, but I'm not sure there is a better\n> one short of reverting the warning entirely.\n> \n> We could also tie it to warn_ambiguous_refs (or add another config\n> variable), but I don't think that makes sense. It is not about \"do I\n> care about ambiguities in this repository\", but rather \"I am going to\n> do a really large number of sha1 resolutions, and I do not want to pay\n> the price in this particular code path for the warning\").\n> \n> There may be other sites that resolve a large number of refs and run\n> into this, but I couldn't think of any. Doing for_each_ref would not\n> have the same problem, as we already know they are refs there.\n\nISTM that callers of --batch-check would usually know whether they are\nfeeding it SHA-1s or arbitrary object specifiers.  (And in most cases\nwhen there are a large number of them, they are probably all SHA-1s).\n\nSo instead of framing this change as \"skip object refname ambiguity\ncheck\" (i.e., sacrifice some correctness for performance), perhaps it\nwould be less hacky to offer the following new feature:\n\nTo cat-file we could add an option like \"--sha1-only\" or \"--literal\" or\n\"--no-dwim\" (... better names are failing me) which would skip *all*\ndwimming of 40-character strings.  It would also assume that any shorter\nstrings are abbreviated SHA-1s and fail if they are not.  This would be\na nice feature by itself (\"these are object names, dammit, and don't try\nto tell me differently!\") and would have the additional small advantage\nof speeding up lookups of abbreviated SHA-1s, which (regardless of your\npatch) otherwise go through the whole DWIM process.\n\nI understand that such an option alone would leave some old scripts\nsuffering the performance regression caused by the check, but in most\ncases it would be easy to fix them.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"223155","messageId":"20130712092212.GA4859@sigill.intra.peff.net","threadId":"34413","inReplyTo":"51DFC2B2.3080300@alum.mit.edu","subject":"Re: [PATCH 1/7] cat-file: disable object/refname ambiguity check for batch mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T09:22:12Z","receivedAt":"2013-07-12T09:22:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 12, 2013 at 10:47:46AM +0200, Michael Haggerty wrote:\n\n> > The solution feels a little hacky, but I'm not sure there is a better\n> > one short of reverting the warning entirely.\n> > \n> > We could also tie it to warn_ambiguous_refs (or add another config\n> > variable), but I don't think that makes sense. It is not about \"do I\n> > care about ambiguities in this repository\", but rather \"I am going to\n> > do a really large number of sha1 resolutions, and I do not want to pay\n> > the price in this particular code path for the warning\").\n> > \n> > There may be other sites that resolve a large number of refs and run\n> > into this, but I couldn't think of any. Doing for_each_ref would not\n> > have the same problem, as we already know they are refs there.\n> \n> ISTM that callers of --batch-check would usually know whether they are\n> feeding it SHA-1s or arbitrary object specifiers.  (And in most cases\n> when there are a large number of them, they are probably all SHA-1s).\n\nThanks for bringing this up; I had meant to outline this option in the\ncover letter, but forgot when posting.\n\nIf were designing cat-file from scratch, I'd consider having --batch\ntake just 40-hex sha1s in the first place. Since we can't do that now\nfor backwards compatibility, having an option is the best we can do\nthere.\n\nBut having to use such an option to avoid a 10x slowdown just strikes me\nas ugly for two reasons:\n\n  1. It's part of the user-visible interface, and it seems like an extra\n     \"wtf?\" moment when somebody wonders why their script is painfully\n     slow, and why they need a magic option to fix it. We're cluttering\n     the interface forever.\n\n  2. It's a sign that the refname check was put in for a specific\n     performance profile (resolving one or a few refs), but didn't\n     consider resolving a large number. I'm wondering what other\n     performance regressions it's possible to trigger.\n\n> So instead of framing this change as \"skip object refname ambiguity\n> check\" (i.e., sacrifice some correctness for performance), perhaps it\n> would be less hacky to offer the following new feature:\n\nI wouldn't frame it as \"correctness for performance\" at all. The check\ndoes not impact behavior at all, and is purely informational (to catch\nquite a rare situation, too). I'd argue that the check simply isn't that\ninteresting in this case in the first place.\n\n> To cat-file we could add an option like \"--sha1-only\" or \"--literal\" or\n> \"--no-dwim\" (... better names are failing me) which would skip *all*\n> dwimming of 40-character strings.  It would also assume that any shorter\n> strings are abbreviated SHA-1s and fail if they are not.  This would be\n> a nice feature by itself (\"these are object names, dammit, and don't try\n> to tell me differently!\") and would have the additional small advantage\n> of speeding up lookups of abbreviated SHA-1s, which (regardless of your\n> patch) otherwise go through the whole DWIM process.\n\nI can see in theory that somebody might want that, but I am having a\nhard time thinking of a practical use.\n\n> I understand that such an option alone would leave some old scripts\n> suffering the performance regression caused by the check, but in most\n> cases it would be easy to fix them.\n\nI'm less worried about old scripts, and more worried about carrying\naround a confusing option forever, especially when I expect most\ninvocations to use it (perhaps my experience is biased, but I use\ncat-file --batch quite a lot in scripting, and it has almost always been\non the output of rev-list).\n\nIOW, it seems like a poor default, and we are choosing it only because\nof backwards compatibility. I guess another option is to switch the\ndefault with the usual deprecation dance.\n\nYet another option is to consider what the check is doing, and\naccomplish the same thing in a different way. The real pain is that we\nare individually trying to resolve each object by hitting the filesystem\n(and doing lots of silly checks on the refname format, when we know it\nmust be valid).\n\nWe don't actually care in this case if the ref list is up to date (we\nare not trying to update or read a ref, but only know if it exists, and\nraciness is OK). IOW, could we replace the dwim_ref call for the warning\nwith something that directly queries the ref cache?\n\n-Peff\n"},{"id":"223158","messageId":"51DFDAAF.60808@alum.mit.edu","threadId":"34413","inReplyTo":"20130712092212.GA4859@sigill.intra.peff.net","subject":"Re: [PATCH 1/7] cat-file: disable object/refname ambiguity check for batch mode","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-12T10:30:07Z","receivedAt":"2013-07-12T10:30:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/12/2013 11:22 AM, Jeff King wrote:\n> Yet another option is to consider what the check is doing, and\n> accomplish the same thing in a different way. The real pain is that we\n> are individually trying to resolve each object by hitting the filesystem\n> (and doing lots of silly checks on the refname format, when we know it\n> must be valid).\n> \n> We don't actually care in this case if the ref list is up to date (we\n> are not trying to update or read a ref, but only know if it exists, and\n> raciness is OK). IOW, could we replace the dwim_ref call for the warning\n> with something that directly queries the ref cache?\n\nI think it would be quite practical to add an API something like\n\n    struct ref_snapshot *get_ref_snapshot(const char *prefix)\n    void release_ref_snapshot(struct ref_snapshot *)\n    int lookup_ref(struct ref_snapshot *, const char *refname,\n                   unsigned char *sha1, int *flags)\n\nwhere prefix is the part of the refs tree that you want included in the\nsnapshot (e.g., \"refs/heads\") and ref_snapshot is probably opaque\noutside of the refs module.\n\nSymbolic refs, which are currently not stored in the ref_cache, would\nhave to be added because otherwise we would have to do all of the\nlookups anyway.\n\nI think this would be a good step to take for many reasons, including\nbecause it would be another useful step in the direction of ref\ntransactions.\n\nBut with particular respect to \"git cat-file\", I see problems:\n\n1. get_ref_snapshot() would have to read all loose and packed refs\nwithin the specified subtree, because loose refs have to be read before\npacked refs.  So the call would be expensive if there are a lot of loose\nrefs.  And DWIM wouldn't know in advance where the references might be,\nso it would have to set prefix=\"\".  If many refs are looked up, then it\nwould presumably be worth it.  But if only a couple of lookups are done\nand there are a lot of loose refs, then using a cache would probably\nslow things down.\n\nThe slowdown could be ameliorated by adding some more intelligence, for\nexample only populating the loose refs cache after a certain number of\nlookups have already been done.\n\n2. A \"git cat-file --batch\" process can be long-lived.  What guarantees\nwould users expect regarding its lookup results?  Currently, its ref\nlookups reflect the state of the repo at the moment the commit\nidentifier is written into the pipe.  Using a cache like this would mean\nthat ref lookups would always reflect the snapshot taken at the start of\nthe \"git cat-file\" run, regardless of whether the script using it might\nhave added or modified some references since then.  I think this would\nhave to be considered a regression.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"223173","messageId":"7vy59blmjt.fsf@alter.siamese.dyndns.org","threadId":"34413","inReplyTo":"20130712061533.GA11297@sigill.intra.peff.net","subject":"Re: [PATCH 0/7] cat-file --batch-check performance improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-12T17:23:34Z","receivedAt":"2013-07-12T17:23:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The results for running (in linux.git):\n>\n>   $ git rev-list --objects --all >objects\n>   $ git cat-file --batch-check='%(objectsize:disk)' <objects >/dev/null\n\nI can see how these patches are very logical avenue to grab only\non-disk footprint for large number of objects, but among the type,\npayload size and on-disk footprint, I find it highly narrow niche\nthat a real user or script is interested _only_ in on-disk footprint\nwithout even worrying about the type of object.\n\n> ... (though I think the result actually cleans up the\n> sha1_object_info_extended interface a bit, and is worth it).\n\nI tend to agree, especially eyeballing the result of 7/7.\n\nThanks.\n"},{"id":"223212","messageId":"20130712201254.GA5276@sigill.intra.peff.net","threadId":"34413","inReplyTo":"7vy59blmjt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/7] cat-file --batch-check performance improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T20:12:54Z","receivedAt":"2013-07-12T20:12:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 12, 2013 at 10:23:34AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The results for running (in linux.git):\n> >\n> >   $ git rev-list --objects --all >objects\n> >   $ git cat-file --batch-check='%(objectsize:disk)' <objects >/dev/null\n> \n> I can see how these patches are very logical avenue to grab only\n> on-disk footprint for large number of objects, but among the type,\n> payload size and on-disk footprint, I find it highly narrow niche\n> that a real user or script is interested _only_ in on-disk footprint\n> without even worrying about the type of object.\n\nYeah, I agree it is a bit of a niche. However, there are other code\npaths that might want only the size and not the type (e.g., we already\nknow the object is a blob, but want to know size before deciding how to\nhandle diff). But in general, I doubt the performance impact is a big\ndeal there. It's only measurable when you're doing millions of objects.\n\n-Peff\n"},{"id":"223367","messageId":"7vk3ksfpum.fsf@alter.siamese.dyndns.org","threadId":"34413","inReplyTo":"20130712092212.GA4859@sigill.intra.peff.net","subject":"Re: [PATCH 1/7] cat-file: disable object/refname ambiguity check for batch mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-15T03:45:37Z","receivedAt":"2013-07-15T03:45:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> To cat-file we could add an option like \"--sha1-only\" or \"--literal\" or\n>> \"--no-dwim\" (... better names are failing me) which would skip *all*\n>> dwimming of 40-character strings.  It would also assume that any shorter\n>> strings are abbreviated SHA-1s and fail if they are not.  This would be\n>> a nice feature by itself (\"these are object names, dammit, and don't try\n>> to tell me differently!\") and would have the additional small advantage\n>> of speeding up lookups of abbreviated SHA-1s, which (regardless of your\n>> patch) otherwise go through the whole DWIM process.\n>\n> I can see in theory that somebody might want that, but I am having a\n> hard time thinking of a practical use.\n\nWould it be a good alternative to call get_sha1_hex() to catch the\nmost common case (reading from rev-list output, for example) and\nthen let the more general get_sha1() to let extended SHA-1 to be\nhandled?\n\n> IOW, it seems like a poor default, and we are choosing it only because\n> of backwards compatibility. I guess another option is to switch the\n> default with the usual deprecation dance.\n\nI agree that \"did you mean the unreadable refname or 40-hex object?\"\nturned on everywhere get_sha1() is called is a very poor default.  I\nwonder if we can limit it only to the end-user input somehow at the\nAPI level.\n"},{"id":"223373","messageId":"20130715041712.GA20900@sigill.intra.peff.net","threadId":"34413","inReplyTo":"7vk3ksfpum.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/7] cat-file: disable object/refname ambiguity check for batch mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-15T04:17:13Z","receivedAt":"2013-07-15T04:17:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 14, 2013 at 08:45:37PM -0700, Junio C Hamano wrote:\n\n> >> To cat-file we could add an option like \"--sha1-only\" or \"--literal\" or\n> >> \"--no-dwim\" (... better names are failing me) which would skip *all*\n> >> dwimming of 40-character strings.  It would also assume that any shorter\n> >> strings are abbreviated SHA-1s and fail if they are not.  This would be\n> >> a nice feature by itself (\"these are object names, dammit, and don't try\n> >> to tell me differently!\") and would have the additional small advantage\n> >> of speeding up lookups of abbreviated SHA-1s, which (regardless of your\n> >> patch) otherwise go through the whole DWIM process.\n> >\n> > I can see in theory that somebody might want that, but I am having a\n> > hard time thinking of a practical use.\n> \n> Would it be a good alternative to call get_sha1_hex() to catch the\n> most common case (reading from rev-list output, for example) and\n> then let the more general get_sha1() to let extended SHA-1 to be\n> handled?\n\nFor a 40-byte sha1, that _should_ be what get_sha1 does (i.e., go more\nor less directly to the 40-hex code path, and return). And that's\nbasically what happens now, except that after we do so, we now have the\nextra \"oh, is it also a refname?\" check.\n\nFor a shortened sha1, I don't think it would have the same behavior.\nRight now, I believe the order is to treat a short sha1 as a possible\nrefname, and only if that fails consider it as a short sha1.\n\n> > IOW, it seems like a poor default, and we are choosing it only because\n> > of backwards compatibility. I guess another option is to switch the\n> > default with the usual deprecation dance.\n> \n> I agree that \"did you mean the unreadable refname or 40-hex object?\"\n> turned on everywhere get_sha1() is called is a very poor default.  I\n> wonder if we can limit it only to the end-user input somehow at the\n> API level.\n\nIt is easy to do on top of my patch (just flip the default on the switch\nI introduced, and turn it back on in whichever code paths are\nappropriate).  But the question is: what is end-user input? Do you mean\ncommand-line arguments to \"rev-list\" and friends?\n\n-Peff\n"},{"id":"223375","messageId":"20130715042348.GB20900@sigill.intra.peff.net","threadId":"34413","inReplyTo":"51DFDAAF.60808@alum.mit.edu","subject":"Re: [PATCH 1/7] cat-file: disable object/refname ambiguity check for batch mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-15T04:23:48Z","receivedAt":"2013-07-15T04:23:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 12, 2013 at 12:30:07PM +0200, Michael Haggerty wrote:\n\n> But with particular respect to \"git cat-file\", I see problems:\n> \n> 1. get_ref_snapshot() would have to read all loose and packed refs\n> within the specified subtree, because loose refs have to be read before\n> packed refs.  So the call would be expensive if there are a lot of loose\n> refs.  And DWIM wouldn't know in advance where the references might be,\n> so it would have to set prefix=\"\".  If many refs are looked up, then it\n> would presumably be worth it.  But if only a couple of lookups are done\n> and there are a lot of loose refs, then using a cache would probably\n> slow things down.\n> \n> The slowdown could be ameliorated by adding some more intelligence, for\n> example only populating the loose refs cache after a certain number of\n> lookups have already been done.\n\nI had assumed we would load the loose ref cache progressively by\ndirectory. Of course that only helps avoiding \"refs/foo/*\" when you are\ninterested in \"refs/heads/foo\". If your \"refs/heads/*\" is very large,\nyou have to load all of it, and at some point it is cheaper to do a few\nstat() calls than to actually readdir() the whole directory. On the\nother hand, that is basically how packed-refs work now (if we hit any\nref, we have to load the whole file), and repos with many refs would\ntypically pack them all anyway.\n\n> 2. A \"git cat-file --batch\" process can be long-lived.  What guarantees\n> would users expect regarding its lookup results?\n\nI hadn't really intended this for general lookups, but just to speed up\nthe refname warning, where being out-of-date is more acceptable (since\nthe warning is a purely informational hint).\n\n-Peff\n"}]}