diff mbox series

[04/34] commit-slab: add a function to deep free entries on the slab

Message ID 20200529085038.26008-5-szeder.dev@gmail.com (mailing list archive)
State New, archived
Headers show
Series An alternative modified path Bloom filters implementation | expand

Commit Message

SZEDER Gábor May 29, 2020, 8:50 a.m. UTC
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.

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>
---
 commit-slab-decl.h |  1 +
 commit-slab-impl.h | 13 +++++++++++++
 commit-slab.h      | 10 ++++++++++
 shallow.c          | 14 +++++---------
 4 files changed, 29 insertions(+), 9 deletions(-)

Comments

Derrick Stolee June 4, 2020, 4:43 p.m. UTC | #1
On 5/29/2020 4:50 AM, SZEDER Gábor wrote:
> +void free_depth_in_slab(int **ptr)

This needs to be "static" to compile with DEVELOPER=1.

(I'm working to apply your patches on top of ds/line-log-on-bloom
so we can get the benefit of these universally-good things. I just
want to report the things that I stumble on.)

Thanks,
-Stolee
SZEDER Gábor June 27, 2020, 3:56 p.m. UTC | #2
On Thu, Jun 04, 2020 at 12:43:32PM -0400, Derrick Stolee wrote:
> On 5/29/2020 4:50 AM, SZEDER Gábor wrote:
> > +void free_depth_in_slab(int **ptr)
> 
> This needs to be "static" to compile with DEVELOPER=1.

You are right, of course, but...

Do you get an error because of this when building with DEVELOPER=1?  I
don't get an error when building with DEVELOPER=1...

> (I'm working to apply your patches on top of ds/line-log-on-bloom
> so we can get the benefit of these universally-good things. I just
> want to report the things that I stumble on.)

Thanks.
Derrick Stolee June 29, 2020, 11:30 a.m. UTC | #3
On 6/27/2020 11:56 AM, SZEDER Gábor wrote:
> On Thu, Jun 04, 2020 at 12:43:32PM -0400, Derrick Stolee wrote:
>> On 5/29/2020 4:50 AM, SZEDER Gábor wrote:
>>> +void free_depth_in_slab(int **ptr)
>>
>> This needs to be "static" to compile with DEVELOPER=1.
> 
> You are right, of course, but...
> 
> Do you get an error because of this when building with DEVELOPER=1?  I
> don't get an error when building with DEVELOPER=1...

Yes, I believe that's how I discovered it.

>> (I'm working to apply your patches on top of ds/line-log-on-bloom
>> so we can get the benefit of these universally-good things. I just
>> want to report the things that I stumble on.)
> 
> Thanks.

Sure thing.
-Stolee
diff mbox series

Patch

diff --git a/commit-slab-decl.h b/commit-slab-decl.h
index adc7b46c83..286164b7e2 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 5c0eb91a5d..557738df27 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);						\
+}									\
+									\
 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 69bf0c807c..7bd472171e 100644
--- a/commit-slab.h
+++ b/commit-slab.h
@@ -42,6 +42,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 7fd04afed1..d02ba494ee 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 *);
+void free_depth_in_slab(int **ptr)
+{
+	FREE_AND_NULL(*ptr);
+}
 struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 		int shallow_flag, int not_shallow_flag)
 {
@@ -150,15 +154,7 @@  struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 			}
 		}
 	}
-	for (i = 0; i < depths.slab_count; i++) {
-		int j;
-
-		if (!depths.slab[i])
-			continue;
-		for (j = 0; j < depths.slab_size; j++)
-			free(depths.slab[i][j]);
-	}
-	clear_commit_depth(&depths);
+	deep_clear_commit_depth(&depths, free_depth_in_slab);
 
 	return result;
 }