| Message ID | 20181027071003.1347-10-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reduce #ifdef NO_PTHREADS | expand |
On 10/27/2018 3:10 AM, Nguyễn Thái Ngọc Duy wrote: > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > read-cache.c | 49 ++++++++++++++++++------------------------------- > 1 file changed, 18 insertions(+), 31 deletions(-) > > diff --git a/read-cache.c b/read-cache.c > index d57958233e..ba870bc3fd 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -1920,19 +1920,15 @@ struct index_entry_offset_table > struct index_entry_offset entries[FLEX_ARRAY]; > }; > > -#ifndef NO_PTHREADS > static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset); > static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot); > -#endif > > static size_t read_eoie_extension(const char *mmap, size_t mmap_size); > static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset); > > struct load_index_extensions > { > -#ifndef NO_PTHREADS > pthread_t pthread; > -#endif > struct index_state *istate; > const char *mmap; > size_t mmap_size; > @@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct index_state *istate, > return consumed; > } > > -#ifndef NO_PTHREADS > - > /* > * Mostly randomly chosen maximum thread counts: we > * cap the parallelism to online_cpus() threads, and we want > @@ -2122,7 +2116,6 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con > > return consumed; > } > -#endif > > /* remember to discard_cache() before reading a different cache! */ > int do_read_index(struct index_state *istate, const char *path, int must_exist) > @@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) > size_t mmap_size; > struct load_index_extensions p; > size_t extension_offset = 0; > -#ifndef NO_PTHREADS > int nr_threads, cpus; > struct index_entry_offset_table *ieot = NULL; > -#endif > > if (istate->initialized) > return istate->cache_nr; > @@ -2181,15 +2172,18 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) > > src_offset = sizeof(*hdr); > > -#ifndef NO_PTHREADS > - nr_threads = git_config_get_index_threads(); > + if (HAVE_THREADS) { In this case, nr_threads is already capped to online_cpus() below so this HAVE_THREADS test isn't needed. > + nr_threads = git_config_get_index_threads(); > > - /* TODO: does creating more threads than cores help? */ > - if (!nr_threads) { > - nr_threads = istate->cache_nr / THREAD_COST; > - cpus = online_cpus(); > - if (nr_threads > cpus) > - nr_threads = cpus; > + /* TODO: does creating more threads than cores help? */ > + if (!nr_threads) { > + nr_threads = istate->cache_nr / THREAD_COST; > + cpus = online_cpus(); > + if (nr_threads > cpus) > + nr_threads = cpus; > + } > + } else { > + nr_threads = 1; > } > > if (nr_threads > 1) { > @@ -2219,21 +2213,16 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) > } else { > src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset); > } > -#else > - src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset); > -#endif > > istate->timestamp.sec = st.st_mtime; > istate->timestamp.nsec = ST_MTIME_NSEC(st); > > /* if we created a thread, join it otherwise load the extensions on the primary thread */ > -#ifndef NO_PTHREADS > - if (extension_offset) { > + if (HAVE_THREADS && extension_offset) { Here extension_offset won't be set if there wasn't a thread created so the HAVE_THREADS test is not needed. > int ret = pthread_join(p.pthread, NULL); > if (ret) > die(_("unable to join load_index_extensions thread: %s"), strerror(ret)); > } > -#endif > if (!extension_offset) { > p.src_offset = src_offset; > load_index_extensions(&p); > @@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, > if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0) > return -1; > > -#ifndef NO_PTHREADS > - nr_threads = git_config_get_index_threads(); > + if (HAVE_THREADS) This shouldn't be needed either. My assumption was that if someone explicitly asked for multiple threads we're better off failing than silently ignoring their request. The default/automatic setting will detect the number of cpus and behave correctly. > + nr_threads = git_config_get_index_threads(); > + else > + nr_threads = 1; > + > if (nr_threads != 1) { > int ieot_blocks, cpus; > > @@ -2787,7 +2779,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, > ieot_entries = DIV_ROUND_UP(entries, ieot_blocks); > } > } > -#endif > > offset = lseek(newfd, 0, SEEK_CUR); > if (offset < 0) { > @@ -2871,8 +2862,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, > * strip_extensions parameter as we need it when loading the shared > * index. > */ > -#ifndef NO_PTHREADS > - if (ieot) { > + if (HAVE_THREADS && ieot) { Again, this one isn't needed. If there is only 1 thread, we don't read the eoie or the ieot (see the code just above this) so the ieot test is sufficient. > struct strbuf sb = STRBUF_INIT; > > write_ieot_extension(&sb, ieot); > @@ -2883,7 +2873,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, > if (err) > return -1; > } > -#endif > > if (!strip_extensions && istate->split_index) { > struct strbuf sb = STRBUF_INIT; > @@ -3469,7 +3458,6 @@ static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, > strbuf_add(sb, hash, the_hash_algo->rawsz); > } > > -#ifndef NO_PTHREADS > #define IEOT_VERSION (1) > > static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset) > @@ -3542,4 +3530,3 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta > strbuf_add(sb, &buffer, sizeof(uint32_t)); > } > } > -#endif >
On Mon, Oct 29, 2018 at 6:05 PM Ben Peart <peartben@gmail.com> wrote: > > @@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, > > if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0) > > return -1; > > > > -#ifndef NO_PTHREADS > > - nr_threads = git_config_get_index_threads(); > > + if (HAVE_THREADS) > > This shouldn't be needed either. My assumption was that if someone > explicitly asked for multiple threads we're better off failing than > silently ignoring their request. The default/automatic setting will > detect the number of cpus and behave correctly. No. I can have ~/.gitconfig shared between different machines. One with multithread support and one without. I should not have to fall back to conditional includes in order to use the same config file on the machine without multithread.
On 10/29/2018 1:21 PM, Duy Nguyen wrote: > On Mon, Oct 29, 2018 at 6:05 PM Ben Peart <peartben@gmail.com> wrote: >>> @@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, >>> if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0) >>> return -1; >>> >>> -#ifndef NO_PTHREADS >>> - nr_threads = git_config_get_index_threads(); >>> + if (HAVE_THREADS) >> >> This shouldn't be needed either. My assumption was that if someone >> explicitly asked for multiple threads we're better off failing than >> silently ignoring their request. The default/automatic setting will >> detect the number of cpus and behave correctly. > > No. I can have ~/.gitconfig shared between different machines. One > with multithread support and one without. I should not have to fall > back to conditional includes in order to use the same config file on > the machine without multithread. > If you want to share the ~/.gitconfig across machines that have different numbers of CPUs, it makes more sense to me to leave the setting at the "auto" setting rather than specifically overriding it to a number that won't work on at least one of your machines. Then it all "just works" without the need to conditional includes.
Duy Nguyen <pclouds@gmail.com> writes: >> This shouldn't be needed either. My assumption was that if someone >> explicitly asked for multiple threads we're better off failing than >> silently ignoring their request. The default/automatic setting will >> detect the number of cpus and behave correctly. > > No. I can have ~/.gitconfig shared between different machines. One > with multithread support and one without. I should not have to fall > back to conditional includes in order to use the same config file on > the machine without multithread. Our attitude has been to fail unsatisfyable requests from the command line loudly to let the users' know, and tolerate having elements from the future versions of Git in the configuration file by ignoring them or tweaking them (silently or with a warning). So this hunk is in line with the current practice, and we should keep it this way. It is a separate discussion if we want to rethink that whole attitude, that discussion may be worth having, and I think that discussion is wider than this single topic.
diff --git a/read-cache.c b/read-cache.c index d57958233e..ba870bc3fd 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1920,19 +1920,15 @@ struct index_entry_offset_table struct index_entry_offset entries[FLEX_ARRAY]; }; -#ifndef NO_PTHREADS static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset); static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot); -#endif static size_t read_eoie_extension(const char *mmap, size_t mmap_size); static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset); struct load_index_extensions { -#ifndef NO_PTHREADS pthread_t pthread; -#endif struct index_state *istate; const char *mmap; size_t mmap_size; @@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct index_state *istate, return consumed; } -#ifndef NO_PTHREADS - /* * Mostly randomly chosen maximum thread counts: we * cap the parallelism to online_cpus() threads, and we want @@ -2122,7 +2116,6 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con return consumed; } -#endif /* remember to discard_cache() before reading a different cache! */ int do_read_index(struct index_state *istate, const char *path, int must_exist) @@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) size_t mmap_size; struct load_index_extensions p; size_t extension_offset = 0; -#ifndef NO_PTHREADS int nr_threads, cpus; struct index_entry_offset_table *ieot = NULL; -#endif if (istate->initialized) return istate->cache_nr; @@ -2181,15 +2172,18 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) src_offset = sizeof(*hdr); -#ifndef NO_PTHREADS - nr_threads = git_config_get_index_threads(); + if (HAVE_THREADS) { + nr_threads = git_config_get_index_threads(); - /* TODO: does creating more threads than cores help? */ - if (!nr_threads) { - nr_threads = istate->cache_nr / THREAD_COST; - cpus = online_cpus(); - if (nr_threads > cpus) - nr_threads = cpus; + /* TODO: does creating more threads than cores help? */ + if (!nr_threads) { + nr_threads = istate->cache_nr / THREAD_COST; + cpus = online_cpus(); + if (nr_threads > cpus) + nr_threads = cpus; + } + } else { + nr_threads = 1; } if (nr_threads > 1) { @@ -2219,21 +2213,16 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) } else { src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset); } -#else - src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset); -#endif istate->timestamp.sec = st.st_mtime; istate->timestamp.nsec = ST_MTIME_NSEC(st); /* if we created a thread, join it otherwise load the extensions on the primary thread */ -#ifndef NO_PTHREADS - if (extension_offset) { + if (HAVE_THREADS && extension_offset) { int ret = pthread_join(p.pthread, NULL); if (ret) die(_("unable to join load_index_extensions thread: %s"), strerror(ret)); } -#endif if (!extension_offset) { p.src_offset = src_offset; load_index_extensions(&p); @@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0) return -1; -#ifndef NO_PTHREADS - nr_threads = git_config_get_index_threads(); + if (HAVE_THREADS) + nr_threads = git_config_get_index_threads(); + else + nr_threads = 1; + if (nr_threads != 1) { int ieot_blocks, cpus; @@ -2787,7 +2779,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, ieot_entries = DIV_ROUND_UP(entries, ieot_blocks); } } -#endif offset = lseek(newfd, 0, SEEK_CUR); if (offset < 0) { @@ -2871,8 +2862,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, * strip_extensions parameter as we need it when loading the shared * index. */ -#ifndef NO_PTHREADS - if (ieot) { + if (HAVE_THREADS && ieot) { struct strbuf sb = STRBUF_INIT; write_ieot_extension(&sb, ieot); @@ -2883,7 +2873,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, if (err) return -1; } -#endif if (!strip_extensions && istate->split_index) { struct strbuf sb = STRBUF_INIT; @@ -3469,7 +3458,6 @@ static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, strbuf_add(sb, hash, the_hash_algo->rawsz); } -#ifndef NO_PTHREADS #define IEOT_VERSION (1) static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset) @@ -3542,4 +3530,3 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta strbuf_add(sb, &buffer, sizeof(uint32_t)); } } -#endif
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- read-cache.c | 49 ++++++++++++++++++------------------------------- 1 file changed, 18 insertions(+), 31 deletions(-)