From 768d7965068189ec5287ac53b4b55bfd443bb456 Mon Sep 17 00:00:00 2001 From: Jameson Miller Date: Mon, 2 Jul 2018 19:49:29 +0000 Subject: read-cache: teach refresh_cache_entry to take istate Refactor refresh_cache_entry() to work on a specific index, instead of implicitly using the_index. This is in preparation for making the make_cache_entry function apply to a specific index. Signed-off-by: Jameson Miller Signed-off-by: Junio C Hamano --- cache.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'cache.h') diff --git a/cache.h b/cache.h index 89a107a7f7..9538511d9f 100644 --- a/cache.h +++ b/cache.h @@ -751,7 +751,7 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st); #define REFRESH_IGNORE_SUBMODULES 0x0010 /* ignore submodules */ #define REFRESH_IN_PORCELAIN 0x0020 /* user friendly output, not "needs update" */ extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg); -extern struct cache_entry *refresh_cache_entry(struct cache_entry *, unsigned int); +extern struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int); /* * Opportunistically update the index but do not complain if we can't. -- cgit v1.2.3 From 825ed4d9a044380ac093563e6bd74311ea4488ef Mon Sep 17 00:00:00 2001 From: Jameson Miller Date: Mon, 2 Jul 2018 19:49:30 +0000 Subject: read-cache: teach make_cache_entry to take object_id Teach make_cache_entry function to take object_id instead of a SHA-1. Signed-off-by: Junio C Hamano --- cache.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'cache.h') diff --git a/cache.h b/cache.h index 9538511d9f..7953d52caa 100644 --- a/cache.h +++ b/cache.h @@ -698,7 +698,12 @@ extern int remove_file_from_index(struct index_state *, const char *path); extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags); extern int add_file_to_index(struct index_state *, const char *path, int flags); -extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options); +extern struct cache_entry *make_cache_entry(unsigned int mode, + const struct object_id *oid, + const char *path, + int stage, + unsigned int refresh_options); + extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip); extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b); extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce); -- cgit v1.2.3 From a849735bfbf159b98ead9ef4c843dc8acfd372f0 Mon Sep 17 00:00:00 2001 From: Jameson Miller Date: Mon, 2 Jul 2018 19:49:31 +0000 Subject: block alloc: add lifecycle APIs for cache_entry structs It has been observed that the time spent loading an index with a large number of entries is partly dominated by malloc() calls. This change is in preparation for using memory pools to reduce the number of malloc() calls made to allocate cahce entries when loading an index. Add an API to allocate and discard cache entries, abstracting the details of managing the memory backing the cache entries. This commit does actually change how memory is managed - this will be done in a later commit in the series. This change makes the distinction between cache entries that are associated with an index and cache entries that are not associated with an index. A main use of cache entries is with an index, and we can optimize the memory management around this. We still have other cases where a cache entry is not persisted with an index, and so we need to handle the "transient" use case as well. To keep the congnitive overhead of managing the cache entries, there will only be a single discard function. This means there must be enough information kept with the cache entry so that we know how to discard them. A summary of the main functions in the API is: make_cache_entry: create cache entry for use in an index. Uses specified parameters to populate cache_entry fields. make_empty_cache_entry: Create an empty cache entry for use in an index. Returns cache entry with empty fields. make_transient_cache_entry: create cache entry that is not used in an index. Uses specified parameters to populate cache_entry fields. make_empty_transient_cache_entry: create cache entry that is not used in an index. Returns cache entry with empty fields. discard_cache_entry: A single function that knows how to discard a cache entry regardless of how it was allocated. Signed-off-by: Jameson Miller Signed-off-by: Junio C Hamano --- cache.h | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) (limited to 'cache.h') diff --git a/cache.h b/cache.h index 7953d52caa..5aadaebaeb 100644 --- a/cache.h +++ b/cache.h @@ -339,6 +339,40 @@ extern void remove_name_hash(struct index_state *istate, struct cache_entry *ce) extern void free_name_hash(struct index_state *istate); +/* Cache entry creation and cleanup */ + +/* + * Create cache_entry intended for use in the specified index. Caller + * is responsible for discarding the cache_entry with + * `discard_cache_entry`. + */ +struct cache_entry *make_cache_entry(struct index_state *istate, + unsigned int mode, + const struct object_id *oid, + const char *path, + int stage, + unsigned int refresh_options); + +struct cache_entry *make_empty_cache_entry(struct index_state *istate, + size_t name_len); + +/* + * Create a cache_entry that is not intended to be added to an index. + * Caller is responsible for discarding the cache_entry + * with `discard_cache_entry`. + */ +struct cache_entry *make_transient_cache_entry(unsigned int mode, + const struct object_id *oid, + const char *path, + int stage); + +struct cache_entry *make_empty_transient_cache_entry(size_t name_len); + +/* + * Discard cache entry. + */ +void discard_cache_entry(struct cache_entry *ce); + #ifndef NO_THE_INDEX_COMPATIBILITY_MACROS #define active_cache (the_index.cache) #define active_nr (the_index.cache_nr) @@ -698,12 +732,6 @@ extern int remove_file_from_index(struct index_state *, const char *path); extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags); extern int add_file_to_index(struct index_state *, const char *path, int flags); -extern struct cache_entry *make_cache_entry(unsigned int mode, - const struct object_id *oid, - const char *path, - int stage, - unsigned int refresh_options); - extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip); extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b); extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce); -- cgit v1.2.3 From 8e72d67529ba0adfb6f7d24a21e909c2a1e1d069 Mon Sep 17 00:00:00 2001 From: Jameson Miller Date: Mon, 2 Jul 2018 19:49:37 +0000 Subject: 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 Signed-off-by: Junio C Hamano --- cache.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) (limited to 'cache.h') diff --git a/cache.h b/cache.h index 5aadaebaeb..c1c7e9e65f 100644 --- a/cache.h +++ b/cache.h @@ -15,6 +15,7 @@ #include "path.h" #include "sha1-array.h" #include "repository.h" +#include "mem-pool.h" #include typedef struct git_zstream { @@ -156,6 +157,7 @@ struct cache_entry { struct stat_data ce_stat_data; unsigned int ce_mode; unsigned int ce_flags; + unsigned int mem_pool_allocated; unsigned int ce_namelen; unsigned int index; /* for link extension */ struct object_id oid; @@ -227,6 +229,7 @@ static inline void copy_cache_entry(struct cache_entry *dst, const struct cache_entry *src) { unsigned int state = dst->ce_flags & CE_HASHED; + int mem_pool_allocated = dst->mem_pool_allocated; /* Don't copy hash chain and name */ memcpy(&dst->ce_stat_data, &src->ce_stat_data, @@ -235,6 +238,9 @@ static inline void copy_cache_entry(struct cache_entry *dst, /* Restore the hash state */ dst->ce_flags = (dst->ce_flags & ~CE_HASHED) | state; + + /* Restore the mem_pool_allocated flag */ + dst->mem_pool_allocated = mem_pool_allocated; } static inline unsigned create_ce_flags(unsigned stage) @@ -328,6 +334,7 @@ struct index_state { struct untracked_cache *untracked; uint64_t fsmonitor_last_update; struct ewah_bitmap *fsmonitor_dirty; + struct mem_pool *ce_mem_pool; }; extern struct index_state the_index; @@ -373,6 +380,20 @@ struct cache_entry *make_empty_transient_cache_entry(size_t name_len); */ void discard_cache_entry(struct cache_entry *ce); +/* + * Duplicate a cache_entry. Allocate memory for the new entry from a + * memory_pool. Takes into account cache_entry fields that are meant + * for managing the underlying memory allocation of the cache_entry. + */ +struct cache_entry *dup_cache_entry(const struct cache_entry *ce, struct index_state *istate); + +/* + * Validate the cache entries in the index. This is an internal + * consistency check that the cache_entry structs are allocated from + * the expected memory pool. + */ +void validate_cache_entries(const struct index_state *istate); + #ifndef NO_THE_INDEX_COMPATIBILITY_MACROS #define active_cache (the_index.cache) #define active_nr (the_index.cache_nr) -- cgit v1.2.3 From 8616a2d0cb57865540f1c00ac2e5385a6cc5d84e Mon Sep 17 00:00:00 2001 From: Jameson Miller Date: Mon, 2 Jul 2018 19:49:39 +0000 Subject: block alloc: add validations around cache_entry lifecyle Add an option (controlled by an environment variable) perform extra validations on mem_pool allocated cache entries. When set: 1) Invalidate cache_entry memory when discarding cache_entry. 2) When discarding index_state struct, verify that all cache_entries were allocated from expected mem_pool. 3) When discarding mem_pools, invalidate mem_pool memory. This should provide extra checks that mem_pools and their allocated cache_entries are being used as expected. Signed-off-by: Jameson Miller Signed-off-by: Junio C Hamano --- cache.h | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'cache.h') diff --git a/cache.h b/cache.h index c1c7e9e65f..a3334a71ca 100644 --- a/cache.h +++ b/cache.h @@ -380,6 +380,12 @@ struct cache_entry *make_empty_transient_cache_entry(size_t name_len); */ void discard_cache_entry(struct cache_entry *ce); +/* + * Check configuration if we should perform extra validation on cache + * entries. + */ +int should_validate_cache_entries(void); + /* * Duplicate a cache_entry. Allocate memory for the new entry from a * memory_pool. Takes into account cache_entry fields that are meant -- cgit v1.2.3