Re: [PATCH v3 0/4] read-cache: speed up index load through parallelization

From: Ben Peart <peartben@gmail.com>
Date: Sep 7, 2018, 18:31 UTC
Message-ID: <6ef05d29-b303-0282-7ca7-6e51efded005@gmail.com>
In-Reply-To: <xmqq5zzhxlxm.fsf@gitster-ct.c.googlers.com>

On 9/7/2018 1:21 PM, Junio C Hamano wrote:

Show 20 quoted lines

> Ben Peart <benpeart@microsoft.com> writes:
> 
>> On further investigation with the previous patch, I noticed that my test
>> repos didn't contain the cache tree extension in their index. After doing a
>> commit to ensure they existed, I realized that in some instances, the time
>> to load the cache tree exceeded the time to load all the cache entries in
>> parallel.  Because the thread to read the cache tree was started last (due
>> to having to parse through all the cache entries first) we weren't always
>> getting optimal performance.
>>
>> To better optimize for this case, I decided to write the EOIE extension
>> as suggested by Junio [1] in response to my earlier multithreading patch
>> series [2].  This enables me to spin up the thread to load the extensions
>> earlier as it no longer has to parse through all the cache entries first.
> 
> Hmph. I kinda liked the simplicity of the previous one, but if we
> need to start reading the extension sections sooner by eliminating
> the overhead to scan the cache entries, perhaps we should bite the
> bullet now.
>

I preferred the simplicity as well but when I was profiling the code and found out that loading the extensions was most often the last thread to complete, I took this intermediate step to speed things up.

Show 16 quoted lines

>> The big changes in this iteration are:
>>
>> - add the EOIE extension
>> - update the index extension worker thread to start first
> 
> I guess I'd need to see the actual patch to find this out, but once
> we rely on a new extension, then we could omit scanning the main
> index even to partition the work among workers (i.e. like the topic
> long ago, you can have list of pointers into the main index to help
> partitioning, plus reset the prefix compression used in v4).  I
> think you didn't get that far in this round, which is good.  If the
> gain with EOIE alone (and starting the worker for the extension
> section early) is large enough without such a pre-computed work
> partition table, the simplicity of this round may give us a good
> stopping point.
>

Agreed. I didn't go that far in this series as it doesn't appear to be necessary. We could always add that later if it turned out to be worth the additional complexity.

Show 9 quoted lines

>> This patch conflicts with Duy's patch to remove the double memory copy and
>> pass in the previous ce instead.  The two will need to be merged/reconciled
>> once they settle down a bit.
> 
> Thanks.  I have a feeling that 67922abb ("read-cache.c: optimize
> reading index format v4", 2018-09-02) is already 'next'-worthy
> and ready to be built on, but I'd prefer to hear from Duy to double
> check.
>

I'll take a closer look at what this will entail. It gets more complicated as we don't actually have a previous expanded cache entry when starting each thread. I'll see how complex it makes the code and how much additional performance it gives.