Hi,
On Sat, 13 Jan 2018, Kim Gybels wrote:
> Take a hint from commit ea68b0ce9f8ce8da3e360aed3cbd6720159ffbee and use
Maybe use
ea68b0ce9f8 (hash-object: don't use mmap() for small files,
2010-02-21)
instead of the full commit name?
> read() instead of mmap() for small packed-refs files.
>
> This also fixes the problem[1] where xmmap() returns NULL for zero
> length[2], for which munmap() later fails.
>
> Alternatively, we could simply check for NULL before munmap(), or
> introduce an xmunmap() that could be used together with xmmap().
>
> [1] https://github.com/git-for-windows/git/issues/1410
> [2] Logic introduced in commit 9130ac1e1966adb9922e64f645730d0d45383495
>
> Signed-off-by: Kim Gybels <[email protected]>
> ---
> refs/packed-backend.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index dab8a85d9a..7177e5bc2f 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -455,6 +455,8 @@ static void verify_buffer_safe(struct snapshot *snapshot)
> last_line, eof - last_line);
> }
>
> +#define SMALL_FILE_SIZE (32*1024)
> +
> /*
> * Depending on `mmap_strategy`, either mmap or read the contents of
> * the `packed-refs` file into the snapshot. Return 1 if the file
> @@ -489,21 +491,21 @@ static int load_contents(struct snapshot *snapshot)
> die_errno("couldn't stat %s", snapshot->refs->path);
> size = xsize_t(st.st_size);
>
> - switch (mmap_strategy) {
> - case MMAP_NONE:
> + if (!size) {
> + snapshot->buf = NULL;
> + snapshot->eof = NULL;
> + snapshot->mmapped = 0;
> + } else if (size <= SMALL_FILE_SIZE || mmap_strategy == MMAP_NONE) {
> snapshot->buf = xmalloc(size);
> bytes_read = read_in_full(fd, snapshot->buf, size);
> if (bytes_read < 0 || bytes_read != size)
> die_errno("couldn't read %s", snapshot->refs->path);
> snapshot->eof = snapshot->buf + size;
> snapshot->mmapped = 0;
> - break;
> - case MMAP_TEMPORARY:
> - case MMAP_OK:
> + } else {
> snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd,
> 0);
> snapshot->eof = snapshot->buf + size;
> snapshot->mmapped = 1;
> - break;
> }
> close(fd);
Nicely explained, and nicely solved, for a potential extra performance
benefit ;-)
Thank you!
Dscho
