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

From: Derrick Stolee <stolee@gmail.com>
Date: Oct 1, 2024, 13:20 UTC
Message-ID: <8ebebcd5-4ddb-4c1e-9bf8-4e9a79944e2b@gmail.com>
In-Reply-To: <ZvqaTqegJxow1x-b@pks.im>

On 9/30/24 8:32 AM, Patrick Steinhardt wrote:

Show 6 quoted lines

> 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"

>> memory pool. This pool needs to be cleaned up while closing the threads
>> down, or it will be leaked.

Show 5 quoted lines

> 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.

Thanks, -Stolee