Re: [PATCH 4/4] packed-backend: use mmap when opening large "packed-refs" file
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 6, 2025, 19:00 UTC
- Message-ID
- <xmqqecx1k1ig.fsf@gitster.g>
- In-Reply-To
- <aBo7tOkheM6zOJpe@ArchLinux>
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.
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.
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.
The patch looks OK. Nice to see this one-off strbuf use going away.
Show 34 quoted lines
> - struct strbuf packed_ref_content = STRBUF_INIT;
> + struct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));
> unsigned int sorted = 0;
> struct stat st;
> int ret = 0;
> @@ -2121,21 +2121,21 @@ static int packed_fsck(struct ref_store *ref_store,
> if (!st.st_size)
> goto cleanup;
>
> - if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
> - ret = error_errno(_("unable to read '%s'"), refs->path);
> + if (!allocate_snapshot_buffer(snapshot, fd, &st))
> goto cleanup;
> - }
> + munmap_snapshot_if_temporary(snapshot);
>
> - ret = packed_fsck_ref_content(o, ref_store, &sorted, packed_ref_content.buf,
> - packed_ref_content.buf + packed_ref_content.len);
> + ret = packed_fsck_ref_content(o, ref_store, &sorted, snapshot->start,
> + snapshot->eof);
> if (!ret && sorted)
> - ret = packed_fsck_ref_sorted(o, ref_store, packed_ref_content.buf,
> - packed_ref_content.buf + packed_ref_content.len);
> + ret = packed_fsck_ref_sorted(o, ref_store, snapshot->start,
> + snapshot->eof);
>
> cleanup:
> if (fd >= 0)
> close(fd);
> - strbuf_release(&packed_ref_content);
> + clear_snapshot_buffer(snapshot);
> + free(snapshot);
> return ret;
> }