Re: [PATCH 1/2] read-cache: free threaded memory pool

[Patrick Steinhardt <ps@pks.im>]

On Tue, Oct 01, 2024 at 09:20:01AM -0400, Derrick Stolee wrote:
> On 9/30/24 8:32 AM, Patrick Steinhardt wrote:
> > On Mon, Sep 30, 2024 at 11:40:23AM +0000, Derrick Stolee via GitGitGadget wrote:
> > > From: Derrick Stolee <stolee@gmail.com>
> > > 
> > > In load_cache_entries_threaded(), each thread is allocated its own
> > 
> > s/allocated/allocating/
> 
> You're right that the wording is awkward but I'm not thrilled with the
> suggested alternative.
> 
> Perhaps "each thread allocates its own"

Sure, works for me :)

> > > memory pool. This pool needs to be cleaned up while closing the threads
> > > down, or it will be leaked.
> 
> > Okay. We move over the contents of the pool, but forgot to free the pool
> > itself. As far as I can see the pool is always allocated and only used
> > in two functions, both of which assume that it is allocated. So I wonder
> > why it is allocated in the first place instead of making it a direct
> > member of `struct load_cache_entries_thread_data`.
> 
> I took a look at what it would take to replace the pointer with an inline
> struct but found complications with situations such as the find_mem_pool()
> method. While we could replace some of the logic to recognize the new
> type, the existing logic seems to depend on using the NULL pointer as an
> indicator that the pool should be lazily initialized.
> 
> If we were to pull the struct inline, we would either need another boolean
> to indicate initialization or lose lazy initialization.
> 
> I'm leaning towards the simpler leak fix over the disruption of that
> change.

Fair enough, no complaint from my side. I thought it would've been easy,
but didn't dive deep. So if you say it is harder than I made it out to
be with my shallow understanding I'm going to trust your judgement.
After all, the leak fix is a strict improvement by itself.

Patrick