Re: [PATCH v1 0/5] Allocate cache entries from memory pool

[Junio C Hamano <gitster@pobox.com>]

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.

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.

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.