Re: [PATCH v2 0/7] thread lazy_init_name_hash

[Jeff Hostetler]

On 4/5/2017 10:22 PM, Duy Nguyen wrote:

On Thu, Mar 23, 2017 at 8:46 PM,   wrote:

This patch series is a performance optimization for
lazy_init_name_hash() in name-hash.c on very large
repositories.

This change allows lazy_init_name_hash() to optionally
use multiple threads when building the the_index.dir_hash
and the_index.name_hash hashmaps.  The original code path
has been preserved and is used when the repo is small or
the system does not have sufficient CPUs.


If sha1 verification in the index file can now be optionally skipped,
I wonder if you would have faster startup time by storing hashes in
the index as an extension. I have never tried it (though I planned to
have some sort of caching for this) but I would guess loading hashes
would cost less than 0.27 seconds, hopefully closer to 0.05 seconds.



I've thought about doing that, but haven't gotten around to
actually trying it yet.  It's on my TODO list though.

Thanks
Jeff

Re: [PATCH v2 0/7] thread lazy_init_name_hash

[Duy Nguyen]

On Thu, Mar 23, 2017 at 8:46 PM,   wrote:
> This patch series is a performance optimization for
> lazy_init_name_hash() in name-hash.c on very large
> repositories.
>
> This change allows lazy_init_name_hash() to optionally
> use multiple threads when building the the_index.dir_hash
> and the_index.name_hash hashmaps.  The original code path
> has been preserved and is used when the repo is small or
> the system does not have sufficient CPUs.

If sha1 verification in the index file can now be optionally skipped,
I wonder if you would have faster startup time by storing hashes in
the index as an extension. I have never tried it (though I planned to
have some sort of caching for this) but I would guess loading hashes
would cost less than 0.27 seconds, hopefully closer to 0.05 seconds.
-- 
Duy

Re: [PATCH v2 0/7] thread lazy_init_name_hash

[Junio C Hamano]

Jeff Hostetler  writes:

> WRT the assert() in name-hash.c, Stefan suggested converting it
> to an if-!-die form in an earlier message in this thread.  I'm OK
> with that or with removing the assert completely.

I actually am OK with leaving things as they are ;-)

Re: [PATCH v2 0/7] thread lazy_init_name_hash

[Stefan Beller]

On Fri, Mar 24, 2017 at 5:39 AM, Jeff Hostetler  wrote:
> WRT the assert() in name-hash.c, Stefan suggested converting it
> to an if-!-die form in an earlier message in this thread.  I'm OK
> with that or with removing the assert completely.

I think removing them completely sounds even better.

Thanks,
Stefan

Re: [PATCH v2 0/7] thread lazy_init_name_hash

[Jeff Hostetler]

On 3/23/2017 1:52 PM, Junio C Hamano wrote:

The API document update in 4/7 is a nice addition and it comes at
the right spot in the series, just after API enhancement is done.  I
gave a quick reading on it twice, and all looked reasonable.  Nicely
done.


Thanks.


I queued the sparse things Ramsay pointed out in the form of
"SQUASH???" immediately that follows your original patch while
applying the series, so please check what I push out (when it
happens) and if there is no further change needed, just tell me to
actually squash them in, if you think these SQUASH??? patches are
good.  That way we do not have to have another reroll only to fix
these up ;-)


The squashes look fine.

WRT the assert() in name-hash.c, Stefan suggested converting it
to an if-!-die form in an earlier message in this thread.  I'm OK
with that or with removing the assert completely.


I'll need to re-read "name-hash: perf improvement for lazy_init_name_hash"
later again to convince myself, but during my initial read (from the
last round) I didn't spot anything wrong there.

Thanks.



Thanks,
Jeff

Re: [PATCH v2 0/7] thread lazy_init_name_hash

[Junio C Hamano]

g...@jeffhostetler.com writes:

> From: Jeff Hostetler 
>
> Version 2 of this patch series addresses the coding
> style issues, compile errors in non-threaded builds,
> and updated API documentation.
>
>
> This patch series is a performance optimization for
> lazy_init_name_hash() in name-hash.c on very large
> repositories.
>
> This change allows lazy_init_name_hash() to optionally
> use multiple threads when building the the_index.dir_hash
> and the_index.name_hash hashmaps.  The original code path
> has been preserved and is used when the repo is small or
> the system does not have sufficient CPUs.
>
> A helper command (t/helper/test-lazy-init-name-hash) was
> created to demonstrate performance differences and validate
> output.  For example, use the '-p' option to compare both
> code paths on a large repo.
>
> During our testing on the Windows source tree (3.1M
> files, 500K folders, 450MB index) with an 8 (logical)
> core machine, we reduced the runtime of lazy_init_name_hash()
> from 1.4 to 0.27 seconds.
>
> This patch series replaces my earlier
>  * jh/memihash-opt (2017-02-17) 5 commits
> patch series.  This series is an improvement over the
> original proposal because it completely isolates the changes
> in name name-hash.c (rather than having parts in preload-index.c)
> and eliminates the need to update/invalidate precomputed hash
> values as cache_entries are changed.

Having the above helps readers by giving them what to expect to see
to prepare them before they dive into the patches ;-)

The API document update in 4/7 is a nice addition and it comes at
the right spot in the series, just after API enhancement is done.  I
gave a quick reading on it twice, and all looked reasonable.  Nicely
done.

I queued the sparse things Ramsay pointed out in the form of
"SQUASH???" immediately that follows your original patch while
applying the series, so please check what I push out (when it
happens) and if there is no further change needed, just tell me to
actually squash them in, if you think these SQUASH??? patches are
good.  That way we do not have to have another reroll only to fix
these up ;-)

I'll need to re-read "name-hash: perf improvement for lazy_init_name_hash"
later again to convince myself, but during my initial read (from the
last round) I didn't spot anything wrong there.

Thanks.