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

[Junio C Hamano <gitster@pobox.com>]

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.

> -	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;
>  }