Re: [PATCH v3 3/4] read-cache: load cache extensions on a worker thread

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

Ben Peart <benpeart@microsoft.com> writes:

> +struct load_index_extensions
> +{
> +#ifndef NO_PTHREADS
> +	pthread_t pthread;
> +#endif
> +	struct index_state *istate;
> +	void *mmap;
> +	size_t mmap_size;
> +	unsigned long src_offset;

If the file format only allows uint32_t on any platform, perhaps
this is better specified as uint32_t?  Or if this is offset into
a mmap'ed region of memory, size_t may be more appropriate.

Same comment applies to "extension_offset" we see below (which in
turn means the returned type of read_eoie_extension() function may
want to match).

> + };

Space before '}'??

> +
> +static void *load_index_extensions(void *_data)
> +{
> +	struct load_index_extensions *p = _data;

Perhaps we are being superstitious, but I think our code try to
avoid leading underscore when able, i.e.

	load_index_extensions(void *data_)
	{
		struct load_index_extensions *p = data;

> +	unsigned long src_offset = p->src_offset;
> +
> +	while (src_offset <= p->mmap_size - the_hash_algo->rawsz - 8) {
> +		/* After an array of active_nr index entries,
> +		 * there can be arbitrary number of extended
> +		 * sections, each of which is prefixed with
> +		 * extension name (4-byte) and section length
> +		 * in 4-byte network byte order.
> +		 */
> +		uint32_t extsize;
> +		memcpy(&extsize, (char *)p->mmap + src_offset + 4, 4);
> +		extsize = ntohl(extsize);

The same "ntohl(), not get_be32()?" question as the one for the
previous step applies here, too.  I think the answer is "the
original was written that way" and that is acceptable, but once this
series lands, we may want to review the whole file and see if it is
worth making them consistent with a separate clean-up patch.

I think mmap() and munmap() are the only places that wants p->mmap
and mmap parameters passed around in various callchains to be of
type "void *"---I wonder if it is simpler to use "const char *"
throughout and only cast it to "void *" when necessary (I suspect
that there is nowhere we need to cast to or from "void *" explicitly
if we did so---assignment and argument passing would give us an
appropriate cast for free)?

> +		if (read_index_extension(p->istate,
> +			(const char *)p->mmap + src_offset,
> +			(char *)p->mmap + src_offset + 8,
> +			extsize) < 0) {
> +			munmap(p->mmap, p->mmap_size);
> +			die("index file corrupt");
> +		}
> +	...
> @@ -1907,6 +1951,11 @@ ...
> ...
> +	p.mmap = mmap;
> +	p.mmap_size = mmap_size;
> +
> +#ifndef NO_PTHREADS
> +	nr_threads = git_config_get_index_threads();
> +	if (!nr_threads)
> +		nr_threads = online_cpus();
> +
> +	if (nr_threads >= 2) {
> +		extension_offset = read_eoie_extension(mmap, mmap_size);
> +		if (extension_offset) {
> +			/* create a thread to load the index extensions */
> +			p.src_offset = extension_offset;
> +			if (pthread_create(&p.pthread, NULL, load_index_extensions, &p))
> +				die(_("unable to create load_index_extensions_thread"));
> +		}
> +	}
> +#endif

Makes sense.