From: Ben Peart <peartben@gmail.com>
Date: Fri, 07 Sep 2018 13:43:39 GMT
Subject: Re: [PATCH v3 4/4] read-cache: speed up index load through parallelization
Message-ID: <18022c85-cf2d-026e-d643-2b872f3cfbe8@gmail.com>
In-Reply-To: <20180907041657.GA12835@tor.lan>



On 9/7/2018 12:16 AM, Torsten Bögershausen wrote:
> 
>> diff --git a/read-cache.c b/read-cache.c
>> index fcc776aaf0..8537a55750 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1941,20 +1941,212 @@ static void *load_index_extensions(void *_data)
>>   	return NULL;
>>   }
>>   
>> +/*
>> + * A helper function that will load the specified range of cache entries
>> + * from the memory mapped file and add them to the given index.
>> + */
>> +static unsigned long load_cache_entry_block(struct index_state *istate,
>> +			struct mem_pool *ce_mem_pool, int offset, int nr, void *mmap,
>> +			unsigned long start_offset, struct strbuf *previous_name)
>> +{
>> +	int i;
>> +	unsigned long src_offset = start_offset;
> 
> I read an unsigned long here:
> should that be a size_t instead ?
> 
> (And probably even everywhere else in this patch)
> 

It's a fair question.  The pre-patch code had a mix of unsigned long and 
size_t.  Both src_offset and consumed were unsigned long but mmap_size 
was a size_t.  I stuck with that pattern for consistency.

While it would be possible to convert everything to size_t as a step to 
enable index files >4 GB, I have a hard time believing that will be 
necessary for a very long time and would likely require more substantial 
changes to enable that to work.

>> +
>> +	for (i = offset; i < offset + nr; i++) {
>> +		struct ondisk_cache_entry *disk_ce;
>> +		struct cache_entry *ce;
>> +		unsigned long consumed;
>> +
>> +		disk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);
>> +		ce = create_from_disk(ce_mem_pool, disk_ce, &consumed, previous_name);
>> +		set_index_entry(istate, i, ce);
>> +
>> +		src_offset += consumed;
>> +	}
>> +	return src_offset - start_offset;
>> +}
>> +
>> +static unsigned long load_all_cache_entries(struct index_state *istate,
>> +			void *mmap, size_t mmap_size, unsigned long src_offset)
>> +{
>> +	struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
>> +	unsigned long consumed;
>> +
>> +	if (istate->version == 4) {
>> +		previous_name = &previous_name_buf;
>> +		mem_pool_init(&istate->ce_mem_pool,
>> +				estimate_cache_size_from_compressed(istate->cache_nr));
>> +	} else {
>> +		previous_name = NULL;
>> +		mem_pool_init(&istate->ce_mem_pool,
>> +				estimate_cache_size(mmap_size, istate->cache_nr));
>> +	}
>> +
>> +	consumed = load_cache_entry_block(istate, istate->ce_mem_pool,
>> +					0, istate->cache_nr, mmap, src_offset, previous_name);
>> +	strbuf_release(&previous_name_buf);
>> +	return consumed;
>> +}
>> +
>> +#ifndef NO_PTHREADS
>> +