midx: add packs to packed_git linked list

The multi-pack-index allows searching for objects across multiple
packs using one object list. The original design gains many of
these performance benefits by keeping the packs in the
multi-pack-index out of the packed_git list.

Unfortunately, this has one major drawback. If the multi-pack-index
covers thousands of packs, and a command loads many of those packs,
then we can hit the limit for open file descriptors. The
close_one_pack() method is used to limit this resource, but it
only looks at the packed_git list, and uses an LRU cache to prevent
thrashing.

Instead of complicating this close_one_pack() logic to include
direct references to the multi-pack-index, simply add the packs
opened by the multi-pack-index to the packed_git list. This
immediately solves the file-descriptor limit problem, but requires
some extra steps to avoid performance issues or other problems:

1. Create a multi_pack_index bit in the packed_git struct that is
one if and only if the pack was loaded from a multi-pack-index.

2. Skip packs with the multi_pack_index bit when doing object
lookups and abbreviations. These algorithms already check the
multi-pack-index before the packed_git struct. This has a very
small performance hit, as we need to walk more packed_git
structs. This is acceptable, since these operations run binary
search on the other packs, so this walk-and-ignore logic is
very fast by comparison.

3. When closing a multi-pack-index file, do not close its packs,
as those packs will be closed using close_all_packs(). In some
cases, such as 'git repack', we run 'close_midx()' without also
closing the packs, so we need to un-set the multi_pack_index bit
in those packs. This is necessary, and caught by running
t6501-freshen-objects.sh with GIT_TEST_MULTI_PACK_INDEX=1.

To manually test this change, I inserted trace2 logging into
close_pack_fd() and set pack_max_fds to 10, then ran 'git rev-list
--all --objects' on a copy of the Git repo with 300+ pack-files and
a multi-pack-index. The logs verified the packs are closed as
we read them beyond the file descriptor limit.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/midx.c b/midx.c
index 8b8faec35afbdc272c8838e3f238cc034c4938a4..e7e1fe4d65ac3be54154e44ff07cd6122011405c 100644 (file)
--- a/midx.c
+++ b/midx.c
@@ -192,10 +192,8 @@ void close_midx(struct multi_pack_index *m)
        m->fd = -1;
 
        for (i = 0; i < m->num_packs; i++) {
-               if (m->packs[i]) {
-                       close_pack(m->packs[i]);
-                       free(m->packs[i]);
-               }
+               if (m->packs[i])
+                       m->packs[i]->multi_pack_index = 0;
        }
        FREE_AND_NULL(m->packs);
        FREE_AND_NULL(m->pack_names);
@@ -204,6 +202,7 @@ void close_midx(struct multi_pack_index *m)
 int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id)
 {
        struct strbuf pack_name = STRBUF_INIT;
+       struct packed_git *p;
 
        if (pack_int_id >= m->num_packs)
                die(_("bad pack-int-id: %u (%u total packs)"),
@@ -215,9 +214,18 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t
        strbuf_addf(&pack_name, "%s/pack/%s", m->object_dir,
                    m->pack_names[pack_int_id]);
 
-       m->packs[pack_int_id] = add_packed_git(pack_name.buf, pack_name.len, m->local);
+       p = add_packed_git(pack_name.buf, pack_name.len, m->local);
        strbuf_release(&pack_name);
-       return !m->packs[pack_int_id];
+
+       if (!p)
+               return 1;
+
+       p->multi_pack_index = 1;
+       m->packs[pack_int_id] = p;
+       install_packed_git(r, p);
+       list_add_tail(&p->mru, &r->objects->packed_git_mru);
+
+       return 0;
 }
 
 int bsearch_midx(const struct object_id *oid, struct multi_pack_index *m, uint32_t *result)

diff --git a/object-store.h b/object-store.h
index b086f5ecdb82a105b9b3381de9893cb72bc5871c..7acbc7fffe57dc9793d095368e232b84ae26ce3a 100644 (file)
--- a/object-store.h
+++ b/object-store.h
@@ -76,7 +76,8 @@ struct packed_git {
                 pack_keep_in_core:1,
                 freshened:1,
                 do_not_close:1,
-                pack_promisor:1;
+                pack_promisor:1,
+                multi_pack_index:1;
        unsigned char hash[GIT_MAX_RAWSZ];
        struct revindex_entry *revindex;
        /* something like ".git/objects/pack/xxxxx.pack" */
@@ -128,12 +129,6 @@ struct raw_object_store {
        /* A most-recently-used ordered version of the packed_git list. */
        struct list_head packed_git_mru;
 
-       /*
-        * A linked list containing all packfiles, starting with those
-        * contained in the multi_pack_index.
-        */
-       struct packed_git *all_packs;
-
        /*
         * A fast, rough count of the number of objects in the repository.
         * These two fields are not meant for direct access. Use

diff --git a/packfile.c b/packfile.c
index 7b94a14726d0641e24c5248502e11933472da890..060de420d1a4d39e9303e3a8ed3c672dd3025f98 100644 (file)
--- a/packfile.c
+++ b/packfile.c
@@ -994,8 +994,6 @@ static void prepare_packed_git(struct repository *r)
        }
        rearrange_packed_git(r);
 
-       r->objects->all_packs = NULL;
-
        prepare_packed_git_mru(r);
        r->objects->packed_git_initialized = 1;
 }
@@ -1026,26 +1024,16 @@ struct multi_pack_index *get_multi_pack_index(struct repository *r)
 
 struct packed_git *get_all_packs(struct repository *r)
 {
-       prepare_packed_git(r);
-
-       if (!r->objects->all_packs) {
-               struct packed_git *p = r->objects->packed_git;
-               struct multi_pack_index *m;
-
-               for (m = r->objects->multi_pack_index; m; m = m->next) {
-                       uint32_t i;
-                       for (i = 0; i < m->num_packs; i++) {
-                               if (!prepare_midx_pack(r, m, i)) {
-                                       m->packs[i]->next = p;
-                                       p = m->packs[i];
-                               }
-                       }
-               }
+       struct multi_pack_index *m;
 
-               r->objects->all_packs = p;
+       prepare_packed_git(r);
+       for (m = r->objects->multi_pack_index; m; m = m->next) {
+               uint32_t i;
+               for (i = 0; i < m->num_packs; i++)
+                       prepare_midx_pack(r, m, i);
        }
 
-       return r->objects->all_packs;
+       return r->objects->packed_git;
 }
 
 struct list_head *get_packed_git_mru(struct repository *r)
@@ -2004,7 +1992,7 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa
 
        list_for_each(pos, &r->objects->packed_git_mru) {
                struct packed_git *p = list_entry(pos, struct packed_git, mru);
-               if (fill_pack_entry(oid, e, p)) {
+               if (!p->multi_pack_index && fill_pack_entry(oid, e, p)) {
                        list_move(&p->mru, &r->objects->packed_git_mru);
                        return 1;
                }

diff --git a/sha1-name.c b/sha1-name.c
index 07c71a7567012aedc7faeccfa5c89cf2363d42c0..42ac1c5bb6b1a7875f908bcdfb42c198098aaf56 100644 (file)
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -157,6 +157,9 @@ static void unique_in_pack(struct packed_git *p,
        uint32_t num, i, first = 0;
        const struct object_id *current = NULL;
 
+       if (p->multi_pack_index)
+               return;
+
        if (open_pack_index(p) || !p->num_objects)
                return;
 
@@ -589,6 +592,9 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
        struct object_id oid;
        const struct object_id *mad_oid;
 
+       if (p->multi_pack_index)
+               return;
+
        if (open_pack_index(p) || !p->num_objects)
                return;