git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: "René Scharfe" <l.s.r@web.de>,
	"SZEDER Gábor via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: me@ttaylorr.com, szeder.dev@gmail.com, jnareb@gmail.com,
	peff@peff.net, garimasigit@gmail.com,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 04/10] commit-slab: add a function to deep free entries on the slab
Date: Fri, 19 Jun 2020 08:52:13 -0400	[thread overview]
Message-ID: <b275791b-0875-75e2-7fe4-4366b7399ee8@gmail.com> (raw)
In-Reply-To: <1c982d57-a446-b655-af95-1e4aeabf30ef@web.de>

On 6/18/2020 4:59 PM, René Scharfe wrote:
> Am 05.06.20 um 15:00 schrieb SZEDER Gábor via GitGitGadget:
>> From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= <szeder.dev@gmail.com>
>>
>> clear_##slabname() frees only the memory allocated for a commit slab
>> itself, but entries in the commit slab might own additional memory
>> outside the slab that should be freed as well.  We already have (at
>> least) one such commit slab, and this patch series is about to add one
>> more.
>>
>> To free all additional memory owned by entries on the commit slab the
>> user of such a slab could iterate over all commits it knows about,
>> peek whether there is a valid entry associated with each commit, and
>> free the additional memory, if any.  Or it could rely on intimate
>> knowledge about the internals of the commit slab implementation, and
>> could itself iterate directly through all entries in the slab, and
>> free the additional memory.  Or it could just leak the additional
>> memory...
>>
>> Introduce deep_clear_##slabname() to allow releasing memory owned by
>> commit slab entries by invoking the 'void free_fn(elemtype *ptr)'
>> function specified as parameter for each entry in the slab.
> 
> Adding a new function instead of extending the existing ones makes
> sense, as this is a rare requirement.
> 
>>
>> Use it in get_shallow_commits() in 'shallow.c' to replace an
>> open-coded iteration over a commit slab's entries.
>>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>  commit-slab-decl.h |  1 +
>>  commit-slab-impl.h | 13 +++++++++++++
>>  commit-slab.h      | 10 ++++++++++
>>  shallow.c          | 14 +++++---------
>>  4 files changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/commit-slab-decl.h b/commit-slab-decl.h
>> index adc7b46c83b..286164b7e27 100644
>> --- a/commit-slab-decl.h
>> +++ b/commit-slab-decl.h
>> @@ -32,6 +32,7 @@ struct slabname {							\
>>  void init_ ##slabname## _with_stride(struct slabname *s, unsigned stride); \
>>  void init_ ##slabname(struct slabname *s);				\
>>  void clear_ ##slabname(struct slabname *s);				\
>> +void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *ptr)); \
>>  elemtype *slabname## _at_peek(struct slabname *s, const struct commit *c, int add_if_missing); \
>>  elemtype *slabname## _at(struct slabname *s, const struct commit *c);	\
>>  elemtype *slabname## _peek(struct slabname *s, const struct commit *c)
>> diff --git a/commit-slab-impl.h b/commit-slab-impl.h
>> index 5c0eb91a5d1..557738df271 100644
>> --- a/commit-slab-impl.h
>> +++ b/commit-slab-impl.h
>> @@ -38,6 +38,19 @@ scope void clear_ ##slabname(struct slabname *s)			\
>>  	FREE_AND_NULL(s->slab);						\
>>  }									\
>>  									\
>> +scope void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *)) \
>> +{									\
>> +	unsigned int i;							\
>> +	for (i = 0; i < s->slab_count; i++) {				\
>> +		unsigned int j;						\
>> +		if (!s->slab[i])					\
>> +			continue;					\
>> +		for (j = 0; j < s->slab_size; j++)			\
>> +			free_fn(&s->slab[i][j * s->stride]);		\
>> +	}								\
>> +	clear_ ##slabname(s);						> +}									\
> 
> 
> Why pass an elemtype pointer to the callback function instead of
> a plain elemtype?  Because it matches the return type of _at() and
> _peek().  Consistency, good.  Handing it a pointer allows the
> callback to pass it on to free(), though, which would be bad,
> since we do that in clear_() as well.  Hmm.
> 
>> +									\
>>  scope elemtype *slabname## _at_peek(struct slabname *s,			\
>>  						  const struct commit *c, \
>>  						  int add_if_missing)   \
>> diff --git a/commit-slab.h b/commit-slab.h
>> index 05b3f2804e7..8e72a305365 100644
>> --- a/commit-slab.h
>> +++ b/commit-slab.h
>> @@ -47,6 +47,16 @@
>>   *
>>   *   Call this function before the slab falls out of scope to avoid
>>   *   leaking memory.
>> + *
>> + * - void deep_clear_indegree(struct indegree *, void (*free_fn)(int*))
>> + *
>> + *   Empties the slab, similar to clear_indegree(), but in addition it
>> + *   calls the given 'free_fn' for each slab entry to release any
>> + *   additional memory that might be owned by the entry (but not the
>> + *   entry itself!).
>> + *   Note that 'free_fn' might be called even for entries for which no
>> + *   indegree_at() call has been made; in this case 'free_fn' is invoked
>> + *   with a pointer to a zero-initialized location.
>>   */
>>
>>  #define define_commit_slab(slabname, elemtype) \
>> diff --git a/shallow.c b/shallow.c
>> index 7fd04afed19..c4ac8a73273 100644
>> --- a/shallow.c
>> +++ b/shallow.c
>> @@ -84,6 +84,10 @@ int is_repository_shallow(struct repository *r)
>>   * supports a "valid" flag.
>>   */
>>  define_commit_slab(commit_depth, int *);
>> +static void free_depth_in_slab(int **ptr)
>> +{
>> +	FREE_AND_NULL(*ptr);
>> +}
> 
> Why FREE_AND_NULL?  The original loop below called free().  The slabs
> are all released by deep_clear_() immediately after the callbacks are
> done anyway, so what's the point in zeroing these pointers?

I think the point was that a later change was going to free
elements in the slab on a one-by-one basis while computing
the filters, to save memory overall. To be future-proof
against such a change, we need to NULL the pointers here.

Perhaps that viewpoint also answers your other comment about
"why pass the pointer?"

Thanks,
-Stolee


  reply	other threads:[~2020-06-19 12:52 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-05 13:00 [PATCH 00/10] Szeder's commit-graph cleanups Derrick Stolee via GitGitGadget
2020-06-05 13:00 ` [PATCH 01/10] tree-walk.c: don't match submodule entries for 'submod/anything' SZEDER Gábor via GitGitGadget
2020-06-05 13:00 ` [PATCH 02/10] commit-graph: fix parsing the Chunk Lookup table SZEDER Gábor via GitGitGadget
2020-06-05 13:00 ` [PATCH 03/10] commit-graph-format.txt: all multi-byte numbers are in network byte order SZEDER Gábor via GitGitGadget
2020-06-05 13:00 ` [PATCH 04/10] commit-slab: add a function to deep free entries on the slab SZEDER Gábor via GitGitGadget
2020-06-18 20:59   ` René Scharfe
2020-06-19 12:52     ` Derrick Stolee [this message]
2020-06-27 15:53   ` SZEDER Gábor
2020-06-05 13:00 ` [PATCH 05/10] diff.h: drop diff_tree_oid() & friends' return value SZEDER Gábor via GitGitGadget
2020-06-05 13:00 ` [PATCH 06/10] commit-graph: clean up #includes SZEDER Gábor via GitGitGadget
2020-06-05 13:00 ` [PATCH 07/10] commit-graph: simplify parse_commit_graph() #1 SZEDER Gábor via GitGitGadget
2020-06-05 13:00 ` [PATCH 08/10] commit-graph: simplify parse_commit_graph() #2 SZEDER Gábor via GitGitGadget
2020-06-05 13:00 ` [PATCH 09/10] commit-graph: simplify write_commit_graph_file() #1 SZEDER Gábor via GitGitGadget
2020-06-05 13:00 ` [PATCH 10/10] commit-graph: simplify write_commit_graph_file() #2 SZEDER Gábor via GitGitGadget
2020-06-08 17:39 ` [PATCH 00/10] Szeder's commit-graph cleanups Junio C Hamano
2020-06-18  1:48 ` Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b275791b-0875-75e2-7fe4-4366b7399ee8@gmail.com \
    --to=stolee@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=garimasigit@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jnareb@gmail.com \
    --cc=l.s.r@web.de \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).