From: Ben Peart Date: Fri, 07 Sep 2018 18:31:54 GMT Subject: Re: [PATCH v3 0/4] read-cache: speed up index load through parallelization Message-ID: <6ef05d29-b303-0282-7ca7-6e51efded005@gmail.com> In-Reply-To: On 9/7/2018 1:21 PM, Junio C Hamano wrote: > Ben Peart 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. >> 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. >> 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.