{"thread":{"id":"32285","subject":"[PATCH 0/2] make repeated fetch faster on \"read only\" repos","startedAt":"2012-12-07T13:53:52Z","lastAt":"2012-12-07T14:04:44Z","messageCount":3,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"204571","messageId":"20121207135351.GA10538@sigill.intra.peff.net","threadId":"32285","inReplyTo":null,"subject":"[PATCH 0/2] make repeated fetch faster on \"read only\" repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-07T13:53:52Z","receivedAt":"2012-12-07T13:53:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Like many dev shops, we run a CI server that basically does:\n\n  git fetch $some_branch &&\n  git checkout $some_branch &&\n  make test\n\nall day long. Sometimes the fetches would get very slow, and the problem\nturned out to be a combination of:\n\n  1. Never running \"git gc\". This means you can end up with a ton of\n     loose objects, or even a bunch of small packs[1].\n\n  2. One of the loops in fetch caused us to re-scan the entire\n     objects/pack directory repeatedly, proportional to the number of\n     refs on the remote.\n\nI think the fundamental fix is to gc more often, as it makes the re-scans\nless expensive, along with making general object lookup faster. But the\nrepeated re-scans strike me as kind of hacky. This series tries to\naddress both:\n\n  [1/2]: fetch: run gc --auto after fetching\n  [2/2]: fetch-pack: avoid repeatedly re-scanning pack directory\n\n-Peff\n\n[1] It turns out we had our transfer.unpacklimit set unreasonably low,\n    leading to a large number of tiny packs, but even with the defaults,\n    you will end up with a ton of loose objects if you do repeated small\n    fetches.\n"},{"id":"204572","messageId":"20121207135800.GA10964@sigill.intra.peff.net","threadId":"32285","inReplyTo":"20121207135351.GA10538@sigill.intra.peff.net","subject":"[PATCH 1/2] fetch: run gc --auto after fetching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-07T13:58:00Z","receivedAt":"2012-12-07T13:58:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generally try to run \"gc --auto\" after any commands that\nmight introduce a large number of new objects. An obvious\nplace to do so is after running \"fetch\", which may introduce\nnew loose objects or packs (depending on the size of the\nfetch).\n\nWhile an active developer repository will probably\neventually trigger a \"gc --auto\" on another action (e.g.,\ngit-rebase), there are two good reasons why it is nicer to\ndo it at fetch time:\n\n  1. Read-only repositories which track an upstream (e.g., a\n     continuous integration server which fetches and builds,\n     but never makes new commits) will accrue loose objects\n     and small packs, but never coalesce them into a more\n     efficient larger pack.\n\n  2. Fetching is often already perceived to be slow to the\n     user, since they have to wait on the network. It's much\n     more pleasant to include a potentially slow auto-gc as\n     part of the already-long network fetch than in the\n     middle of productive work with git-rebase or similar.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe original \"gc --auto\" patch in 2007 suggested adding this to fetch,\nbut we never did (there was some brief mention that maybe tweaking\nfetch.unpacklimit would be relevant, but I think you would eventually\nwant to gc, whether you are accruing packs or loose objects).\n\nI didn't bother protecting this with an option (as we do for\nreceive.gc). I don't see much point, as anybody who doesn't want gc on\ntheir client repos will already have set gc.auto to prevent it from\ntriggering via other commands).\n\n builtin/fetch.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 4b5a898..1ddbf0d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -959,6 +959,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tstruct string_list list = STRING_LIST_INIT_NODUP;\n \tstruct remote *remote;\n \tint result = 0;\n+\tstatic const char *argv_gc_auto[] = {\n+\t\t\"gc\", \"--auto\", NULL,\n+\t};\n \n \tpacket_trace_identity(\"fetch\");\n \n@@ -1026,5 +1029,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tlist.strdup_strings = 1;\n \tstring_list_clear(&list, 0);\n \n+\trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n+\n \treturn result;\n }\n-- \n1.8.1.rc0.4.g5948dfd.dirty\n"},{"id":"204573","messageId":"20121207140444.GB10964@sigill.intra.peff.net","threadId":"32285","inReplyTo":"20121207135351.GA10538@sigill.intra.peff.net","subject":"[PATCH 2/2] fetch-pack: avoid repeatedly re-scanning pack directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-07T14:04:44Z","receivedAt":"2012-12-07T14:04:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we look up a sha1 object for reading, we first check\npackfiles, and then loose objects. If we still haven't found\nit, we re-scan the list of packfiles in `objects/pack`. This\nfinal step ensures that we can co-exist with a simultaneous\nrepack process which creates a new pack and then prunes the\nold object.\n\nThis extra re-scan usually does not have a performance\nimpact for two reasons:\n\n  1. If an object is missing, then typically the re-scan\n     will find a new pack, then no more misses will occur.\n     Or if it truly is missing, then our next step is\n     usually to die().\n\n  2. Re-scanning is cheap enough that we do not even notice.\n\nHowever, these do not always hold. The assumption in (1) is\nthat the caller is expecting to find the object. This is\nusually the case, but the call to `parse_object` in\n`everything_local` does not follow this pattern. It is\nlooking to see whether we have objects that the remote side\nis advertising, not something we expect to have. Therefore\nif we are fetching from a remote which has many refs\npointing to objects we do not have, we may end up\nre-scanning the pack directory many times.\n\nEven with this extra re-scanning, the impact is often not\nnoticeable due to (2); we just readdir() the packs directory\nand skip any packs that are already loaded. However, if\nthere are a large number of packs, then even enumerating the\ndirectory directory can be expensive (especially if we do it\nrepeatedly). Having this many packs is a good sign the user\nshould run `git gc`, but it would still be nice to avoid\nhaving to scan the directory at all.\n\nThis patch checks has_sha1_file (which does not have the\nre-scan and re-check behavior) in the critical loop, and\navoids calling parse_object at all if we do not have the\nobject.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm lukewarm on this patch.  The re-scan _shouldn't_ be that expensive,\nso maybe patch 1 is enough to be a reasonable fix. The fact that we\nre-scan repeatedly seems ugly and hacky to me, but it really is just\nopendir/readdir/closedir in the case that nothing has changed (and if\nsomething has changed, then it's a good thing to be checking). And with\nmy patch, fetch-pack would not notice new packs from a simultaneous\nrepack process (although it's OK, as the result is not incorrect, but\nmerely that we may ask for the object from the server).\n\nAnother option would be to make the reprepare_packed_git re-scan less\nexpensive by checking the mtime of the directory before scanning it.\n\n fetch-pack.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 099ff4d..b4383c6 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -594,6 +594,9 @@ static int everything_local(struct fetch_pack_args *args,\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct object *o;\n \n+\t\tif (!has_sha1_file(ref->old_sha1))\n+\t\t\tcontinue;\n+\n \t\to = parse_object(ref->old_sha1);\n \t\tif (!o)\n \t\t\tcontinue;\n-- \n1.8.1.rc0.4.g5948dfd.dirty\n"}]}