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@xxxxxxxxx> > > > > > > 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
