git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v1 2/5] Add an API creating / discarding cache_entry structs

From
Ben Peart <peartben@gmail.com>
Date
Apr 17, 2018, 23:11 UTC
Message-ID
<921a8d5c-b118-c5b1-35dd-459130faeed9@gmail.com>
In-Reply-To
<20180417163400.3875-4-jamill@microsoft.com>
On 4/17/2018 12:34 PM, Jameson Miller wrote:
Show 14 quoted lines
> Add an API around managing the lifetime of cache_entry structs. Abstracting
> memory management details behind an API will allow for alternative memory
> management strategies without affecting all the call sites.  This commit does
> not change how memory is allocated / freed. A later commit in this series will
> allocate cache entries from memory pools as appropriate.
> 
> Motivation:
> 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 when loading an index.
> 
> This API makes a distinction between cache entries that are intended for use
> with a particular to an index and cache entries that are not.

The wording here is awkward. Did you mean "intended for use with a particular index?"

This enables us
Show 35 quoted lines
> to use the knowledge about how a cache entry will be used to make informed
> decisions about how to handle the corresponding memory.
> ---
>   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                |  29 +++++++++++++-
>   merge-recursive.c      |   2 +-
>   read-cache.c           | 105 +++++++++++++++++++++++++++++++++++--------------
>   resolve-undo.c         |   6 ++-
>   split-index.c          |   8 ++--
>   tree.c                 |   4 +-
>   unpack-trees.c         |  27 ++++++++-----
>   13 files changed, 166 insertions(+), 94 deletions(-)
> 
> diff --git a/apply.c b/apply.c
> index 7e5792c996..47903f427b 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4090,12 +4090,12 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)
>   			return error(_("sha1 information is lacking or useless "
>   				       "(%s)."), name);
>   
> -		ce = make_cache_entry(patch->old_mode, oid.hash, name, 0, 0);
> +		ce = make_index_cache_entry(&result, patch->old_mode, oid.hash, name, 0, 0);
>   		if (!ce)
> -			return error(_("make_cache_entry failed for path '%s'"),
> +			return error(_("make_index_cache_entry failed for path '%s'"),
>   				     name);
>   		if (add_index_entry(&result, ce, ADD_CACHE_OK_TO_ADD)) {
> -			free(ce);
> +			index_cache_entry_discard(ce);

I personally prefer name symmetry. To me, make_index_cache_entry() should be paired with discard_index_cache_entry().

The rest of this patch looks like a fairly mechanical refactoring with the biggest exception being the difference between the the *_index_cache_entry() APIs and the *_transient_cache_entry() APIs.

There are quite a few changes but I didn't see any instances that were missed or any errors. I see that later patches will put verification code in place to detect if any were done incorrectly and to prevent regressions moving forward.

Overall, it looks correct and reasonable.
<snip>
Show 64 quoted lines
>   
> -struct cache_entry *make_cache_entry(unsigned int mode,
> -		const unsigned char *sha1, const char *path, int stage,
> -		unsigned int refresh_options)
> +struct cache_entry *make_empty_index_cache_entry(struct index_state *istate, size_t len)
> +{
> +	return xcalloc(1, cache_entry_size(len));
> +}
> +
> +struct cache_entry *make_empty_transient_cache_entry(size_t len)
> +{
> +	return xcalloc(1, cache_entry_size(len));
> +}
> +
> +struct cache_entry *make_index_cache_entry(struct index_state *istate, unsigned int mode,
> +			    const unsigned char *sha1, const char *path,
> +			    int stage, unsigned int refresh_options)
>   {
> -	int size, len;
>   	struct cache_entry *ce, *ret;
> +	int len;
>   
>   	if (!verify_path(path)) {
>   		error("Invalid path '%s'", path);
> @@ -758,8 +767,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,
>   	}
>   
>   	len = strlen(path);
> -	size = cache_entry_size(len);
> -	ce = xcalloc(1, size);
> +	ce = make_empty_index_cache_entry(istate, len);
>   
>   	hashcpy(ce->oid.hash, sha1);
>   	memcpy(ce->name, path, len);
> @@ -769,10 +777,34 @@ struct cache_entry *make_cache_entry(unsigned int mode,
>   
>   	ret = refresh_cache_entry(&the_index, ce, refresh_options);
>   	if (ret != ce)
> -		free(ce);
> +		index_cache_entry_discard(ce);
> +
>   	return ret;
>   }
>   
> +struct cache_entry *make_transient_cache_entry(unsigned int mode, const unsigned char *sha1,
> +			   const char *path, int stage)
> +{
> +	struct cache_entry *ce;
> +	int len;
> +
> +	if (!verify_path(path)) {
> +		error("Invalid path '%s'", path);
> +		return NULL;
> +	}
> +
> +	len = strlen(path);
> +	ce = make_empty_transient_cache_entry(len);
> +
> +	hashcpy(ce->oid.hash, sha1);
> +	memcpy(ce->name, path, len);
> +	ce->ce_flags = create_ce_flags(stage);
> +	ce->ce_namelen = len;
> +	ce->ce_mode = create_ce_mode(mode);
> +

Nit, feel free to ignore. There isn't a lot of initialization here but I wonder if it makes sense to have an internal helper function to ensure these stay the same.

Show 50 quoted lines
> +	return ce;
> +}
> +
>   /*
>    * Chmod an index entry with either +x or -x.
>    *
> @@ -1243,7 +1275,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
>   {
>   	struct stat st;
>   	struct cache_entry *updated;
> -	int changed, size;
> +	int changed;
>   	int refresh = options & CE_MATCH_REFRESH;
>   	int ignore_valid = options & CE_MATCH_IGNORE_VALID;
>   	int ignore_skip_worktree = options & CE_MATCH_IGNORE_SKIP_WORKTREE;
> @@ -1323,8 +1355,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
>   		return NULL;
>   	}
>   
> -	size = ce_size(ce);
> -	updated = xmalloc(size);
> +	updated = make_empty_index_cache_entry(istate, ce_namelen(ce));
>   	copy_cache_entry(updated, ce);
>   	memcpy(updated->name, ce->name, ce->ce_namelen + 1);
>   	fill_stat_cache_info(updated, &st);
> @@ -1610,12 +1641,13 @@ int read_index(struct index_state *istate)
>   	return read_index_from(istate, get_index_file(), get_git_dir());
>   }
>   
> -static struct cache_entry *cache_entry_from_ondisk(struct ondisk_cache_entry *ondisk,
> +static struct cache_entry *cache_entry_from_ondisk(struct index_state *istate,
> +						   struct ondisk_cache_entry *ondisk,
>   						   unsigned int flags,
>   						   const char *name,
>   						   size_t len)
>   {
> -	struct cache_entry *ce = xmalloc(cache_entry_size(len));
> +	struct cache_entry *ce = make_empty_index_cache_entry(istate, len);
>   
>   	ce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);
>   	ce->ce_stat_data.sd_mtime.sec = get_be32(&ondisk->mtime.sec);
> @@ -1657,7 +1689,8 @@ static unsigned long expand_name_field(struct strbuf *name, const char *cp_)
>   	return (const char *)ep + 1 - cp_;
>   }
>   
> -static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,
> +static struct cache_entry *create_from_disk(struct index_state *istate,
> +					    struct ondisk_cache_entry *ondisk,
>   					    unsigned long *ent_size,
>   					    struct strbuf *previous_name)

Nit. Just wondering why you pulled refresh_cache_entry() out into an earlier/separate commit but then did create_from_disk() as part of the large refactoring focused on adding the create/discard APIs?

<snip>
Show 10 quoted lines
> diff --git a/unpack-trees.c b/unpack-trees.c
> index e73745051e..232cdecc72 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -192,10 +192,10 @@ static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>   			       ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
>   }
>   
> -static struct cache_entry *dup_entry(const struct cache_entry *ce)
> +static struct cache_entry *dup_entry(const struct cache_entry *ce, struct index_state *istate)
Ditto with the dup_entry() and create_ce_entry() functions refactoring.
Show 6 quoted lines
> -static struct cache_entry *create_ce_entry(const struct traverse_info *info, const struct name_entry *n, int stage)
> +static struct cache_entry *create_ce_entry(const struct traverse_info *info,
> +	const struct name_entry *n,
> +	int stage,
> +	struct index_state *istate,
> +	int is_transient)
Previous: Jameson MillerNext: Jameson Miller
Message 4 of 100 in “Allocate cache entries from memory pool”
  1. 0/5 Allocate cache entries from memory poolJameson Miller, Apr 17, 2018
  2. 0/5 Allocate cache entries from memory poolJameson Miller, Apr 17, 2018
  3. 2/5 Add an API creating / discarding cache_entry structsJameson Miller, Apr 17, 2018
  4. Ben PeartApr 17, 2018
  5. 1/5 read-cache: teach refresh_cache_entry to take istateJameson Miller, Apr 17, 2018
  6. Ben PeartApr 17, 2018
  7. 3/5 mem-pool: fill out functionalityJameson Miller, Apr 17, 2018
  8. Jonathan TanApr 20, 2018
  9. Jameson MillerApr 23, 2018
  10. Jonathan TanApr 23, 2018
  11. Jameson MillerApr 23, 2018
  12. 5/5 Add optional memory validations around cache_entry lifecyleJameson Miller, Apr 17, 2018
  13. 4/5 Allocate cache entries from memory poolsJameson Miller, Apr 17, 2018
  14. Ben PeartApr 17, 2018
  15. Jameson MillerApr 23, 2018
  16. Junio C HamanoApr 18, 2018
  17. Stefan BellerApr 20, 2018
  18. Jameson MillerApr 23, 2018
  19. Stefan BellerApr 23, 2018
  20. Jameson MillerApr 23, 2018
  21. Jonathan TanApr 20, 2018
  22. Jameson MillerApr 23, 2018
  23. 0/5 Allocate cache entries from memory poolJameson Miller, Apr 30, 2018
  24. 1/5 read-cache: teach refresh_cache_entry() to take istateJameson Miller, Apr 30, 2018
  25. 3/5 mem-pool: fill out functionalityJameson Miller, Apr 30, 2018
  26. Stefan BellerApr 30, 2018
  27. Jameson MillerMay 1, 2018
  28. Duy NguyenMay 3, 2018
  29. 5/5 block alloc: add validations around cache_entry lifecyleJameson Miller, Apr 30, 2018
  30. Duy NguyenMay 3, 2018
  31. 2/5 block alloc: add lifecycle APIs for cache_entry structsJameson Miller, Apr 30, 2018
  32. 4/5 block alloc: allocate cache entries from mem_poolJameson Miller, Apr 30, 2018
  33. Duy NguyenMay 3, 2018
  34. Stefan BellerMay 3, 2018
  35. Duy NguyenMay 3, 2018
  36. Stefan BellerMay 3, 2018
  37. Jameson MillerMay 3, 2018
  38. alloc.c: replace alloc by mempoolStefan Beller, May 3, 2018
  39. Duy NguyenMay 4, 2018
  40. Junio C HamanoMay 8, 2018
  41. Stefan BellerMay 8, 2018
  42. Junio C HamanoMay 8, 2018
  43. 0/7 allocate cache entries from memory poolJameson Miller, May 23, 2018
  44. 1/7 read-cache: teach refresh_cache_entry() to take istateJameson Miller, May 23, 2018
  45. Stefan BellerMay 25, 2018
  46. 3/7 mem-pool: only search head block for available spaceJameson Miller, May 23, 2018
  47. 2/7 block alloc: add lifecycle APIs for cache_entry structsJameson Miller, May 23, 2018
  48. Junio C HamanoMay 24, 2018
  49. Jameson MillerMay 24, 2018
  50. 4/7 mem-pool: add lifecycle management functionsJameson Miller, May 23, 2018
  51. 5/7 mem-pool: fill out functionalityJameson Miller, May 23, 2018
  52. Stefan BellerJun 1, 2018
  53. 6/7 block alloc: allocate cache entries from mem_poolJameson Miller, May 23, 2018
  54. 7/7 block alloc: add validations around cache_entry lifecyleJameson Miller, May 23, 2018
  55. Junio C HamanoMay 24, 2018
  56. Jameson MillerMay 24, 2018
  57. Stefan BellerMay 25, 2018
  58. Jameson MillerJun 20, 2018
  59. Stefan BellerMay 25, 2018
  60. 0/8 Allocate cache entries from mem_poolJameson Miller, Jun 20, 2018
  61. 1/8 read-cache: teach refresh_cache_entry() to take istateJameson Miller, Jun 20, 2018
  62. 3/8 mem-pool: only search head block for available spaceJameson Miller, Jun 20, 2018
  63. Stefan BellerJun 21, 2018
  64. Jameson MillerJun 28, 2018
  65. 5/8 mem-pool: add lifecycle management functionsJameson Miller, Jun 20, 2018
  66. 8/8 block alloc: add validations around cache_entry lifecyleJameson Miller, Jun 20, 2018
  67. 7/8 block alloc: allocate cache entries from mem_poolJameson Miller, Jun 20, 2018
  68. 4/8 mem-pool: tweak math on mp_block allocation sizeJameson Miller, Jun 20, 2018
  69. 2/8 block alloc: add lifecycle APIs for cache_entry structsJameson Miller, Jun 20, 2018
  70. Stefan BellerJun 21, 2018
  71. Jameson MillerJun 28, 2018
  72. 6/8 mem-pool: fill out functionalityJameson Miller, Jun 20, 2018
  73. 0/8 Allocate cache entries from mem_poolJameson Miller, Jun 28, 2018
  74. 2/8 read-cache: make_cache_entry should take object_id structJameson Miller, Jun 28, 2018
  75. Junio C HamanoJun 28, 2018
  76. SZEDER GáborJun 28, 2018
  77. 3/8 block alloc: add lifecycle APIs for cache_entry structsJameson Miller, Jun 28, 2018
  78. Junio C HamanoJun 28, 2018
  79. SZEDER GáborJun 28, 2018
  80. 5/8 mem-pool: add life cycle management functionsJameson Miller, Jun 28, 2018
  81. Junio C HamanoJun 28, 2018
  82. 4/8 mem-pool: only search head block for available spaceJameson Miller, Jun 28, 2018
  83. 6/8 mem-pool: fill out functionalityJameson Miller, Jun 28, 2018
  84. Junio C HamanoJun 28, 2018
  85. Jameson MillerJul 2, 2018
  86. 8/8 block alloc: add validations around cache_entry lifecyleJameson Miller, Jun 28, 2018
  87. 1/8 read-cache: teach refresh_cache_entry() to take istateJameson Miller, Jun 28, 2018
  88. 7/8 block alloc: allocate cache entries from mem-poolJameson Miller, Jun 28, 2018
  89. 0/8 Allocate cache entries from mem_poolJameson Miller, Jul 2, 2018
  90. 2/8 read-cache: teach make_cache_entry to take object_idJameson Miller, Jul 2, 2018
  91. Stefan BellerJul 2, 2018
  92. Jameson MillerJul 5, 2018
  93. 3/8 block alloc: add lifecycle APIs for cache_entry structsJameson Miller, Jul 2, 2018
  94. Duy NguyenJul 22, 2018
  95. 4/8 mem-pool: only search head block for available spaceJameson Miller, Jul 2, 2018
  96. 5/8 mem-pool: add life cycle management functionsJameson Miller, Jul 2, 2018
  97. 7/8 block alloc: allocate cache entries from mem_poolJameson Miller, Jul 2, 2018
  98. 8/8 block alloc: add validations around cache_entry lifecyleJameson Miller, Jul 2, 2018
  99. 6/8 mem-pool: fill out functionalityJameson Miller, Jul 2, 2018
  100. 1/8 read-cache: teach refresh_cache_entry to take istateJameson Miller, Jul 2, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.