On 9/7/2018 1:21 PM, Junio C Hamano wrote:
Ben Peart <benpeart@xxxxxxxxxxxxx> 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.
