block alloc: allocate cache entries from mem_pool

When reading large indexes from disk, a portion of the time is dominated in malloc() calls. This can be mitigated by allocating a large block of memory and manage it ourselves via memory pools. This change moves the cache entry allocation to be on top of memory pools. Design: The index_state struct will gain a notion of an associated memory_pool from which cache_entries will be allocated from. When reading in the index from disk, we have information on the number of entries and their size, which can guide us in deciding how large our initial memory allocation should be. When an index is discarded, the associated memory_pool will be discarded as well - so the lifetime of a cache_entry is tied to the lifetime of the index_state that it was allocated for. In the case of a Split Index, the following rules are followed. 1st, some terminology is defined: Terminology: - 'the_index': represents the logical view of the index - 'split_index': represents the "base" cache entries. Read from the split index file. 'the_index' can reference a single split_index, as well as cache_entries from the split_index. `the_index` will be discarded before the `split_index` is. This means that when we are allocating cache_entries in the presence of a split index, we need to allocate the entries from the `split_index`'s memory pool. This allows us to follow the pattern that `the_index` can reference cache_entries from the `split_index`, and that the cache_entries will not be freed while they are still being referenced. Managing transient cache_entry structs: Cache entries are usually allocated for an index, but this is not always the case. Cache entries are sometimes allocated because this is the type that the existing checkout_entry function works with. Because of this, the existing code needs to handle cache entries associated with an index / memory pool, and those that only exist transiently. Several strategies were contemplated around how to handle this: Chosen approach: An extra field was added to the cache_entry type to track whether the cache_entry was allocated from a memory pool or not. This is currently an int field, as there are no more available bits in the existing ce_flags bit field. If / when more bits are needed, this new field can be turned into a proper bit field. Alternatives: 1) Do not include any information about how the cache_entry was allocated. Calling code would be responsible for tracking whether the cache_entry needed to be freed or not. Pro: No extra memory overhead to track this state Con: Extra complexity in callers to handle this correctly. The extra complexity and burden to not regress this behavior in the future was more than we wanted. 2) cache_entry would gain knowledge about which mem_pool allocated it Pro: Could (potentially) do extra logic to know when a mem_pool no longer had references to any cache_entry Con: cache_entry would grow heavier by a pointer, instead of int We didn't see a tangible benefit to this approach 3) Do not add any extra information to a cache_entry, but when freeing a cache entry, check if the memory exists in a region managed by existing mem_pools. Pro: No extra memory overhead to track state Con: Extra computation is performed when freeing cache entries We decided tracking and iterating over known memory pool regions was less desirable than adding an extra field to track this stae. Signed-off-by: Jameson Miller <jamill@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jameson Miller committed Jul 2, 2018 at 19:49 UTC 8e72d67529ba0adfb6f7d24a21e909c2a1e1d069
5 files changed +167 -39
cache.h
+21
@@ -15,6 +15,7 @@
15 #include "path.h"
16 #include "sha1-array.h"
17 #include "repository.h"
18 +#include "mem-pool.h"
19
20 #include <zlib.h>
21 typedef struct git_zstream {
@@ -156,6 +157,7 @@ struct cache_entry {
157 struct stat_data ce_stat_data;
158 unsigned int ce_mode;
159 unsigned int ce_flags;
160 + unsigned int mem_pool_allocated;
161 unsigned int ce_namelen;
162 unsigned int index; /* for link extension */
163 struct object_id oid;
@@ -227,6 +229,7 @@ static inline void copy_cache_entry(struct cache_entry *dst,
229 const struct cache_entry *src)
230 {
231 unsigned int state = dst->ce_flags & CE_HASHED;
232 + int mem_pool_allocated = dst->mem_pool_allocated;
233
234 /* Don't copy hash chain and name */
235 memcpy(&dst->ce_stat_data, &src->ce_stat_data,
@@ -235,6 +238,9 @@ static inline void copy_cache_entry(struct cache_entry *dst,
238
239 /* Restore the hash state */
240 dst->ce_flags = (dst->ce_flags & ~CE_HASHED) | state;
241 +
242 + /* Restore the mem_pool_allocated flag */
243 + dst->mem_pool_allocated = mem_pool_allocated;
244 }
245
246 static inline unsigned create_ce_flags(unsigned stage)
@@ -328,6 +334,7 @@ struct index_state {
334 struct untracked_cache *untracked;
335 uint64_t fsmonitor_last_update;
336 struct ewah_bitmap *fsmonitor_dirty;
337 + struct mem_pool *ce_mem_pool;
338 };
339
340 extern struct index_state the_index;
@@ -373,6 +380,20 @@ struct cache_entry *make_empty_transient_cache_entry(size_t name_len);
380 */
381 void discard_cache_entry(struct cache_entry *ce);
382
383 +/*
384 + * Duplicate a cache_entry. Allocate memory for the new entry from a
385 + * memory_pool. Takes into account cache_entry fields that are meant
386 + * for managing the underlying memory allocation of the cache_entry.
387 + */
388 +struct cache_entry *dup_cache_entry(const struct cache_entry *ce, struct index_state *istate);
389 +
390 +/*
391 + * Validate the cache entries in the index. This is an internal
392 + * consistency check that the cache_entry structs are allocated from
393 + * the expected memory pool.
394 + */
395 +void validate_cache_entries(const struct index_state *istate);
396 +
397 #ifndef NO_THE_INDEX_COMPATIBILITY_MACROS
398 #define active_cache (the_index.cache)
399 #define active_nr (the_index.cache_nr)
mem-pool.c
+2 -1
@@ -54,7 +54,8 @@ void mem_pool_discard(struct mem_pool *mem_pool)
54 {
55 struct mp_block *block, *block_to_free;
56
57 - while ((block = mem_pool->mp_block))
57 + block = mem_pool->mp_block;
58 + while (block)
59 {
60 block_to_free = block;
61 block = block->next_block;
read-cache.c
+100 -19
@@ -46,6 +46,48 @@
46 CE_ENTRY_ADDED | CE_ENTRY_REMOVED | CE_ENTRY_CHANGED | \
47 SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED | FSMONITOR_CHANGED)
48
49 +
50 +/*
51 + * This is an estimate of the pathname length in the index. We use
52 + * this for V4 index files to guess the un-deltafied size of the index
53 + * in memory because of pathname deltafication. This is not required
54 + * for V2/V3 index formats because their pathnames are not compressed.
55 + * If the initial amount of memory set aside is not sufficient, the
56 + * mem pool will allocate extra memory.
57 + */
58 +#define CACHE_ENTRY_PATH_LENGTH 80
59 +
60 +static inline struct cache_entry *mem_pool__ce_alloc(struct mem_pool *mem_pool, size_t len)
61 +{
62 + struct cache_entry *ce;
63 + ce = mem_pool_alloc(mem_pool, cache_entry_size(len));
64 + ce->mem_pool_allocated = 1;
65 + return ce;
66 +}
67 +
68 +static inline struct cache_entry *mem_pool__ce_calloc(struct mem_pool *mem_pool, size_t len)
69 +{
70 + struct cache_entry * ce;
71 + ce = mem_pool_calloc(mem_pool, 1, cache_entry_size(len));
72 + ce->mem_pool_allocated = 1;
73 + return ce;
74 +}
75 +
76 +static struct mem_pool *find_mem_pool(struct index_state *istate)
77 +{
78 + struct mem_pool **pool_ptr;
79 +
80 + if (istate->split_index && istate->split_index->base)
81 + pool_ptr = &istate->split_index->base->ce_mem_pool;
82 + else
83 + pool_ptr = &istate->ce_mem_pool;
84 +
85 + if (!*pool_ptr)
86 + mem_pool_init(pool_ptr, 0);
87 +
88 + return *pool_ptr;
89 +}
90 +
91 struct index_state the_index;
92 static const char *alternate_index_output;
93
@@ -746,7 +788,7 @@ int add_file_to_index(struct index_state *istate, const char *path, int flags)
788
789 struct cache_entry *make_empty_cache_entry(struct index_state *istate, size_t len)
790 {
749 - return xcalloc(1, cache_entry_size(len));
791 + return mem_pool__ce_calloc(find_mem_pool(istate), len);
792 }
793
794 struct cache_entry *make_empty_transient_cache_entry(size_t len)
@@ -1668,13 +1710,13 @@ int read_index(struct index_state *istate)
1710 return read_index_from(istate, get_index_file(), get_git_dir());
1711 }
1712
1671 -static struct cache_entry *cache_entry_from_ondisk(struct index_state *istate,
1713 +static struct cache_entry *cache_entry_from_ondisk(struct mem_pool *mem_pool,
1714 struct ondisk_cache_entry *ondisk,
1715 unsigned int flags,
1716 const char *name,
1717 size_t len)
1718 {
1677 - struct cache_entry *ce = make_empty_cache_entry(istate, len);
1719 + struct cache_entry *ce = mem_pool__ce_alloc(mem_pool, len);
1720
1721 ce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);
1722 ce->ce_stat_data.sd_mtime.sec = get_be32(&ondisk->mtime.sec);
@@ -1716,7 +1758,7 @@ static unsigned long expand_name_field(struct strbuf *name, const char *cp_)
1758 return (const char *)ep + 1 - cp_;
1759 }
1760
1719 -static struct cache_entry *create_from_disk(struct index_state *istate,
1761 +static struct cache_entry *create_from_disk(struct mem_pool *mem_pool,
1762 struct ondisk_cache_entry *ondisk,
1763 unsigned long *ent_size,
1764 struct strbuf *previous_name)
@@ -1748,13 +1790,13 @@ static struct cache_entry *create_from_disk(struct index_state *istate,
1790 /* v3 and earlier */
1791 if (len == CE_NAMEMASK)
1792 len = strlen(name);
1751 - ce = cache_entry_from_ondisk(istate, ondisk, flags, name, len);
1793 + ce = cache_entry_from_ondisk(mem_pool, ondisk, flags, name, len);
1794
1795 *ent_size = ondisk_ce_size(ce);
1796 } else {
1797 unsigned long consumed;
1798 consumed = expand_name_field(previous_name, name);
1757 - ce = cache_entry_from_ondisk(istate, ondisk, flags,
1799 + ce = cache_entry_from_ondisk(mem_pool, ondisk, flags,
1800 previous_name->buf,
1801 previous_name->len);
1802
@@ -1828,6 +1870,22 @@ static void post_read_index_from(struct index_state *istate)
1870 tweak_fsmonitor(istate);
1871 }
1872
1873 +static size_t estimate_cache_size_from_compressed(unsigned int entries)
1874 +{
1875 + return entries * (sizeof(struct cache_entry) + CACHE_ENTRY_PATH_LENGTH);
1876 +}
1877 +
1878 +static size_t estimate_cache_size(size_t ondisk_size, unsigned int entries)
1879 +{
1880 + long per_entry = sizeof(struct cache_entry) - sizeof(struct ondisk_cache_entry);
1881 +
1882 + /*
1883 + * Account for potential alignment differences.
1884 + */
1885 + per_entry += align_padding_size(sizeof(struct cache_entry), -sizeof(struct ondisk_cache_entry));
1886 + return ondisk_size + entries * per_entry;
1887 +}
1888 +
1889 /* remember to discard_cache() before reading a different cache! */
1890 int do_read_index(struct index_state *istate, const char *path, int must_exist)
1891 {
@@ -1874,10 +1932,15 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
1932 istate->cache = xcalloc(istate->cache_alloc, sizeof(*istate->cache));
1933 istate->initialized = 1;
1934
1877 - if (istate->version == 4)
1935 + if (istate->version == 4) {
1936 previous_name = &previous_name_buf;
1879 - else
1937 + mem_pool_init(&istate->ce_mem_pool,
1938 + estimate_cache_size_from_compressed(istate->cache_nr));
1939 + } else {
1940 previous_name = NULL;
1941 + mem_pool_init(&istate->ce_mem_pool,
1942 + estimate_cache_size(mmap_size, istate->cache_nr));
1943 + }
1944
1945 src_offset = sizeof(*hdr);
1946 for (i = 0; i < istate->cache_nr; i++) {
@@ -1886,7 +1949,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
1949 unsigned long consumed;
1950
1951 disk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);
1889 - ce = create_from_disk(istate, disk_ce, &consumed, previous_name);
1952 + ce = create_from_disk(istate->ce_mem_pool, disk_ce, &consumed, previous_name);
1953 set_index_entry(istate, i, ce);
1954
1955 src_offset += consumed;
@@ -1983,17 +2046,13 @@ int is_index_unborn(struct index_state *istate)
2046
2047 int discard_index(struct index_state *istate)
2048 {
1986 - int i;
2049 + /*
2050 + * Cache entries in istate->cache[] should have been allocated
2051 + * from the memory pool associated with this index, or from an
2052 + * associated split_index. There is no need to free individual
2053 + * cache entries.
2054 + */
2055
1988 - for (i = 0; i < istate->cache_nr; i++) {
1989 - if (istate->cache[i]->index &&
1990 - istate->split_index &&
1991 - istate->split_index->base &&
1992 - istate->cache[i]->index <= istate->split_index->base->cache_nr &&
1993 - istate->cache[i] == istate->split_index->base->cache[istate->cache[i]->index - 1])
1994 - continue;
1995 - discard_cache_entry(istate->cache[i]);
1996 - }
2056 resolve_undo_clear_index(istate);
2057 istate->cache_nr = 0;
2058 istate->cache_changed = 0;
@@ -2007,6 +2066,12 @@ int discard_index(struct index_state *istate)
2066 discard_split_index(istate);
2067 free_untracked_cache(istate->untracked);
2068 istate->untracked = NULL;
2069 +
2070 + if (istate->ce_mem_pool) {
2071 + mem_pool_discard(istate->ce_mem_pool);
2072 + istate->ce_mem_pool = NULL;
2073 + }
2074 +
2075 return 0;
2076 }
2077
@@ -2798,7 +2863,23 @@ void move_index_extensions(struct index_state *dst, struct index_state *src)
2863 src->untracked = NULL;
2864 }
2865
2866 +struct cache_entry *dup_cache_entry(const struct cache_entry *ce,
2867 + struct index_state *istate)
2868 +{
2869 + unsigned int size = ce_size(ce);
2870 + int mem_pool_allocated;
2871 + struct cache_entry *new_entry = make_empty_cache_entry(istate, ce_namelen(ce));
2872 + mem_pool_allocated = new_entry->mem_pool_allocated;
2873 +
2874 + memcpy(new_entry, ce, size);
2875 + new_entry->mem_pool_allocated = mem_pool_allocated;
2876 + return new_entry;
2877 +}
2878 +
2879 void discard_cache_entry(struct cache_entry *ce)
2880 {
2881 + if (ce && ce->mem_pool_allocated)
2882 + return;
2883 +
2884 free(ce);
2885 }
split-index.c
+42 -8
@@ -73,16 +73,31 @@ void move_cache_to_base_index(struct index_state *istate)
73 int i;
74
75 /*
76 - * do not delete old si->base, its index entries may be shared
77 - * with istate->cache[]. Accept a bit of leaking here because
78 - * this code is only used by short-lived update-index.
76 + * If there was a previous base index, then transfer ownership of allocated
77 + * entries to the parent index.
78 */
79 + if (si->base &&
80 + si->base->ce_mem_pool) {
81 +
82 + if (!istate->ce_mem_pool)
83 + mem_pool_init(&istate->ce_mem_pool, 0);
84 +
85 + mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool);
86 + }
87 +
88 si->base = xcalloc(1, sizeof(*si->base));
89 si->base->version = istate->version;
90 /* zero timestamp disables racy test in ce_write_index() */
91 si->base->timestamp = istate->timestamp;
92 ALLOC_GROW(si->base->cache, istate->cache_nr, si->base->cache_alloc);
93 si->base->cache_nr = istate->cache_nr;
94 +
95 + /*
96 + * The mem_pool needs to move with the allocated entries.
97 + */
98 + si->base->ce_mem_pool = istate->ce_mem_pool;
99 + istate->ce_mem_pool = NULL;
100 +
101 COPY_ARRAY(si->base->cache, istate->cache, istate->cache_nr);
102 mark_base_index_entries(si->base);
103 for (i = 0; i < si->base->cache_nr; i++)
@@ -331,12 +346,31 @@ void remove_split_index(struct index_state *istate)
346 {
347 if (istate->split_index) {
348 /*
334 - * can't discard_split_index(&the_index); because that
335 - * will destroy split_index->base->cache[], which may
336 - * be shared with the_index.cache[]. So yeah we're
337 - * leaking a bit here.
349 + * When removing the split index, we need to move
350 + * ownership of the mem_pool associated with the
351 + * base index to the main index. There may be cache entries
352 + * allocated from the base's memory pool that are shared with
353 + * the_index.cache[].
354 */
339 - istate->split_index = NULL;
355 + mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool);
356 +
357 + /*
358 + * The split index no longer owns the mem_pool backing
359 + * its cache array. As we are discarding this index,
360 + * mark the index as having no cache entries, so it
361 + * will not attempt to clean up the cache entries or
362 + * validate them.
363 + */
364 + if (istate->split_index->base)
365 + istate->split_index->base->cache_nr = 0;
366 +
367 + /*
368 + * We can discard the split index because its
369 + * memory pool has been incorporated into the
370 + * memory pool associated with the the_index.
371 + */
372 + discard_split_index(istate);
373 +
374 istate->cache_changed |= SOMETHING_CHANGED;
375 }
376 }
unpack-trees.c
+2 -11
@@ -203,20 +203,11 @@ static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
203 ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
204 }
205
206 -static struct cache_entry *dup_entry(const struct cache_entry *ce, struct index_state *istate)
207 -{
208 - unsigned int size = ce_size(ce);
209 - struct cache_entry *new_entry = make_empty_cache_entry(istate, ce_namelen(ce));
210 -
211 - memcpy(new_entry, ce, size);
212 - return new_entry;
213 -}
214 -
206 static void add_entry(struct unpack_trees_options *o,
207 const struct cache_entry *ce,
208 unsigned int set, unsigned int clear)
209 {
219 - do_add_entry(o, dup_entry(ce, &o->result), set, clear);
210 + do_add_entry(o, dup_cache_entry(ce, &o->result), set, clear);
211 }
212
213 /*
@@ -1802,7 +1793,7 @@ static int merged_entry(const struct cache_entry *ce,
1793 struct unpack_trees_options *o)
1794 {
1795 int update = CE_UPDATE;
1805 - struct cache_entry *merge = dup_entry(ce, &o->result);
1796 + struct cache_entry *merge = dup_cache_entry(ce, &o->result);
1797
1798 if (!old) {
1799 /*