Re: [PATCH v3 4/4] read-cache: speed up index load through parallelization
- From
Ben Peart <peartben@gmail.com>
- Date
- Sep 7, 2018, 13:43 UTC
- 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:
Show 25 quoted lines
>
>> 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.
Show 39 quoted lines
>> +
>> + 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
>> +