Re: [PATCH v1 0/5] Allocate cache entries from memory pool
- From
- Jameson Miller <jameson.miller81@gmail.com>
- Date
- Apr 23, 2018, 16:19 UTC
- Message-ID
- <cb1c1c86-48f7-5cf6-16ff-0d54550fe958@gmail.com>
- In-Reply-To
- <xmqqwox5i0f7.fsf@gitster-ct.c.googlers.com>
On 04/18/2018 12:49 AM, Junio C Hamano wrote:
Show 36 quoted lines
> Jameson Miller <jamill@microsoft.com> writes: > >> This patch series improves the performance of loading indexes by >> reducing the number of malloc() calls. ... >> >> Jameson Miller (5): >> read-cache: teach refresh_cache_entry to take istate >> Add an API creating / discarding cache_entry structs >> mem-pool: fill out functionality >> Allocate cache entries from memory pools >> Add optional memory validations around cache_entry lifecyle >> >> apply.c | 26 +++--- >> blame.c | 5 +- >> builtin/checkout.c | 8 +- >> builtin/difftool.c | 8 +- >> builtin/reset.c | 6 +- >> builtin/update-index.c | 26 +++--- >> cache.h | 40 ++++++++- >> git.c | 3 + >> mem-pool.c | 136 ++++++++++++++++++++++++++++- >> mem-pool.h | 34 ++++++++ >> merge-recursive.c | 4 +- >> read-cache.c | 229 +++++++++++++++++++++++++++++++++++++++---------- >> resolve-undo.c | 6 +- >> split-index.c | 31 +++++-- >> tree.c | 4 +- >> unpack-trees.c | 27 +++--- >> 16 files changed, 476 insertions(+), 117 deletions(-) >> >> >> base-commit: cafaccae98f749ebf33495aec42ea25060de8682 > I couldn't quite figure out what these five patches were based on, > even with this line. Basing on and referring to a commit that is > not part of our published history with "base-commit" is not all that > useful to others.
My apologies - this patch series should be applied to the 'next' branch. It applies cleanly on top of b46fe60e1d ("merge-fix/ps/test-chmtime-get", 2018-04-20), which is a commit in the 'next' branch.
Show 18 quoted lines
> Offhand without applying these patches and viewing the changes in > wider contexts, one thing that makes me wonder is how the two > allocation schemes can be used with two implementations of free(). > Upon add_index_entry() that replaces an index entry for an existing > path, we'd discard an entry that was originally read as part of > read_cache(). If we do that again, the second add_index_entry() > will be discading, in its call to replace_index_entry(), the entry > that was allocated by the caller of the first add_index_entry() > call. And replace_index_entry() would not have a way to know from > which allocation the entry's memory came from. > > Perhaps it is not how you are using the "transient" stuff, and from > the comment in 2/5, it is for "entries that are not meant to go into > the index", but then higher stage index entries in unpack_trees seem > to be allocated via the "transient" stuff, so I am not sure what the > plans are for things like merge_recursive() that uses unpack_trees() > to prepare the index and then later "fix it up" by further futzing > around the index to adjust for renames it finds, etc.
Good points. The intention with this logic is that any entries that *could* go into an index are allocated from the memory pool. The "transient" entries only exist for a short period of time. These have a defined lifetime and we can always trace the corresponding "free" call. make_transient_cache_entry() is only used to construct a temporary cache_entry to pass to the checkout_entry() / write_entry(). There is a note in checkout.c indicating that write_entry() needs to be re-factored to not take a cache_entry.
The cache_entry type could gain knowledge about where it was allocated from. This would allow us to only have a single "free()" function, which could inspect the cache_entry to see if it was allocated from a mem_pool (and possibly which mem_pool) and take the appropriate action. The downside of this approach is that cache_entry would need to gain a field to track this information, and I *think* all of the bit field spots are taken.
Show 5 quoted lines
> Let me read it fully once we know where these patches are to be > applied, but before that I cannot say much about them X-<. > > Thanks. >