Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

read-cache: two small leak fixes #1801

Closed
wants to merge 1 commit into from
Closed

read-cache: two small leak fixes #1801

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions read-cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2188,6 +2188,7 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
if (err)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

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/

> memory pool. This pool needs to be cleaned up while closing the threads
> down, or it will be leaked.
> 
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  read-cache.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/read-cache.c b/read-cache.c
> index 764fdfec465..3c078afadbc 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2188,6 +2188,7 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
>  		if (err)
>  			die(_("unable to join load_cache_entries thread: %s"), strerror(err));
>  		mem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);
> +		free(p->ce_mem_pool);
>  		consumed += p->consumed;
>  	}

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

Patrick

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

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"

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

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

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

die(_("unable to join load_cache_entries thread: %s"), strerror(err));
mem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);
free(p->ce_mem_pool);
consumed += p->consumed;
}

Expand Down
Loading