Omar Polo <op@omarpolo.com> wrote:
> We spend a decent amount of time reading the fileindex in
> reallocarray().  The fileindex has the path stored (NUL included) with
> a final padding to the next multiple of eight bytes, so we read 8
> bytes per iteration.  That's fine.  Calling reallocarray() to increase
> the size of the allocation by 8 byte each iteration however is a
> waste.
> 
> Instead, use a local buffer and strdup() on exit.
> 
> Git paths don't have an implicit length limit, but this is the
> fileindex of a worktree and AFAIK we can't check-out files whose path
> is more than PATH_MAX anyway.
> 
> ok?

This is good! I think we should do this irrespective of whether we load
the whole file up front.

Profiling 'got info Makefile' on my T14s cuts time spent in
read_fileindex_path() by more than half: from 19.43%[0] down to 8.38%[1],
which is great! Eliminating those small, repeating 8 byte reallocs saves
the most time spent in this routine--which is the most expensive part of
read_fileindex_entry()--and for a net code reduction, so ok for me :)

Interestingly, the mmap approach cut it right down to 3.26%[2], which
was just slightly better than malloc+read's 3.68%[3]. They enable
skipping the 8 byte reads, which is where the rest of the time is spent
in this routine.

In any case, I think this makes the tree entry insertion code path the
next place to look for improvement.

[0]: https://ss.jamsek.net/profile-info-Makefile-main.png
[1]: https://ss.jamsek.net/profile-info-Makefile-PATH_MAX.png
[2]: https://ss.jamsek.net/profile-info-Makefile-mmap.png
[3]: https://ss.jamsek.net/profile-info-Makefile-malloc+read.png

> -----------------------------------------------
> commit 1772f04de18f32fe8a4b91869eaf51da7ce85167
> from: Omar Polo <op@omarpolo.com>
> date: Fri Jul 28 08:48:01 2023 UTC
>  
>  read_fileindex_path: avoid many reallocarray() calls, use local buffer
>  
> diff c2eedbd13a618c74343a8d8d645acecdfb677b8f 1772f04de18f32fe8a4b91869eaf51da7ce85167
> commit - c2eedbd13a618c74343a8d8d645acecdfb677b8f
> commit + 1772f04de18f32fe8a4b91869eaf51da7ce85167
> blob - 1575b0dd69dc7a97d0d393fa46ba56957b8ee0b7
> blob + 62f7df1b95d5d56bd0b23c83df32e7d74f3d4b00
> --- lib/fileindex.c
> +++ lib/fileindex.c
> @@ -579,38 +579,26 @@ read_fileindex_path(char **path, struct got_hash *ctx,
>  static const struct got_error *
>  read_fileindex_path(char **path, struct got_hash *ctx, FILE *infile)
>  {
> -	const struct got_error *err = NULL;
>  	const size_t chunk_size = 8;
> -	size_t n, len = 0, totlen = chunk_size;
> +	char p[PATH_MAX];
> +	size_t n, len = 0;
>  
> -	*path = malloc(totlen);
> -	if (*path == NULL)
> -		return got_error_from_errno("malloc");
> -
>  	do {
> -		if (len + chunk_size > totlen) {
> -			char *p = reallocarray(*path, totlen + chunk_size, 1);
> -			if (p == NULL) {
> -				err = got_error_from_errno("reallocarray");
> -				break;
> -			}
> -			totlen += chunk_size;
> -			*path = p;
> -		}
> -		n = fread(*path + len, 1, chunk_size, infile);
> -		if (n != chunk_size) {
> -			err = got_ferror(infile, GOT_ERR_FILEIDX_BAD);
> -			break;
> -		}
> -		got_hash_update(ctx, *path + len, chunk_size);
> +		if (len + chunk_size > sizeof(p))
> +			return got_error(GOT_ERR_FILEIDX_BAD);
> +
> +		n = fread(&p[len], 1, chunk_size, infile);
> +		if (n != chunk_size)
> +			return got_ferror(infile, GOT_ERR_FILEIDX_BAD);
> +
> +		got_hash_update(ctx, &p[len], chunk_size);
>  		len += chunk_size;
> -	} while (memchr(*path + len - chunk_size, '\0', chunk_size) == NULL);
> +	} while (memchr(&p[len - chunk_size], '\0', chunk_size) == NULL);
>  
> -	if (err) {
> -		free(*path);
> -		*path = NULL;
> -	}
> -	return err;
> +	*path = strdup(p);
> +	if (*path == NULL)
> +		return got_error_from_errno("strdup");
> +	return NULL;
>  }
>  
>  static const struct got_error *


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68