Re: [PATCH] packed_ref_cache: don't use mmap() for small files

[Johannes Schindelin]

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 <kgyb...@infogroep.be>
> ---
>  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