From: Ben Peart <peartben@gmail.com>
Date: Sat, 08 Sep 2018 14:56:22 GMT
Subject: Re: [PATCH v3 3/4] read-cache: load cache extensions on a worker thread
Message-ID: <447feebe-c99a-dc53-21ae-d33541baf30d@gmail.com>
In-Reply-To: <xmqqbm99vwsy.fsf@gitster-ct.c.googlers.com>



On 9/7/2018 5:10 PM, Junio C Hamano wrote:
> 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;

That's what I get for copying code from elsewhere in the source. :-)

static void *preload_thread(void *_data)
{
	int nr;
	struct thread_data *p = _data;

since there isn't any need for the underscore at all, I'll just make it:

static void *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.
> 

Makes sense, I'll add a cleanup patch to fix the inconsistency and have 
them use get_be32().

> 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)?

Sure, I'll add minimizing the casting to the clean up patch.

> 
>> +		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.
>