Re: [PATCH 4/4] packed-backend: use mmap when opening large "packed-refs" file

From: shejialuo <shejialuo@gmail.com>
Date: May 7, 2025, 12:34 UTC
Message-ID: <aBtTXIpCPlQJtVqr@ArchLinux>
In-Reply-To: <xmqqecx1k1ig.fsf@gitster.g>

On Tue, May 06, 2025 at 12:00:39PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> shejialuo <shejialuo@gmail.com> writes:
> 
> > We use "strbuf_read" to read the content of "packed-refs". However, this
> > is a bad practice which would consume a lot of memory usage if there are
> > multiple processes reading large "packed-refs".
> 
> Neither this nor the commit title says that the issue is limited to
> the code path that runs fsck on packed-refs file, but I thought the
> code paths to use packed-refs to resolve refs correctly uses mmap()
> and does not share this issue?  If it is limited to one single code
> path, please mention it explicitly.
>

Nice advice, I will improve this in the next version.

Show 5 quoted lines

> Also, I think it was already pointed out that "multiple processes"
> is not all that interesting issue.  Even if there is a single
> process using a single large packed-refs file, alloc+read gives the
> system more memory pressure than the read-only mmap like we do.
>

That's right. Even there is only one process, we could avoid copying the file content from kernel space to the user space. I will improve the commit message.

Show 7 quoted lines

> As to the title
> 
> 	packed-backend: use mmap when opening large "packed-refs" file
> 	packed-backend: mmap large "packed-refs" file during fsck
> 
> would be shorter and clearer.
>

Yes, the second one is better. Will improve this.