[3/7] merge-ort: add pool_alloc, pool_calloc, and pool_strndup wrappers

Commit Message

Elijah Newren July 23, 2021, 12:54 p.m. UTC

From: Elijah Newren <[e-mail]>

We need functions which will either call
    xmalloc, xcalloc, xstrndup
or
    mem_pool_alloc, mem_pool_calloc, mem_pool_strndup
depending on whether we have a non-NULL memory pool.  Add these
functions; the next commit will make use of these.

Signed-off-by: Elijah Newren <[e-mail]>
---
 merge-ort.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Eric Sunshine July 23, 2021, 10:07 p.m. UTC | #1

On Fri, Jul 23, 2021 at 8:55 AM Elijah Newren via GitGitGadget
<[e-mail]> wrote:
> We need functions which will either call
>     xmalloc, xcalloc, xstrndup
> or
>     mem_pool_alloc, mem_pool_calloc, mem_pool_strndup
> depending on whether we have a non-NULL memory pool.  Add these
> functions; the next commit will make use of these.
>
> Signed-off-by: Elijah Newren <[e-mail]>

Patch [2/7] feels somewhat incomplete without the utility functions
introduced by this patch (and, indeed, when reading [2/7], I was
wondering how you were going to deal with the potential NULL pointer).
From a review standpoint, I could easily see [2/7] and [3/7] presented
as a single patch, but it's not worth a re-roll.

> diff --git a/merge-ort.c b/merge-ort.c
> @@ -683,6 +683,30 @@ static void path_msg(struct merge_options *opt,
> +MAYBE_UNUSED
> +static void *pool_calloc(struct mem_pool *pool, size_t count, size_t size)
> +{
> +       if (!pool)
> +               return xcalloc(count, size);
> +       return mem_pool_calloc(pool, count, size);
> +}
> +
> +MAYBE_UNUSED
> +static void *pool_alloc(struct mem_pool *pool, size_t size)
> +{
> +       if (!pool)
> +               return xmalloc(size);
> +       return mem_pool_alloc(pool, size);
> +}
> +
> +MAYBE_UNUSED
> +static void *pool_strndup(struct mem_pool *pool, const char *str, size_t len)
> +{
> +       if (!pool)
> +               return xstrndup(str, len);
> +       return mem_pool_strndup(pool, str, len);
> +}
> +

Derrick Stolee July 26, 2021, 2:36 p.m. UTC | #2

On 7/23/2021 8:54 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[e-mail]>
> 
> We need functions which will either call
>     xmalloc, xcalloc, xstrndup
> or
>     mem_pool_alloc, mem_pool_calloc, mem_pool_strndup
> depending on whether we have a non-NULL memory pool.  Add these
> functions; the next commit will make use of these.

I briefly considered that this should just be the way the
mem_pool_* methods work. It does rely on the caller knowing
to free() the allocated memory when their pool is NULL, so
perhaps such a universal change might be too much. What do
you think?

Thanks,
-Stolee

Elijah Newren July 28, 2021, 10:49 p.m. UTC | #3

On Mon, Jul 26, 2021 at 8:36 AM Derrick Stolee <[e-mail]> wrote:
>
> On 7/23/2021 8:54 AM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <[e-mail]>
> >
> > We need functions which will either call
> >     xmalloc, xcalloc, xstrndup
> > or
> >     mem_pool_alloc, mem_pool_calloc, mem_pool_strndup
> > depending on whether we have a non-NULL memory pool.  Add these
> > functions; the next commit will make use of these.
>
> I briefly considered that this should just be the way the
> mem_pool_* methods work. It does rely on the caller knowing
> to free() the allocated memory when their pool is NULL, so
> perhaps such a universal change might be too much. What do
> you think?

That's interesting, but I'm worried it might be a bit much.  Do others
on the list have an opinion here?

Jeff King July 29, 2021, 3:26 p.m. UTC | #4

On Wed, Jul 28, 2021 at 04:49:18PM -0600, Elijah Newren wrote:

> On Mon, Jul 26, 2021 at 8:36 AM Derrick Stolee <[e-mail]> wrote:
> >
> > On 7/23/2021 8:54 AM, Elijah Newren via GitGitGadget wrote:
> > > From: Elijah Newren <[e-mail]>
> > >
> > > We need functions which will either call
> > >     xmalloc, xcalloc, xstrndup
> > > or
> > >     mem_pool_alloc, mem_pool_calloc, mem_pool_strndup
> > > depending on whether we have a non-NULL memory pool.  Add these
> > > functions; the next commit will make use of these.
> >
> > I briefly considered that this should just be the way the
> > mem_pool_* methods work. It does rely on the caller knowing
> > to free() the allocated memory when their pool is NULL, so
> > perhaps such a universal change might be too much. What do
> > you think?
> 
> That's interesting, but I'm worried it might be a bit much.  Do others
> on the list have an opinion here?

FWIW, I had the same thought. You can also provide a helper to make the
freeing side nicer:

  static void mem_pool_free(struct mem_pool *m, void *ptr)
  {
	if (m)
		return; /* will be freed when pool frees */
	free(ptr);
  }

We do something similar with unuse_commit_buffer(), where the caller
isn't aware of we pulled the buffer from cache or allocated it
especially for them.

-Peff

Elijah Newren July 30, 2021, 2:27 a.m. UTC | #5

On Thu, Jul 29, 2021 at 9:26 AM Jeff King <[e-mail]> wrote:
>
> On Wed, Jul 28, 2021 at 04:49:18PM -0600, Elijah Newren wrote:
>
> > On Mon, Jul 26, 2021 at 8:36 AM Derrick Stolee <[e-mail]> wrote:
> > >
> > > On 7/23/2021 8:54 AM, Elijah Newren via GitGitGadget wrote:
> > > > From: Elijah Newren <[e-mail]>
> > > >
> > > > We need functions which will either call
> > > >     xmalloc, xcalloc, xstrndup
> > > > or
> > > >     mem_pool_alloc, mem_pool_calloc, mem_pool_strndup
> > > > depending on whether we have a non-NULL memory pool.  Add these
> > > > functions; the next commit will make use of these.
> > >
> > > I briefly considered that this should just be the way the
> > > mem_pool_* methods work. It does rely on the caller knowing
> > > to free() the allocated memory when their pool is NULL, so
> > > perhaps such a universal change might be too much. What do
> > > you think?
> >
> > That's interesting, but I'm worried it might be a bit much.  Do others
> > on the list have an opinion here?
>
> FWIW, I had the same thought. You can also provide a helper to make the
> freeing side nicer:
>
>   static void mem_pool_free(struct mem_pool *m, void *ptr)
>   {
>         if (m)
>                 return; /* will be freed when pool frees */
>         free(ptr);
>   }
>
> We do something similar with unuse_commit_buffer(), where the caller
> isn't aware of we pulled the buffer from cache or allocated it
> especially for them.

Having a paired function may help one side, but I worry that the name
(mem_pool_free) might introduce some confusion of its own -- "Why is
there a mem_pool_free() function, isn't the point of memory pools to
not need to individually free things?"  Or, "Why are they freeing the
pool here and what's the extra parameter?"

I'm not sure I see the right way to address that, so I think I'm going
to leave this part out of my series and let someone else add such
changes on top if they feel motivated to do so.

Jeff King July 30, 2021, 4:12 p.m. UTC | #6

On Thu, Jul 29, 2021 at 08:27:51PM -0600, Elijah Newren wrote:

> > FWIW, I had the same thought. You can also provide a helper to make the
> > freeing side nicer:
> >
> >   static void mem_pool_free(struct mem_pool *m, void *ptr)
> >   {
> >         if (m)
> >                 return; /* will be freed when pool frees */
> >         free(ptr);
> >   }
> >
> > We do something similar with unuse_commit_buffer(), where the caller
> > isn't aware of we pulled the buffer from cache or allocated it
> > especially for them.
> 
> Having a paired function may help one side, but I worry that the name
> (mem_pool_free) might introduce some confusion of its own -- "Why is
> there a mem_pool_free() function, isn't the point of memory pools to
> not need to individually free things?"  Or, "Why are they freeing the
> pool here and what's the extra parameter?"

Yeah, "mem_pool_maybe_free" or something might explain it. But...

> I'm not sure I see the right way to address that, so I think I'm going
> to leave this part out of my series and let someone else add such
> changes on top if they feel motivated to do so.

That's fine, especially as dropping the conditiona USE_MEMORY_POOL flag
means these functions will go away entirely.

-Peff

Patch

diff --git a/merge-ort.c b/merge-ort.c
index cb33c76760f..2bca4b71f2a 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -683,6 +683,30 @@  static void path_msg(struct merge_options *opt,
 	strbuf_addch(sb, '\n');
 }
 
+MAYBE_UNUSED
+static void *pool_calloc(struct mem_pool *pool, size_t count, size_t size)
+{
+	if (!pool)
+		return xcalloc(count, size);
+	return mem_pool_calloc(pool, count, size);
+}
+
+MAYBE_UNUSED
+static void *pool_alloc(struct mem_pool *pool, size_t size)
+{
+	if (!pool)
+		return xmalloc(size);
+	return mem_pool_alloc(pool, size);
+}
+
+MAYBE_UNUSED
+static void *pool_strndup(struct mem_pool *pool, const char *str, size_t len)
+{
+	if (!pool)
+		return xstrndup(str, len);
+	return mem_pool_strndup(pool, str, len);
+}
+
 /* add a string to a strbuf, but converting "/" to "_" */
 static void add_flattened_path(struct strbuf *out, const char *s)
 {