Re: [PATCH v1] mem_pool: add GIT_TRACE_MEMPOOL support

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Ben Peart <peartben@gmail.com>
Cc: git@vger.kernel.org, benpeart@microsoft.com
Subject: Re: [PATCH v1] mem_pool: add GIT_TRACE_MEMPOOL support
Date: Thu, 29 Nov 2018 16:37:50 +0100 (STD)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1811291637330.41@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <9ce2df67-d698-0372-4770-32659668ab7e@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2609 bytes --]

Hi Ben,

On Thu, 29 Nov 2018, Ben Peart wrote:

> On 11/28/2018 4:37 AM, Johannes Schindelin wrote:
> > Hi Ben,
> >
> > On Tue, 27 Nov 2018, Ben Peart wrote:
> >
> > > From: Ben Peart <benpeart@microsoft.com>
> > >
> > > Add tracing around initializing and discarding mempools. In discard report
> > > on the amount of memory unused in the current block to help tune setting
> > > the initial_size.
> > >
> > > Signed-off-by: Ben Peart <benpeart@microsoft.com>
> > > ---
> > Looks good.
> >
> > My only question: should we also trace calls to _alloc(), _calloc() and
> > _combine()?
> 
> I was trying to tune the initial size in my use of the mem_pool and so found
> this tracing useful to see how much memory was actually being used.  I'm
> inclined to only add tracing as it is needed rather that proactively because
> we think it _might_ be needed.  I suspect _alloc() and _calloc() would get
> very noisy and not add much value.

In other words, YAGNI. Makes sense.

Thanks,
Johannes

> 
> >
> > Ciao,
> > Johannes
> >
> > > Notes:
> > >      Base Ref: * git-trace-mempool
> > >      Web-Diff: https://github.com/benpeart/git/commit/9ac84bbca2
> > >      Checkout: git fetch https://github.com/benpeart/git
> > >      git-trace-mempool-v1 && git checkout 9ac84bbca2
> > >
> > >   mem-pool.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > >
> > > diff --git a/mem-pool.c b/mem-pool.c
> > > index a2841a4a9a..065389aaec 100644
> > > --- a/mem-pool.c
> > > +++ b/mem-pool.c
> > > @@ -5,6 +5,7 @@
> > >   #include "cache.h"
> > >   #include "mem-pool.h"
> > >   
> > > +static struct trace_key trace_mem_pool = TRACE_KEY_INIT(MEMPOOL);
> > >   #define BLOCK_GROWTH_SIZE 1024*1024 - sizeof(struct mp_block);
> > >   
> > >   /*
> > > @@ -48,12 +49,16 @@ void mem_pool_init(struct mem_pool **mem_pool, size_t
> > > initial_size)
> > >     mem_pool_alloc_block(pool, initial_size, NULL);
> > >   
> > >   	*mem_pool = pool;
> > > +	trace_printf_key(&trace_mem_pool, "mem_pool (%p): init (%"PRIuMAX")
> > > initial size\n",
> > > +		pool, (uintmax_t)initial_size);
> > >   }
> > >   
> > >   void mem_pool_discard(struct mem_pool *mem_pool, int invalidate_memory)
> > >   {
> > >    struct mp_block *block, *block_to_free;
> > >   +	trace_printf_key(&trace_mem_pool, "mem_pool (%p): discard (%"PRIuMAX")
> > > unused\n",
> > > +		mem_pool, (uintmax_t)(mem_pool->mp_block->end -
> > > mem_pool->mp_block->next_free));
> > >    block = mem_pool->mp_block;
> > >    while (block)
> > >    {
> > >
> > > base-commit: bb75be6cb916297f271c846f2f9caa3daaaec718
> > > -- 
> > > 2.18.0.windows.1
> > >
> > >
> 
> 