The index-pack command determines if a sha1 collision test is needed by checking the existence of a loose object with the given hash. In my tests, I can improve performance of “git clone” on Amazon EFS by 8x when used with a non-default mount option (lookupcache=pos) that's required for a Gitlab HA setup. My assumption is that this check is unnecessary when cloning into a new repository because the repository will be empty. By default, the Linux NFS client will cache directory entries as well as the non-existence of directory entries. The latter means that when client c1 does stat() on a file that does not exist, the non-existence will be cached and any subsequent stat() operation on the file will return -ENOENT until the cache expires or is invalidated, even if the file was created on client c2 in the mean time. This leads to errors in a Gitlab HA setup when it distributes jobs over multiple worker nodes assuming each worker node has the same view of the shared file system. The recommended workaround by Gitlab is to use the “lookupcache=pos” NFS mount option which disables the negative lookup cache. This option has a high performance impact. Cloning the gitlab-ce repository (https://gitlab.com/gitlab-org/gitlib-ce.git) into an NFS mounted directory gives the following results: lookupcache=all (default): 624 seconds lookupcache=pos: 4957 seconds The reason for the poor performance is that index-pack will issue a stat() call for every object in the repo when checking if a collision test is needed. These stat() calls result in the following NFS operations: LOOKUP dirfh=".git/objects", name="01" -> NFS4ERR_ENOENT With lookupcache=all, the non-existence of the .git/objects/XX directories is cached, so that there will be at most 256 LOOKUP calls. With lookupcache=pos, there will be one LOOKUP operation for every object in the repository, which in case of the gitlab-ce repo is about 1.3 million times. The attached patch removes the collision check when cloning into a new repository. The performance of git clone with this patch is: lookupcache=pos (with patch): 577 seconds I'd welcome feedback on the attached patch and whether my assumption that the sha1 collision check can be safely omitted when cloning into a new repository is correct. Signed-off-by: Geert Jansen <gerardu@xxxxxxxxxx> --- builtin/index-pack.c | 5 ++++- fetch-pack.c | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 2004e25da..22b3d40fb 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -84,6 +84,7 @@ static int verbose; static int show_resolving_progress; static int show_stat; static int check_self_contained_and_connected; +static int cloning; static struct progress *progress; @@ -794,7 +795,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry, assert(data || obj_entry); - if (startup_info->have_repository) { + if (startup_info->have_repository && !cloning) { read_lock(); collision_test_needed = has_sha1_file_with_flags(oid->hash, OBJECT_INFO_QUICK); @@ -1705,6 +1706,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) check_self_contained_and_connected = 1; } else if (!strcmp(arg, "--fsck-objects")) { do_fsck_object = 1; + } else if (!strcmp(arg, "--cloning")) { + cloning = 1; } else if (!strcmp(arg, "--verify")) { verify = 1; } else if (!strcmp(arg, "--verify-stat")) { diff --git a/fetch-pack.c b/fetch-pack.c index b3ed7121b..c75bfb8aa 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -843,6 +843,8 @@ static int get_pack(struct fetch_pack_args *args, argv_array_push(&cmd.args, "--check-self-contained-and-connected"); if (args->from_promisor) argv_array_push(&cmd.args, "--promisor"); + if (args->cloning) + argv_array_pushf(&cmd.args, "--cloning"); } else { cmd_name = "unpack-objects"; -- 2.19.1.328.g5a0cc8aca.dirty
