Reduce #ifdef NO_PTHREADS
cover Reduce #ifdef NO_PTHREADS
People seemed to support the idea of removing these #ifdef NO_PTHREADS [1] so this is a complete series. I left the #ifdef in run-command.c and transport-helper.c because those code looked complicated so perhaps we could clean them up later. Even these updated files could be updated more, I think, to reduce some code duplication, but I tried to keep the change here minimal.
[1] https://public-inbox.org/git/[email protected]/
Nguyễn Thái Ngọc Duy (10):
thread-utils: macros to unconditionally compile pthreads API
index-pack: remove #ifdef NO_PTHREADS
name-hash.c: remove #ifdef NO_PTHREADS
attr.c: remove #ifdef NO_PTHREADS
send-pack.c: remove #ifdef NO_PTHREADS
grep: remove #ifdef NO_PTHREADS
preload-index.c: remove #ifdef NO_PTHREADS
pack-objects: remove #ifdef NO_PTHREADS read-cache.c: remove #ifdef NO_PTHREADS
Clean up pthread_create() error handling
Makefile | 2 +- attr.c | 14 --------- builtin/grep.c | 58 +++++++++++++---------------------- builtin/index-pack.c | 68 +++++++++++------------------------------- builtin/pack-objects.c | 26 ++-------------- grep.c | 6 ---- grep.h | 6 ---- name-hash.c | 38 +++++++++-------------- pack-objects.h | 6 ---- preload-index.c | 23 +++++--------- read-cache.c | 49 +++++++++++------------------- run-command.c | 2 +- send-pack.c | 8 ++--- thread-utils.c | 48 +++++++++++++++++++++++++++++ thread-utils.h | 47 +++++++++++++++++++++++++++-- 15 files changed, 178 insertions(+), 223 deletions(-)
Signature · 1 line
2.19.1.647.g708186aaf9
PATCH 1/10 thread-utils: macros to unconditionally compile pthreads API
When built with NO_PTHREADS, the macros are used make the code build even though pthreads header and library may be missing. The code can still have different code paths for no threads support with HAVE_THREADS variable.
There are of course impacts on no-pthreads builds:
- data structure may get slightly bigger because all the mutexes and pthread_t are present (as an int)
- code execution is not impacted much. Locking (in hot path) is no-op. Other wrapper function calls really should not matter much.
- the binary size grows bigger because of threaded code. But at least on Linux this does not matter, if some code is not executed, it's not mapped in memory.
This is a preparation step to remove "#ifdef NO_PTHREADS" in the code mostly because of maintainability. As Jeff put it
Quoted 4 lines
it's probably OK to stop thinking of it as "non-threaded platforms are the default and must pay zero cost" and more as "threaded platforms are the default, and non-threaded ones are OK to pay a small cost as long as they still work".
--- Makefile | 2 +- thread-utils.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ thread-utils.h | 47 ++++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 93 insertions(+), 4 deletions(-)
Makefile +1 -1
@@ -991,6 +991,7 @@ LIB_OBJS += sub-process.o LIB_OBJS += symlinks.o LIB_OBJS += tag.o LIB_OBJS += tempfile.o +LIB_OBJS += thread-utils.o LIB_OBJS += tmp-objdir.o LIB_OBJS += trace.o LIB_OBJS += trailer.o
@@ -1674,7 +1675,6 @@ ifdef NO_PTHREADS else BASIC_CFLAGS += $(PTHREAD_CFLAGS) EXTLIBS += $(PTHREAD_LIBS) - LIB_OBJS += thread-utils.o endif ifdef HAVE_PATHS_H
thread-utils.c +48 -0
68 lines not on this page · raw message
thread-utils.h +44 -3
56 lines not on this page · raw message
Signature · 1 line
2.19.1.647.g708186aaf9
Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API
thread-utils.h line 25
Quoted 13 lines · Nguyễn Thái Ngọc Duy
thread-utils.h +13
-0+/* +* macros instead of typedefs because pthread definitions may have +* been pulled in by some system dependencies even though the user +* wants to disable pthread. +*/ +#define pthread_t int +#define pthread_mutex_t int +#define pthread_cond_t int + +#define pthread_mutex_init (mutex, attr) dummy_pthread_init(mutex) +#define pthread_mutex_lock (mutex) +#define pthread_mutex_unlock (mutex) +#define pthread_mutex_destroy (mutex)
OK, we only need to do the dummy init to shut up the compiler, and the rest really can just become noops. Makes sense.
thread-utils.h line 31
Quoted 5 lines
thread-utils.h +5
-0+#define pthread_cond_init (cond, attr) dummy_pthread_init(cond) +#define pthread_cond_wait (cond, mutex) +#define pthread_cond_signal (cond) +#define pthread_cond_broadcast (cond) +#define pthread_cond_destroy (cond)
And this is the same.
thread-utils.h line 34
Quoted 2 lines
thread-utils.h +2
-0+#define pthread_key_create (key, attr) dummy_pthread_init(key) +#define pthread_key_delete (key)
We don't need keys because we know we have only one key: the main thread. Makes sense. But...
thread-utils.h line 42
Quoted 2 lines
thread-utils.h +2
-0+#define pthread_setspecific (key, data) +#define pthread_getspecific (key) NULL
We expect to be able to store a void pointer here and get it back, which should work even for a single thread. Do we need something like:
extern void *pthread_specific_data;
#define pthread_setspecific(key, data) do { \
pthread_specific_data = data; \} while(0)
void pthread_getspecific(key) pthread_specific_data
thread-utils.h line 37
Quoted 2 lines
thread-utils.h +2
-0+#define pthread_create (thread, attr, fn, data) \ + dummy_pthread_create(thread, attr, fn, data)
I wondered if this should actually run "fn", but I guess there is not much point. At this point the caller expects the main thread to keep going, so this is already an error, and reporting ENOSYS is probably the best thing to do.
-Peff
Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API
Quoted 6 lines · Jeff King
thread-utils.h line 42
Quoted 2 lines
thread-utils.h +2
-0+#define pthread_setspecific (key, data) +#define pthread_getspecific (key) NULLWe expect to be able to store a void pointer here and get it back, which should work even for a single thread. Do we need something like:
Quoted 7 lines
extern void
*pthread_specific_data;#define pthread_setspecific(key, data) do { \ pthread_specific_data = data; \ } while(0)void
pthread_getspecific(key)pthread_specific_data
The data is per key though so a correct implementation may involve a hashmap or a list. It does simplify index-pack which has to fall back to nothread_data when pthreads is not available. But with index-pack being the only call site that can take advantage of this (run-command.c probably will use real pthreads library anyway), I'm not sure if it's worth really implementing these functions.
Signature · 1 line
Duy
Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API
Quoted 5 lines · Duy Nguyen
Quoted 5 lines
We expect to be able to store a void pointer here and get it back, which should work even for a single thread. Do we need something like:
extern void
*pthread_specific_data;
Quoted 8 lines
Quoted 5 lines
#define pthread_setspecific(key, data) do { \ pthread_specific_data = data; \ } while(0)void
pthread_getspecific(key)pthread_specific_dataThe data is per key though so a correct implementation may involve a hashmap or a list.
Ah, yeah, you're right, I was mixing up the thread id and the key in my head. I think it would just be an array of void pointers, with pthread_key_create() returning an static index.
Quoted 5 lines
It does simplify index-pack which has to fall back to
nothread_datawhen pthreads is not available. But with index-pack being the only call site that can take advantage of this (run-command.cprobably will use real pthreads library anyway), I'm not sure if it's worth really implementing these functions.
Yeah, you might be right.
-Peff
Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API
Quoted 12 lines · Jeff King
Quoted 9 lines · Duy Nguyen
Quoted 9 lines
We expect to be able to store a void pointer here and get it back, which should work even for a single thread. Do we need something like:
extern void
*pthread_specific_data;#define pthread_setspecific(key, data) do { \ pthread_specific_data = data; \ } while(0)
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
void
pthread_getspecific(key)pthread_specific_dataThe data is per key though so a correct implementation may involve a hashmap or a list.
Ah, yeah, you're right, I was mixing up the thread id and the key in my head. I think it would just be an array of void pointers, with
pthread_key_create()returning an static index.
We could redefine pthread_key_t as "void *" though, then _setspecific(key, data) is key = data; But there will be more changes in index-pack.c to take advantage of it. I took a stab but couldn't figure out fast enough where _setspecific(.., ¬hread_data) should be called in NO_PTHREADS mode (to simplify get_thread_data() to just a wrapper of _getspecific) and got bored. It could be a micro project for someone really wants to learn about index-pack.c
Signature · 1 line
Duy
PATCH 2/10 index-pack: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- builtin/index-pack.c | 68 ++++++++++++-------------------------------- 1 file changed, 18 insertions(+), 50 deletions(-)
builtin/index-pack.c +18 -50
@@ -42,9 +42,7 @@ struct base_data { };struct thread_local { -#ifndef NO_PTHREADS pthread_t thread; -#endif struct base_data *base_cache;size_t base_cache_used;int pack_fd;
@@ -98,8 +96,6 @@ static uint32_t input_crc32;static int input_fd, output_fd;static const char *curr_pack; -#ifndef NO_PTHREADS -static struct thread_local *thread_data;static int nr_dispatched;static int threads_active;
@@ -179,26 +175,6 @@ static void cleanup_thread(void)free (thread_data); } -#else - -#define read_lock () -#define read_unlock () - -#define counter_lock () -#define counter_unlock () - -#define work_lock () -#define work_unlock () - -#define deepest_delta_lock () -#define deepest_delta_unlock () - -#define type_cas_lock () -#define type_cas_unlock () - -#endif - -static int mark_link (struct object *obj,int type,void *data,struct fsck_options *options) {if (!obj)
@@ -364,22 +340,20 @@ static NORETURN void bad_object(off_t offset, const char *format, ...)static inline struct thread_local *get_thread_data (void ) { -#ifndef NO_PTHREADS -if (threads_active) -return pthread_getspecific (key); -assert (!threads_active && -"This should only be reached when all threads are gone" ); -#endif +if (HAVE_THREADS ) { +if (threads_active) +return pthread_getspecific (key); +assert (!threads_active && +"This should only be reached when all threads are gone" ); + }return ¬hread_data; } -#ifndef NO_PTHREADS static void set_thread_data (struct thread_local *data) {if (threads_active)pthread_setspecific (key, data); } -#endif static struct base_data *alloc_base_data (void ) {
@@ -1092,7 +1066,6 @@ static void resolve_base(struct object_entry *obj)find_unresolved_deltas (base_obj); } -#ifndef NO_PTHREADS static void *threaded_second_pass (void *data) {set_thread_data (data);
@@ -1116,7 +1089,6 @@ static void *threaded_second_pass(void *data) }return NULL ; } -#endif /* * First pass:
@@ -1213,7 +1185,6 @@ static void resolve_deltas(void) progress =start_progress (_ ("Resolving deltas" ), nr_ref_deltas + nr_ofs_deltas); -#ifndef NO_PTHREADS nr_dispatched =0 ;if (nr_threads >1 ||getenv ("GIT_FORCE_THREADS" )) {init_thread ();
@@ -1229,7 +1200,6 @@ static void resolve_deltas(void)cleanup_thread ();return ; } -#endif for (i =0 ; i < nr_objects; i++) {struct object_entry *obj = &objects[i];
@@ -1531,11 +1501,11 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)if (nr_threads <0 )die (_ ("invalid number of threads specified (%d)" ), nr_threads); -#ifdef NO_PTHREADS -if (nr_threads !=1 ) -warning (_ ("no threads support, ignoring %s" ), k); - nr_threads =1 ; -#endif +if (!HAVE_THREADS ) { +if (nr_threads !=1 ) +warning (_ ("no threads support, ignoring %s" ), k); + nr_threads =1 ; + }return 0 ; }return git_default_config (k, v, cb);
@@ -1723,12 +1693,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) nr_threads =strtoul (arg+10 , &end,0 );if (!arg[10 ] || *end || nr_threads <0 )usage (index_pack_usage); -#ifdef NO_PTHREADS -if (nr_threads !=1 ) -warning (_ ("no threads support, " -"ignoring %s" ), arg); - nr_threads =1 ; -#endif +if (!HAVE_THREADS ) { +if (nr_threads !=1 ) +warning (_ ("no threads support, " +"ignoring %s" ), arg); + nr_threads =1 ; + } }else if (starts_with (arg,"--pack_header=" )) {struct pack_header *hdr;char *c;
@@ -1791,14 +1761,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)if (strict) opts.flags |=WRITE_IDX_STRICT ; -#ifndef NO_PTHREADS -if (!nr_threads) { +if (HAVE_THREADS && !nr_threads) { nr_threads =online_cpus ();/* An experiment showed that more threads does not mean faster */ if (nr_threads >3 ) nr_threads =3 ; } -#endif curr_pack =open_pack_file (pack_name);parse_pack_header ();
Signature · 1 line
2.19.1.647.g708186aaf9
Re: [PATCH 02/10] index-pack: remove #ifdef NO_PTHREADS
Quoted 14 lines · Nguyễn Thái Ngọc Duy
static inline struct
thread_local*get_thread_data(void){builtin/index-pack.c +6
-6-#ifndef NO_PTHREADS -if (threads_active) -return pthread_getspecific (key); -assert (!threads_active && -"This should only be reached when all threads are gone" ); -#endif +if (HAVE_THREADS ) { +if (threads_active) +return pthread_getspecific (key); +assert (!threads_active && +"This should only be reached when all threads are gone" ); + }
builtin/index-pack.c static NORETURN void bad_object(off_t offset, const char *format, ...) · line 382
Quoted 10 lines
builtin/index-pack.c +0
-2return ¬hread_data; } -#ifndef NO_PTHREADS static void set_thread_data (struct thread_local *data) {if (threads_active)pthread_setspecific (key, data); } -#endif
If we do the getspecific/setspecific thing I mentioned for the previous patch, then I think helpers like this can just go away.
-Peff
PATCH 3/10 name-hash.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- name-hash.c | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-)
name-hash.c +4 -18
@@ -7,6 +7,7 @@*/ #define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" +#include "thread-utils.h" struct dir_entry {struct hashmap_entry ent;
@@ -131,22 +132,6 @@ static int cache_entry_cmp(const void *unused_cmp_data,static int lazy_try_threaded =1 ;static int lazy_nr_dir_threads; -#ifdef NO_PTHREADS - -static inline int lookup_lazy_params (struct index_state *istate) -{ -return 0 ; -} - -static inline void threaded_lazy_init_name_hash ( -struct index_state *istate) -{ -} - -#else - -#include "thread-utils.h" -/* * Set a minimum number of cache_entries that we will handle per * thread and use that to decide how many threads to run (upto
@@ -516,6 +501,9 @@ static void threaded_lazy_init_name_hash(struct lazy_dir_thread_data *td_dir;struct lazy_name_thread_data *td_name; +if (!HAVE_THREADS ) +return ; + k_start =0 ; nr_each =DIV_ROUND_UP (istate->cache_nr, lazy_nr_dir_threads);
@@ -574,8 +562,6 @@ static void threaded_lazy_init_name_hash(free (lazy_entries); } -#endif -static void lazy_init_name_hash (struct index_state *istate) {
Signature · 1 line
2.19.1.647.g708186aaf9
PATCH 4/10 attr.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- attr.c | 14 -------------- 1 file changed, 14 deletions(-)
attr.c +0 -14
@@ -41,23 +41,17 @@ const char *git_attr_name(const struct git_attr *attr)struct attr_hashmap {struct hashmap map; -#ifndef NO_PTHREADS pthread_mutex_t mutex; -#endif };static inline void hashmap_lock (struct attr_hashmap *map) { -#ifndef NO_PTHREADS pthread_mutex_lock (&map->mutex); -#endif }static inline void hashmap_unlock (struct attr_hashmap *map) { -#ifndef NO_PTHREADS pthread_mutex_unlock (&map->mutex); -#endif }/*
@@ -498,23 +492,17 @@ static struct check_vector {size_t nr;size_t alloc;struct attr_check **checks; -#ifndef NO_PTHREADS pthread_mutex_t mutex; -#endif } check_vector;static inline void vector_lock (void ) { -#ifndef NO_PTHREADS pthread_mutex_lock (&check_vector.mutex); -#endif }static inline void vector_unlock (void ) { -#ifndef NO_PTHREADS pthread_mutex_unlock (&check_vector.mutex); -#endif }static void check_vector_add (struct attr_check *c)
@@ -1181,8 +1169,6 @@ void git_all_attrs(const struct index_state *istate,void attr_start (void ) { -#ifndef NO_PTHREADS pthread_mutex_init (&g_attr_hashmap.mutex,NULL );pthread_mutex_init (&check_vector.mutex,NULL ); -#endif }
Signature · 1 line
2.19.1.647.g708186aaf9
PATCH 5/10 send-pack.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc DuyWhile at there correct "#include cache.h" position. It must be one of the first includes.
--- send-pack.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
send-pack.c +4 -4
@@ -1,4 +1,5 @@#include "builtin.h" +#include "cache.h" #include "config.h" #include "commit.h" #include "refs.h"
@@ -14,7 +15,7 @@#include "version.h" #include "sha1-array.h" #include "gpg-interface.h" -#include "cache.h" +#include "thread-utils.h" int option_parse_push_signed (const struct option *opt,const char *arg,int unset)
@@ -203,9 +204,8 @@ static int receive_status(int in, struct ref *refs)static int sideband_demux (int in,int out,void *data) {int *fd = data, ret; -#ifdef NO_PTHREADS -close (fd[1 ]); -#endif +if (!HAVE_THREADS ) +close (fd[1 ]); ret =recv_sideband ("send-pack" , fd[0 ], out);close (out);return ret;
Signature · 1 line
2.19.1.647.g708186aaf9
Re: [PATCH 05/10] send-pack.c: remove #ifdef NO_PTHREADS
send-pack.c line 2
Quoted 6 lines · Nguyễn Thái Ngọc Duy
While at there correct "#include
cache.h" position. It must be one of the first includes.
[...]send-pack.c +1
-0@@ -1,4 +1,5 @@#include "builtin.h" +#include "cache.h"
I think it's actually fine as it is. The rule is not about cache.h in particular, but that the first include must be one of the special ones. And builtin.h is such a special one. After that, there are no ordering rules.
send-pack.c static int receive_status(int in, struct ref *refs) · line 211
Quoted 12 lines
send-pack.c +2
-3@@ -203,9 +204,8 @@ static int receive_status(int in, struct ref *refs)static int sideband_demux (int in,int out,void *data) {int *fd = data, ret; -#ifdef NO_PTHREADS -close (fd[1 ]); -#endif +if (!HAVE_THREADS ) +close (fd[1 ]); ret =recv_sideband ("send-pack" , fd[0 ], out);close (out);return ret;
This one is a very interesting case. Your conversion here isn't wrong, but what we actually want to know is: is "struct async" implemented as a separate process or not.
And "struct async" will continue to switch on NO_PTHREADS, as it can never use these dummy bits. I can't think of a way that HAVE_THREADS would ever diverge from how "struct async" works, but my gut feeling is that we probably ought to have a separate variable for the async code to tell how it works. Even if it ends up being the same as HAVE_THREADS, it helps untangle the two cases.
-Peff
PATCH 6/10 grep: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- builtin/grep.c | 58 +++++++++++++++++--------------------------------- grep.c | 6 ------ grep.h | 6 ------ 3 files changed, 20 insertions(+), 50 deletions(-)
builtin/grep.c +20 -38
@@ -34,7 +34,6 @@ static int recurse_submodules;#define GREP_NUM_THREADS_DEFAULT 8static int num_threads; -#ifndef NO_PTHREADS static pthread_t *threads;/* We use one producer thread and THREADS consumer
@@ -265,13 +264,6 @@ static int wait_all(void)return hit; } -#else /* !NO_PTHREADS */ - -static int wait_all (void ) -{ -return 0 ; -} -#endif static int grep_cmd_config (const char *var,const char *value,void *cb) {
@@ -284,8 +276,7 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)if (num_threads <0 )die (_ ("invalid number of threads specified (%d) for %s" ), num_threads, var); -#ifdef NO_PTHREADS - else if (num_threads && num_threads != 1) { +else if (!HAVE_THREADS && num_threads && num_threads !=1 ) {/* * TRANSLATORS: %s is the configuration * variable for tweaking threads, currently
@@ -294,7 +285,6 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)warning (_ ("no threads support, ignoring %s" ), var); num_threads =0 ; } -#endif }if (!strcmp (var,"submodule.recurse" ))
@@ -330,17 +320,14 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,grep_source_init (&gs,GREP_SOURCE_OID , pathbuf.buf, path, oid);strbuf_release (&pathbuf); -#ifndef NO_PTHREADS -if (num_threads) { +if (HAVE_THREADS && num_threads) {/* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear() */ add_work (opt, &gs);return 0 ; - } else -#endif - { + }else {int hit; hit =grep_source (opt, &gs);
@@ -363,17 +350,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)grep_source_init (&gs,GREP_SOURCE_FILE , buf.buf, filename, filename);strbuf_release (&buf); -#ifndef NO_PTHREADS -if (num_threads) { +if (HAVE_THREADS && num_threads) {/* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear() */ add_work (opt, &gs);return 0 ; - } else -#endif - { + }else {int hit; hit =grep_source (opt, &gs);
@@ -1038,20 +1022,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix) pathspec.recursive =1 ; pathspec.recurse_submodules = !!recurse_submodules; -#ifndef NO_PTHREADS -if (list.nr || cached || show_in_pager) - num_threads =0 ; -else if (num_threads ==0 ) - num_threads =GREP_NUM_THREADS_DEFAULT ; -else if (num_threads <0 ) -die (_ ("invalid number of threads specified (%d)" ), num_threads); -if (num_threads ==1 ) +if (HAVE_THREADS ) { +if (list.nr || cached || show_in_pager) + num_threads =0 ; +else if (num_threads ==0 ) + num_threads =GREP_NUM_THREADS_DEFAULT ; +else if (num_threads <0 ) +die (_ ("invalid number of threads specified (%d)" ), num_threads); +if (num_threads ==1 ) + num_threads =0 ; + }else { +if (num_threads) +warning (_ ("no threads support, ignoring --threads" )); num_threads =0 ; -#else -if (num_threads) -warning (_ ("no threads support, ignoring --threads" )); - num_threads =0 ; -#endif + }if (!num_threads)/*
@@ -1062,15 +1046,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)*/ compile_grep_patterns (&opt); -#ifndef NO_PTHREADS -if (num_threads) { +if (HAVE_THREADS && num_threads) {if (!(opt.name_only || opt.unmatch_name_only || opt.count) && (opt.pre_context || opt.post_context || opt.file_break || opt.funcbody)) skip_first_line =1 ;start_threads (&opt); } -#endif if (show_in_pager && (cached || list.nr))die (_ ("--open-files-in-pager only works on the worktree" ));
@@ -1121,7 +1103,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) hit =grep_objects (&opt, &pathspec, &list); } -if (num_threads) +if (HAVE_THREADS && num_threads) hit |=wait_all ();if (hit && show_in_pager)run_pager (&opt, prefix);
grep.c +0 -6
@@ -1513,7 +1513,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol, } } -#ifndef NO_PTHREADS int grep_use_locks;/*
@@ -1539,11 +1538,6 @@ static inline void grep_attr_unlock(void)*/ pthread_mutex_t grep_read_mutex; -#else -#define grep_attr_lock () -#define grep_attr_unlock () -#endif -static int match_funcname (struct grep_opt *opt,struct grep_source *gs,char *bol,char *eol) {xdemitconf_t *xecfg = opt->priv;
grep.h +0 -6
@@ -229,7 +229,6 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs);extern struct grep_opt *grep_opt_dup (const struct grep_opt *opt);extern int grep_threads_ok (const struct grep_opt *opt); -#ifndef NO_PTHREADS /* * Mutex used around access to the attributes machinery if * opt->use_threads. Must be initialized/destroyed by callers!
@@ -250,9 +249,4 @@ static inline void grep_read_unlock(void) pthread_mutex_unlock(&grep_read_mutex); } -#else -#define grep_read_lock () -#define grep_read_unlock () -#endif -#endif
Signature · 1 line
2.19.1.647.g708186aaf9
Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS
Quoted 13 lines · Nguyễn Thái Ngọc Duy
builtin/grep.c +0
-1@@ -34,7 +34,6 @@ static int recurse_submodules;#define GREP_NUM_THREADS_DEFAULT 8static int num_threads; -#ifndef NO_PTHREADS static pthread_t *threads;/* We use one producer thread and THREADS consumer
builtin/grep.c static int wait_all(void) · line 274
Quoted 10 lines
builtin/grep.c +0
-7@@ -265,13 +264,6 @@ static int wait_all(void)return hit; } -#else /* !NO_PTHREADS */ - -static int wait_all (void ) -{ -return 0 ; -} -#endif
Cases like this are kind of weird. I'd expect to see wait_all() return immediately when !HAVE_THREADS. But we don't need to, because later we do this:
builtin/grep.c int cmd_grep(int argc, const char **argv, const char *prefix) · line 1107
Quoted 3 lines
builtin/grep.c +1
-1-if (num_threads) +if (HAVE_THREADS && num_threads) hit |=wait_all ();
Which I think works, but it feels like we're introducing some subtle dependencies about which functions get called in which cases. I'd hoped in general that the non-threaded code paths could mostly just collapse down into the same as the "threads == 1" cases, and we wouldn't have to ask "are threads even supported" in a lot of places.
I dunno. My comment isn't really an objection exactly, but mostly just an indication that I'm still thinking through what the best approach is, and what end state we want to end up in.
-Peff
Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS
Quoted 4 lines · Jeff King
Cases like this are kind of weird. I'd expect to see
wait_all()return immediately when !HAVE_THREADS. But we don't need to, because later we do this:
Quoted 9 lines
builtin/grep.c int cmd_grep(int argc, const char **argv, const char *prefix) · line 1107
Quoted 3 lines
builtin/grep.c +1
-1-if (num_threads) +if (HAVE_THREADS && num_threads) hit |=wait_all ();Which I think works, but it feels like we're introducing some subtle dependencies about which functions get called in which cases. I'd hoped in general that the non-threaded code paths could mostly just collapse down into the same as the "threads == 1" cases, and we wouldn't have to ask "are threads even supported" in a lot of places.
True, but the way "grep" code works with threads is already strange, and I suspect that the subtle strangeness you feel mostly comes from that. The single threaded code signaled "hit" with return value of individual functions like grep_file(), but the meaning of return value from them is changed to "does not matter--we do not have meaningful result yet at this point" when threading is used.
In the new world order where "threading is the norm but single-threaded is still supported", I wonder if we would want to drive the single threaded case the same way with the add_work(run) interface and return the "did we see hits?" etc. from the same (or at lesat "more similar than today's") interface, so that the flow of data smells the same in both cases.
Quoted 5 lines
I dunno. My comment isn't really an objection exactly, but mostly just an indication that I'm still thinking through what the best approach is, and what end state we want to end up in.
-Peff
Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS
Quoted 23 lines · Junio C Hamano
Quoted 4 lines · Jeff King
Cases like this are kind of weird. I'd expect to see
wait_all()return immediately when !HAVE_THREADS. But we don't need to, because later we do this:Quoted 9 lines
builtin/grep.c int cmd_grep(int argc, const char **argv, const char *prefix) · line 1107
Quoted 3 lines
builtin/grep.c +1
-1-if (num_threads) +if (HAVE_THREADS && num_threads) hit |=wait_all ();Which I think works, but it feels like we're introducing some subtle dependencies about which functions get called in which cases. I'd hoped in general that the non-threaded code paths could mostly just collapse down into the same as the "threads == 1" cases, and we wouldn't have to ask "are threads even supported" in a lot of places.
True, but the way "grep" code works with threads is already strange, and I suspect that the subtle strangeness you feel mostly comes from that. The single threaded code signaled "hit" with return value of individual functions like
grep_file(), but the meaning of return value from them is changed to "does not matter--we do not have meaningful result yet at this point" when threading is used.
Quoted 6 lines
In the new world order where "threading is the norm but single-threaded is still supported", I wonder if we would want to drive the single threaded case the same way with the
add_work(run)interface and return the "did we see hits?" etc. from the same (or at lesat "more similar than today's") interface, so that the flow of data smells the same in both cases.
Right, your second paragraph here is a better statement of what I was trying to get at. ;)
But if the problem is simply that we are not quite there yet in the grep code, I am OK with taking this as the first pass, and knowing that there is more cleanup to be done later (though that sort of thing is IMHO very useful in a commit message).
-Peff
Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS
Quoted 4 lines · Jeff King
But if the problem is simply that we are not quite there yet in the grep code, I am OK with taking this as the first pass, and knowing that there is more cleanup to be done later (though that sort of thing is IMHO very useful in a commit message).
Since the problem pops up now, I'm ok with updating/cleaning up all this in this series, unless there's benefits in keeping this series simple and merging it early (probably not?)
Signature · 1 line
Duy
Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS
Quoted 9 lines · Duy Nguyen
Quoted 4 lines · Jeff King
But if the problem is simply that we are not quite there yet in the grep code, I am OK with taking this as the first pass, and knowing that there is more cleanup to be done later (though that sort of thing is IMHO very useful in a commit message).
Since the problem pops up now, I'm ok with updating/cleaning up all this in this series, unless there's benefits in keeping this series simple and merging it early (probably not?)
Mostly I did not want to tax you. I would rather have this series and some cleanup left over, than to not have anything. But if you are interested in moving it further, I will not say no. :)
-Peff
Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS
Quoted 8 lines · Jeff King
Quoted 6 lines · Duy Nguyen
Quoted 4 lines · Jeff King
But if the problem is simply that we are not quite there yet in the grep code, I am OK with taking this as the first pass, and knowing that there is more cleanup to be done later (though that sort of thing is IMHO very useful in a commit message).
Quoted 7 lines
Quoted 3 lines
Since the problem pops up now, I'm ok with updating/cleaning up all this in this series, unless there's benefits in keeping this series simple and merging it early (probably not?)
Mostly I did not want to tax you. I would rather have this series and some cleanup left over, than to not have anything. But if you are interested in moving it further, I will not say no. :)
Likewise.
PATCH 7/10 preload-index.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- preload-index.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)
preload-index.c +2 -13
@@ -7,17 +7,7 @@#include "fsmonitor.h" #include "config.h" #include "progress.h" - -#ifdef NO_PTHREADS -static void preload_index (struct index_state *index, -const struct pathspec *pathspec, -unsigned int refresh_flags) -{ - ;/* nothing */ -} -#else - -#include <pthread.h> +#include "thread-utils.h" /* * Mostly randomly chosen maximum thread counts: we
@@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,struct thread_data data[MAX_PARALLEL ];struct progress_data pd; -if (!core_preload_index) +if (!HAVE_THREADS || !core_preload_index)return ; threads = index->cache_nr /THREAD_COST ;
@@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,trace_performance_leave ("preload index" ); } -#endif int read_index_preload(struct index_state *index, const struct pathspec *pathspec,
Signature · 1 line
2.19.1.647.g708186aaf9
Re: [PATCH 07/10] preload-index.c: remove #ifdef NO_PTHREADS
Quoted 29 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- preload-index.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)preload-index.c +1
-11@@ -7,17 +7,7 @@#include "fsmonitor.h" #include "config.h" #include "progress.h" - -#ifdef NO_PTHREADS -static void preload_index (struct index_state *index, -const struct pathspec *pathspec, -unsigned int refresh_flags) -{ - ;/* nothing */ -} -#else - -#include <pthread.h> +#include "thread-utils.h" /* * Mostly randomly chosen maximum thread counts: we
preload-index.c static void preload_index(struct index_state *index, · line 101
Quoted 5 lines
preload-index.c +1
-1@@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,struct thread_data data[MAX_PARALLEL ];struct progress_data pd; -if (!core_preload_index) +if (!HAVE_THREADS || !core_preload_index)
I also "hoped in general that the non-threaded code paths could mostly just collapse into the same as the "threads == 1" cases, and we wouldn't have to "are threads even supported" in a lot of places."
In this case, if we cap the threads to online_cpus() the later test for 'if (threads < 2)' would do that. I haven't measured this specific case but in the other cases I have measured, having more threads than cpus did not result in a performance win.
Quoted 12 lines
preload-index.c +0
-1return ; threads = index->cache_nr /THREAD_COST ;@@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,trace_performance_leave ("preload index" ); } -#endif int read_index_preload(struct index_state *index, const struct pathspec *pathspec,
PATCH 8/10 pack-objects: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- builtin/pack-objects.c | 26 ++------------------------ pack-objects.h | 6 ------ 2 files changed, 2 insertions(+), 30 deletions(-)
builtin/pack-objects.c +2 -24
@@ -1953,8 +1953,6 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,return 0 ; } -#ifndef NO_PTHREADS -/* Protect access to object database */ static pthread_mutex_t read_mutex;#define read_lock () pthread_mutex_lock(&read_mutex)
@@ -1979,16 +1977,6 @@ static pthread_mutex_t progress_mutex;* ahead in the list because they can be stolen and would need * progress_mutex for protection. */ -#else - -#define read_lock () (void)0 -#define read_unlock () (void)0 -#define cache_lock () (void)0 -#define cache_unlock () (void)0 -#define progress_lock () (void)0 -#define progress_unlock () (void)0 - -#endif /* * Return the size of the object without doing any delta
@@ -2347,8 +2335,6 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,free (array); } -#ifndef NO_PTHREADS -static void try_to_free_from_threads (size_t size) {read_lock ();
@@ -2578,10 +2564,6 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,free (p); } -#else -#define ll_find_deltas (l, s, w, d, p) find_deltas(l, &s, w, d, p) -#endif -static void add_tag_chain (const struct object_id *oid) {struct tag *tag;
@@ -2734,12 +2716,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)if (delta_search_threads <0 )die (_ ("invalid number of threads specified (%d)" ), delta_search_threads); -#ifdef NO_PTHREADS -if (delta_search_threads !=1 ) { +if (!HAVE_THREADS && delta_search_threads !=1 ) {warning (_ ("no threads support, ignoring %s" ), k); delta_search_threads =0 ; } -#endif return 0 ; }if (!strcmp (k,"pack.indexversion" )) {
@@ -3402,10 +3382,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)if (!delta_search_threads)/* --threads=0 means autodetect */ delta_search_threads =online_cpus (); -#ifdef NO_PTHREADS -if (delta_search_threads !=1 ) +if (!HAVE_THREADS && delta_search_threads !=1 )warning (_ ("no threads support, ignoring --threads" )); -#endif if (!pack_to_stdout && !pack_size_limit) pack_size_limit = pack_size_limit_cfg;if (pack_to_stdout && pack_size_limit)
pack-objects.h +0 -6
@@ -145,9 +145,7 @@ struct packing_data {struct packed_git **in_pack_by_idx;struct packed_git **in_pack; -#ifndef NO_PTHREADS pthread_mutex_t lock; -#endif /* * This list contains entries for bases which we know the other side
@@ -169,15 +167,11 @@ void prepare_packing_data(struct packing_data *pdata);static inline void packing_data_lock (struct packing_data *pdata) { -#ifndef NO_PTHREADS pthread_mutex_lock (&pdata->lock); -#endif }static inline void packing_data_unlock (struct packing_data *pdata) { -#ifndef NO_PTHREADS pthread_mutex_unlock (&pdata->lock); -#endif } struct object_entry *packlist_alloc(struct packing_data *pdata,
Signature · 1 line
2.19.1.647.g708186aaf9
PATCH 9/10 read-cache.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- read-cache.c | 49 ++++++++++++++++++------------------------------- 1 file changed, 18 insertions(+), 31 deletions(-)
read-cache.c +18 -31
@@ -1920,19 +1920,15 @@ struct index_entry_offset_tablestruct 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, conreturn 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_tastrbuf_add (sb, &buffer,sizeof (uint32_t )); } } -#endif
Signature · 1 line
2.19.1.647.g708186aaf9
Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS
Quoted 58 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- read-cache.c | 49 ++++++++++++++++++------------------------------- 1 file changed, 18 insertions(+), 31 deletions(-)read-cache.c +0
-9@@ -1920,19 +1920,15 @@ struct index_entry_offset_tablestruct 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, conreturn 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;
read-cache.c int do_read_index(struct index_state *istate, const char *path, int must_exist) · line 2175
Quoted 6 lines
read-cache.c +1
-2@@ -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.
Quoted 25 lines
read-cache.c +10
-7+ 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
read-cache.c int do_read_index(struct index_state *istate, const char *path, int must_exist) · line 2221
Quoted 10 lines
read-cache.c +1
-4- 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.
Quoted 9 lines
read-cache.c +0
-1int 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);
read-cache.c static int do_write_index(struct index_state *istate, struct tempfile *tempfile, · line 2748
Quoted 6 lines
read-cache.c +1
-2@@ -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.
Quoted 16 lines
read-cache.c +4
-1+ 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 ) {
read-cache.c static int do_write_index(struct index_state *istate, struct tempfile *tempfile, · line 2865
Quoted 6 lines
read-cache.c +1
-2@@ -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.
Quoted 25 lines
read-cache.c +0
-3struct 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_tastrbuf_add (sb, &buffer,sizeof (uint32_t )); } } -#endif
Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS
Quoted 4 lines · Ben Peart
Quoted 4 lines
read-cache.c +0
-0@@ -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 ;
Quoted 8 lines
read-cache.c static int do_write_index(struct index_state *istate, struct tempfile *tempfile, · line 2748
Quoted 3 lines
read-cache.c +1
-2-#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.
Signature · 1 line
Duy
Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS
Quoted 14 lines · Duy Nguyen
Quoted 4 lines · Ben Peart
Quoted 4 lines
read-cache.c +0
-0@@ -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 ;Quoted 8 lines
read-cache.c static int do_write_index(struct index_state *istate, struct tempfile *tempfile, · line 2748
Quoted 3 lines
read-cache.c +1
-2-#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.
Quoted 5 lines
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.
Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS
In reply to Duy NguyenQuoted 9 lines · Duy Nguyen
Quoted 4 lines
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.
PATCH 10/10 Clean up pthread_create() error handling
In reply to Nguyễn Thái Ngọc DuyNormally pthread_create() rarely fails and sometimes error handling can be neglected. But with new pthreads wrapper, pthread_create() will return ENOSYS on a system without thread support.
Threaded code _is_ protected by HAVE_THREADS and pthread_create() should never run in the first place. But the situation could change in the future and bugs may sneak in. Make sure that all pthread_create() checks and handles the result.
While at there, mark these strings for translation if they aren't.
--- name-hash.c | 16 ++++++++++------ preload-index.c | 8 ++++++-- run-command.c | 2 +- 3 files changed, 17 insertions(+), 9 deletions(-)
name-hash.c +10 -6
@@ -494,6 +494,7 @@ static inline void lazy_update_dir_ref_counts(static void threaded_lazy_init_name_hash (struct index_state *istate) { +int err;int nr_each;int k_start;int t;
@@ -526,8 +527,9 @@ static void threaded_lazy_init_name_hash(if (k_start > istate->cache_nr) k_start = istate->cache_nr; td_dir_t->k_end = k_start; -if (pthread_create (&td_dir_t->pthread,NULL , lazy_dir_thread_proc, td_dir_t)) -die ("unable to create lazy_dir_thread" ); + err =pthread_create (&td_dir_t->pthread,NULL , lazy_dir_thread_proc, td_dir_t); +if (err) +die (_ ("unable to create lazy_dir thread: %s" ),strerror (err)); }for (t =0 ; t < lazy_nr_dir_threads; t++) {struct lazy_dir_thread_data *td_dir_t = td_dir + t;
@@ -547,13 +549,15 @@ static void threaded_lazy_init_name_hash(*/ td_name->istate = istate; td_name->lazy_entries = lazy_entries; -if (pthread_create (&td_name->pthread,NULL , lazy_name_thread_proc, td_name)) -die ("unable to create lazy_name_thread" ); + err =pthread_create (&td_name->pthread,NULL , lazy_name_thread_proc, td_name); +if (err) +die (_ ("unable to create lazy_name thread: %s" ),strerror (err));lazy_update_dir_ref_counts (istate, lazy_entries); -if (pthread_join (td_name->pthread,NULL )) -die ("unable to join lazy_name_thread" ); + err =pthread_join (td_name->pthread,NULL ); +if (err) +die (_ ("unable to join lazy_name thread: %s" ),strerror (err));cleanup_dir_mutex ();
preload-index.c +6 -2
@@ -121,6 +121,8 @@ static void preload_index(struct index_state *index,for (i =0 ; i < threads; i++) {struct thread_data *p = data+i; +int err; + p->index = index;if (pathspec)copy_pathspec (&p->pathspec, pathspec);
@@ -129,8 +131,10 @@ static void preload_index(struct index_state *index,if (pd.progress) p->progress = &pd; offset += work; -if (pthread_create (&p->pthread,NULL , preload_thread, p)) -die ("unable to create threaded lstat" ); + err =pthread_create (&p->pthread,NULL , preload_thread, p); + +if (err) +die (_ ("unable to create threaded lstat: %s" ),strerror (err)); }for (i =0 ; i < threads; i++) {struct thread_data *p = data+i;
run-command.c +1 -1
@@ -1213,7 +1213,7 @@ int start_async(struct async *async) {int err =pthread_create (&async->tid,NULL , run_thread, async);if (err) { -error_errno ("cannot create thread" ); +error (_ ("cannot create async thread: %s" ),strerror (err)); goto error; } }
Signature · 1 line
2.19.1.647.g708186aaf9
Quoted 6 lines · Nguyễn Thái Ngọc Duy
People seemed to support the idea of removing these #ifdef
NO_PTHREADS[1] so this is a complete series. I left the #ifdef inrun-command.candtransport-helper.cbecause those code looked complicated so perhaps we could clean them up later. Even these updated files could be updated more, I think, to reduce some code duplication, but I tried to keep the change here minimal.
Some of the bits in run-command.c can probably be converted, but all of the "struct async" bits can't ever be. Because there the fallback is not "OK, let's just run a single thread" but a whole separate forking mechanism.
-Peff
Quoted 9 lines · Nguyễn Thái Ngọc Duy
People seemed to support the idea of removing these #ifdef
NO_PTHREADS[1] so this is a complete series. I left the #ifdef inrun-command.candtransport-helper.cbecause those code looked complicated so perhaps we could clean them up later. Even these updated files could be updated more, I think, to reduce some code duplication, but I tried to keep the change here minimal.
Quoted 11 lines
Nguyễn Thái Ngọc Duy (10):
thread-utils: macros to unconditionally compile pthreads API
index-pack: remove #ifdefNO_PTHREADS
name-hash.c: remove #ifdefNO_PTHREADS
attr.c: remove #ifdefNO_PTHREADS
send-pack.c: remove #ifdefNO_PTHREADS
grep: remove #ifdefNO_PTHREADS
preload-index.c: remove #ifdefNO_PTHREADS
pack-objects: remove #ifdefNO_PTHREADSread-cache.c: remove #ifdefNO_PTHREADS
Clean uppthread_create()error handling
Compiling with NO_PTHREADS=1, I get (with gcc 8.2.0):
read-cache.c: In function ‘do_read_index’:
read-cache.c:1820:4: error: ‘copy_len’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
memcpy(ce->name, previous_ce->name, copy_len);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~read-cache.c:1749:9: note: ‘copy_len’ was declared here
size_t copy_len;
which seems wrong to me. It's always tied to expand_name_field being true. And curiously an earlier use doesn't trigger the warning. I wonder if some kind of tricky pointer aliasing in the intervening code makes it think that expand_name_field could change, but I sure don't see it.
I ran the tests under ASan/UBSan, since this series seems like it has a good chance of accidentally causing issues there, but didn't come up with any problems (except for the ones already fixed on pu by 8628ace269 (commit-reach: fix cast in compare_commits_by_gen(), 2018-10-01)).
-Peff
Quoted 30 lines · Jeff King
Quoted 9 lines · Nguyễn Thái Ngọc Duy
People seemed to support the idea of removing these #ifdef
NO_PTHREADS[1] so this is a complete series. I left the #ifdef inrun-command.candtransport-helper.cbecause those code looked complicated so perhaps we could clean them up later. Even these updated files could be updated more, I think, to reduce some code duplication, but I tried to keep the change here minimal.Quoted 11 lines
Nguyễn Thái Ngọc Duy (10):
thread-utils: macros to unconditionally compile pthreads API
index-pack: remove #ifdefNO_PTHREADS
name-hash.c: remove #ifdefNO_PTHREADS
attr.c: remove #ifdefNO_PTHREADS
send-pack.c: remove #ifdefNO_PTHREADS
grep: remove #ifdefNO_PTHREADS
preload-index.c: remove #ifdefNO_PTHREADS
pack-objects: remove #ifdefNO_PTHREADSread-cache.c: remove #ifdefNO_PTHREADS
Clean uppthread_create()error handlingCompiling with NO_PTHREADS=1, I get (with gcc 8.2.0):
read-cache.c: In function ‘do_read_index’:
read-cache.c:1820:4: error: ‘copy_len’ may be used uninitialized in this function [-Werror=maybe-uninitialized]memcpy(ce->name, previous_ce->name, copy_len); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Quoted 7 lines
read-cache.c:1749:9: note: ‘copy_len’ was declared here
size_tcopy_len;which seems wrong to me. It's always tied to
expand_name_fieldbeing true. And curiously an earlier use doesn't trigger the warning. I wonder if some kind of tricky pointer aliasing in the intervening code makes it think thatexpand_name_fieldcould change, but I sure don't see it.
I couldn't reproduce it with gcc 8.2.0 on gentoo (I tried all optimization levels just in case). I agree the warning is wrong. But I will initialize copy_len anyway to avoid this.
Signature · 1 line
Duy
v2 cover Reduce #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc DuyThe send-pack.c patch is dropped since it's tied to async code and like transport-helper.c (or more cleanups in index-pack.c) could be left for later.
I added one more patch to shut up -Wmaybe-uninitialized but since I could not reproduce it, Jeff would need to verify if it works for him.
Nguyễn Thái Ngọc Duy (10):
thread-utils: macros to unconditionally compile pthreads API
index-pack: remove #ifdef NO_PTHREADS
name-hash.c: remove #ifdef NO_PTHREADS
attr.c: remove #ifdef NO_PTHREADS
grep: remove #ifdef NO_PTHREADS
preload-index.c: remove #ifdef NO_PTHREADS
pack-objects: remove #ifdef NO_PTHREADS read-cache.c: remove #ifdef NO_PTHREADS
Clean up pthread_create() error handling read-cache.c: initialize copy_len to shut up gcc 8
Makefile | 2 +- attr.c | 14 --------- builtin/grep.c | 59 ++++++++++++++---------------------- builtin/index-pack.c | 68 +++++++++++------------------------------- builtin/pack-objects.c | 26 ++-------------- grep.c | 6 ---- grep.h | 6 ---- name-hash.c | 38 +++++++++-------------- pack-objects.h | 6 ---- preload-index.c | 23 +++++--------- read-cache.c | 53 ++++++++++++-------------------- run-command.c | 2 +- thread-utils.c | 48 +++++++++++++++++++++++++++++ thread-utils.h | 48 +++++++++++++++++++++++++++-- 14 files changed, 178 insertions(+), 221 deletions(-)
Range-diff against v1:
1: fd4926a83a ! 1: 2791c06629 thread-utils: macros to unconditionally compile pthreads API
@@ -146,6 +146,7 @@
+#define pthread_t int
+#define pthread_mutex_t int
+#define pthread_cond_t int
++#define pthread_key_t int
+
+#define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex) +#define pthread_mutex_lock(mutex)
2: 47b1f63fdd = 2: 61b3efceee index-pack: remove #ifdef NO_PTHREADS
3: a842b27e1e = 3: 93aa33eaa6 name-hash.c: remove #ifdef NO_PTHREADS
4: c0939283c3 = 4: 5d9441d8e2 attr.c: remove #ifdef NO_PTHREADS
5: 6c6f3a0d28 < -: ---------- send-pack.c: remove #ifdef NO_PTHREADS
6: 9f6e259fb4 ! 5: 834b19d5c6 grep: remove #ifdef NO_PTHREADS
@@ -13,6 +13,16 @@
static pthread_t *threads;
/* We use one producer thread and THREADS consumer
+@@
+ int hit = 0;
+ int i;
+
++ if (!HAVE_THREADS)
++ return 0;
++
+ grep_lock();
+ all_work_added = 1;
+
@@
return hit;
@@ -136,15 +146,6 @@
if (show_in_pager && (cached || list.nr))
die(_("--open-files-in-pager only works on the worktree"));
-@@
- hit = grep_objects(&opt, &pathspec, &list);
- }
-
-- if (num_threads)
-+ if (HAVE_THREADS && num_threads)
- hit |= wait_all();
- if (hit && show_in_pager)
- run_pager(&opt, prefix);
diff --git a/grep.c b/grep.c
--- a/grep.c
7: 97fb2fdba4 = 6: 10969f86ae preload-index.c: remove #ifdef NO_PTHREADS
8: b8aeb2776b = 7: 7e83190364 pack-objects: remove #ifdef NO_PTHREADS
9: 40a9bfb8f2 = 8: 404e9d9041 read-cache.c: remove #ifdef NO_PTHREADS
10: 0bb0c667ff = 9: 046008b4ef Clean up pthread_create() error handling
-: ---------- > 10: 2ad9554687 read-cache.c: initialize copy_len to shut up gcc 8Signature · 1 line
2.19.1.647.g708186aaf9
PATCH v2 1/10 thread-utils: macros to unconditionally compile pthreads API
When built with NO_PTHREADS, the macros are used make the code build even though pthreads header and library may be missing. The code can still have different code paths for no threads support with HAVE_THREADS variable.
There are of course impacts on no-pthreads builds:
- data structure may get slightly bigger because all the mutexes and pthread_t are present (as an int)
- code execution is not impacted much. Locking (in hot path) is no-op. Other wrapper function calls really should not matter much.
- the binary size grows bigger because of threaded code. But at least on Linux this does not matter, if some code is not executed, it's not mapped in memory.
This is a preparation step to remove "#ifdef NO_PTHREADS" in the code mostly because of maintainability. As Jeff put it
Quoted 4 lines
it's probably OK to stop thinking of it as "non-threaded platforms are the default and must pay zero cost" and more as "threaded platforms are the default, and non-threaded ones are OK to pay a small cost as long as they still work".
--- Makefile | 2 +- thread-utils.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ thread-utils.h | 48 +++++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 94 insertions(+), 4 deletions(-)
Makefile +1 -1
@@ -991,6 +991,7 @@ LIB_OBJS += sub-process.o LIB_OBJS += symlinks.o LIB_OBJS += tag.o LIB_OBJS += tempfile.o +LIB_OBJS += thread-utils.o LIB_OBJS += tmp-objdir.o LIB_OBJS += trace.o LIB_OBJS += trailer.o
@@ -1674,7 +1675,6 @@ ifdef NO_PTHREADS else BASIC_CFLAGS += $(PTHREAD_CFLAGS) EXTLIBS += $(PTHREAD_LIBS) - LIB_OBJS += thread-utils.o endif ifdef HAVE_PATHS_H
thread-utils.c +48 -0
68 lines not on this page · raw message
thread-utils.h +45 -3
57 lines not on this page · raw message
Signature · 1 line
2.19.1.647.g708186aaf9
PATCH v2 2/10 index-pack: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- builtin/index-pack.c | 68 ++++++++++++-------------------------------- 1 file changed, 18 insertions(+), 50 deletions(-)
builtin/index-pack.c +18 -50
@@ -42,9 +42,7 @@ struct base_data { };struct thread_local { -#ifndef NO_PTHREADS pthread_t thread; -#endif struct base_data *base_cache;size_t base_cache_used;int pack_fd;
@@ -98,8 +96,6 @@ static uint32_t input_crc32;static int input_fd, output_fd;static const char *curr_pack; -#ifndef NO_PTHREADS -static struct thread_local *thread_data;static int nr_dispatched;static int threads_active;
@@ -179,26 +175,6 @@ static void cleanup_thread(void)free (thread_data); } -#else - -#define read_lock () -#define read_unlock () - -#define counter_lock () -#define counter_unlock () - -#define work_lock () -#define work_unlock () - -#define deepest_delta_lock () -#define deepest_delta_unlock () - -#define type_cas_lock () -#define type_cas_unlock () - -#endif - -static int mark_link (struct object *obj,int type,void *data,struct fsck_options *options) {if (!obj)
@@ -364,22 +340,20 @@ static NORETURN void bad_object(off_t offset, const char *format, ...)static inline struct thread_local *get_thread_data (void ) { -#ifndef NO_PTHREADS -if (threads_active) -return pthread_getspecific (key); -assert (!threads_active && -"This should only be reached when all threads are gone" ); -#endif +if (HAVE_THREADS ) { +if (threads_active) +return pthread_getspecific (key); +assert (!threads_active && +"This should only be reached when all threads are gone" ); + }return ¬hread_data; } -#ifndef NO_PTHREADS static void set_thread_data (struct thread_local *data) {if (threads_active)pthread_setspecific (key, data); } -#endif static struct base_data *alloc_base_data (void ) {
@@ -1092,7 +1066,6 @@ static void resolve_base(struct object_entry *obj)find_unresolved_deltas (base_obj); } -#ifndef NO_PTHREADS static void *threaded_second_pass (void *data) {set_thread_data (data);
@@ -1116,7 +1089,6 @@ static void *threaded_second_pass(void *data) }return NULL ; } -#endif /* * First pass:
@@ -1213,7 +1185,6 @@ static void resolve_deltas(void) progress =start_progress (_ ("Resolving deltas" ), nr_ref_deltas + nr_ofs_deltas); -#ifndef NO_PTHREADS nr_dispatched =0 ;if (nr_threads >1 ||getenv ("GIT_FORCE_THREADS" )) {init_thread ();
@@ -1229,7 +1200,6 @@ static void resolve_deltas(void)cleanup_thread ();return ; } -#endif for (i =0 ; i < nr_objects; i++) {struct object_entry *obj = &objects[i];
@@ -1531,11 +1501,11 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)if (nr_threads <0 )die (_ ("invalid number of threads specified (%d)" ), nr_threads); -#ifdef NO_PTHREADS -if (nr_threads !=1 ) -warning (_ ("no threads support, ignoring %s" ), k); - nr_threads =1 ; -#endif +if (!HAVE_THREADS ) { +if (nr_threads !=1 ) +warning (_ ("no threads support, ignoring %s" ), k); + nr_threads =1 ; + }return 0 ; }return git_default_config (k, v, cb);
@@ -1723,12 +1693,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) nr_threads =strtoul (arg+10 , &end,0 );if (!arg[10 ] || *end || nr_threads <0 )usage (index_pack_usage); -#ifdef NO_PTHREADS -if (nr_threads !=1 ) -warning (_ ("no threads support, " -"ignoring %s" ), arg); - nr_threads =1 ; -#endif +if (!HAVE_THREADS ) { +if (nr_threads !=1 ) +warning (_ ("no threads support, " +"ignoring %s" ), arg); + nr_threads =1 ; + } }else if (starts_with (arg,"--pack_header=" )) {struct pack_header *hdr;char *c;
@@ -1791,14 +1761,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)if (strict) opts.flags |=WRITE_IDX_STRICT ; -#ifndef NO_PTHREADS -if (!nr_threads) { +if (HAVE_THREADS && !nr_threads) { nr_threads =online_cpus ();/* An experiment showed that more threads does not mean faster */ if (nr_threads >3 ) nr_threads =3 ; } -#endif curr_pack =open_pack_file (pack_name);parse_pack_header ();
Signature · 1 line
2.19.1.647.g708186aaf9
PATCH v2 3/10 name-hash.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- name-hash.c | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-)
name-hash.c +4 -18
@@ -7,6 +7,7 @@*/ #define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" +#include "thread-utils.h" struct dir_entry {struct hashmap_entry ent;
@@ -131,22 +132,6 @@ static int cache_entry_cmp(const void *unused_cmp_data,static int lazy_try_threaded =1 ;static int lazy_nr_dir_threads; -#ifdef NO_PTHREADS - -static inline int lookup_lazy_params (struct index_state *istate) -{ -return 0 ; -} - -static inline void threaded_lazy_init_name_hash ( -struct index_state *istate) -{ -} - -#else - -#include "thread-utils.h" -/* * Set a minimum number of cache_entries that we will handle per * thread and use that to decide how many threads to run (upto
@@ -516,6 +501,9 @@ static void threaded_lazy_init_name_hash(struct lazy_dir_thread_data *td_dir;struct lazy_name_thread_data *td_name; +if (!HAVE_THREADS ) +return ; + k_start =0 ; nr_each =DIV_ROUND_UP (istate->cache_nr, lazy_nr_dir_threads);
@@ -574,8 +562,6 @@ static void threaded_lazy_init_name_hash(free (lazy_entries); } -#endif -static void lazy_init_name_hash (struct index_state *istate) {
Signature · 1 line
2.19.1.647.g708186aaf9
PATCH v2 4/10 attr.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- attr.c | 14 -------------- 1 file changed, 14 deletions(-)
attr.c +0 -14
@@ -41,23 +41,17 @@ const char *git_attr_name(const struct git_attr *attr)struct attr_hashmap {struct hashmap map; -#ifndef NO_PTHREADS pthread_mutex_t mutex; -#endif };static inline void hashmap_lock (struct attr_hashmap *map) { -#ifndef NO_PTHREADS pthread_mutex_lock (&map->mutex); -#endif }static inline void hashmap_unlock (struct attr_hashmap *map) { -#ifndef NO_PTHREADS pthread_mutex_unlock (&map->mutex); -#endif }/*
@@ -498,23 +492,17 @@ static struct check_vector {size_t nr;size_t alloc;struct attr_check **checks; -#ifndef NO_PTHREADS pthread_mutex_t mutex; -#endif } check_vector;static inline void vector_lock (void ) { -#ifndef NO_PTHREADS pthread_mutex_lock (&check_vector.mutex); -#endif }static inline void vector_unlock (void ) { -#ifndef NO_PTHREADS pthread_mutex_unlock (&check_vector.mutex); -#endif }static void check_vector_add (struct attr_check *c)
@@ -1181,8 +1169,6 @@ void git_all_attrs(const struct index_state *istate,void attr_start (void ) { -#ifndef NO_PTHREADS pthread_mutex_init (&g_attr_hashmap.mutex,NULL );pthread_mutex_init (&check_vector.mutex,NULL ); -#endif }
Signature · 1 line
2.19.1.647.g708186aaf9
PATCH v2 5/10 grep: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- builtin/grep.c | 59 +++++++++++++++++++------------------------------- grep.c | 6 ----- grep.h | 6 ----- 3 files changed, 22 insertions(+), 49 deletions(-)
builtin/grep.c +22 -37
@@ -34,7 +34,6 @@ static int recurse_submodules;#define GREP_NUM_THREADS_DEFAULT 8static int num_threads; -#ifndef NO_PTHREADS static pthread_t *threads;/* We use one producer thread and THREADS consumer
@@ -234,6 +233,9 @@ static int wait_all(void)int hit =0 ;int i; +if (!HAVE_THREADS ) +return 0 ; +grep_lock (); all_work_added =1 ;
@@ -265,13 +267,6 @@ static int wait_all(void)return hit; } -#else /* !NO_PTHREADS */ - -static int wait_all (void ) -{ -return 0 ; -} -#endif static int grep_cmd_config (const char *var,const char *value,void *cb) {
@@ -284,8 +279,7 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)if (num_threads <0 )die (_ ("invalid number of threads specified (%d) for %s" ), num_threads, var); -#ifdef NO_PTHREADS - else if (num_threads && num_threads != 1) { +else if (!HAVE_THREADS && num_threads && num_threads !=1 ) {/* * TRANSLATORS: %s is the configuration * variable for tweaking threads, currently
@@ -294,7 +288,6 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)warning (_ ("no threads support, ignoring %s" ), var); num_threads =0 ; } -#endif }if (!strcmp (var,"submodule.recurse" ))
@@ -330,17 +323,14 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,grep_source_init (&gs,GREP_SOURCE_OID , pathbuf.buf, path, oid);strbuf_release (&pathbuf); -#ifndef NO_PTHREADS -if (num_threads) { +if (HAVE_THREADS && num_threads) {/* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear() */ add_work (opt, &gs);return 0 ; - } else -#endif - { + }else {int hit; hit =grep_source (opt, &gs);
@@ -363,17 +353,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)grep_source_init (&gs,GREP_SOURCE_FILE , buf.buf, filename, filename);strbuf_release (&buf); -#ifndef NO_PTHREADS -if (num_threads) { +if (HAVE_THREADS && num_threads) {/* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear() */ add_work (opt, &gs);return 0 ; - } else -#endif - { + }else {int hit; hit =grep_source (opt, &gs);
@@ -1038,20 +1025,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix) pathspec.recursive =1 ; pathspec.recurse_submodules = !!recurse_submodules; -#ifndef NO_PTHREADS -if (list.nr || cached || show_in_pager) - num_threads =0 ; -else if (num_threads ==0 ) - num_threads =GREP_NUM_THREADS_DEFAULT ; -else if (num_threads <0 ) -die (_ ("invalid number of threads specified (%d)" ), num_threads); -if (num_threads ==1 ) +if (HAVE_THREADS ) { +if (list.nr || cached || show_in_pager) + num_threads =0 ; +else if (num_threads ==0 ) + num_threads =GREP_NUM_THREADS_DEFAULT ; +else if (num_threads <0 ) +die (_ ("invalid number of threads specified (%d)" ), num_threads); +if (num_threads ==1 ) + num_threads =0 ; + }else { +if (num_threads) +warning (_ ("no threads support, ignoring --threads" )); num_threads =0 ; -#else -if (num_threads) -warning (_ ("no threads support, ignoring --threads" )); - num_threads =0 ; -#endif + }if (!num_threads)/*
@@ -1062,15 +1049,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)*/ compile_grep_patterns (&opt); -#ifndef NO_PTHREADS -if (num_threads) { +if (HAVE_THREADS && num_threads) {if (!(opt.name_only || opt.unmatch_name_only || opt.count) && (opt.pre_context || opt.post_context || opt.file_break || opt.funcbody)) skip_first_line =1 ;start_threads (&opt); } -#endif if (show_in_pager && (cached || list.nr))die (_ ("--open-files-in-pager only works on the worktree" ));
grep.c +0 -6
@@ -1513,7 +1513,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol, } } -#ifndef NO_PTHREADS int grep_use_locks;/*
@@ -1539,11 +1538,6 @@ static inline void grep_attr_unlock(void)*/ pthread_mutex_t grep_read_mutex; -#else -#define grep_attr_lock () -#define grep_attr_unlock () -#endif -static int match_funcname (struct grep_opt *opt,struct grep_source *gs,char *bol,char *eol) {xdemitconf_t *xecfg = opt->priv;
grep.h +0 -6
@@ -229,7 +229,6 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs);extern struct grep_opt *grep_opt_dup (const struct grep_opt *opt);extern int grep_threads_ok (const struct grep_opt *opt); -#ifndef NO_PTHREADS /* * Mutex used around access to the attributes machinery if * opt->use_threads. Must be initialized/destroyed by callers!
@@ -250,9 +249,4 @@ static inline void grep_read_unlock(void) pthread_mutex_unlock(&grep_read_mutex); } -#else -#define grep_read_lock () -#define grep_read_unlock () -#endif -#endif
Signature · 1 line
2.19.1.647.g708186aaf9
PATCH v2 6/10 preload-index.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- preload-index.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)
preload-index.c +2 -13
@@ -7,17 +7,7 @@#include "fsmonitor.h" #include "config.h" #include "progress.h" - -#ifdef NO_PTHREADS -static void preload_index (struct index_state *index, -const struct pathspec *pathspec, -unsigned int refresh_flags) -{ - ;/* nothing */ -} -#else - -#include <pthread.h> +#include "thread-utils.h" /* * Mostly randomly chosen maximum thread counts: we
@@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,struct thread_data data[MAX_PARALLEL ];struct progress_data pd; -if (!core_preload_index) +if (!HAVE_THREADS || !core_preload_index)return ; threads = index->cache_nr /THREAD_COST ;
@@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,trace_performance_leave ("preload index" ); } -#endif int read_index_preload(struct index_state *index, const struct pathspec *pathspec,
Signature · 1 line
2.19.1.647.g708186aaf9
Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS
Quoted 39 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- preload-index.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)preload-index.c +2
-12@@ -7,17 +7,7 @@#include "fsmonitor.h" #include "config.h" #include "progress.h" - -#ifdef NO_PTHREADS -static void preload_index (struct index_state *index, -const struct pathspec *pathspec, -unsigned int refresh_flags) -{ - ;/* nothing */ -} -#else - -#include <pthread.h> +#include "thread-utils.h" /* * Mostly randomly chosen maximum thread counts: we @@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,struct thread_data data[MAX_PARALLEL ];struct progress_data pd; -if (!core_preload_index) +if (!HAVE_THREADS || !core_preload_index)return ; threads = index->cache_nr /THREAD_COST ;
preload-index.c static void preload_index(struct index_state *index, · line 146
Quoted 7 lines
preload-index.c +0
-1@@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,trace_performance_leave ("preload index" ); } -#endif int read_index_preload(struct index_state *index, const struct pathspec *pathspec,
In the theory that code speaks louder than comments, here is the patch I used when testing out the idea of getting rid of NO_PTHREADS:
git diff head~1 preload-index.c
preload-index.c +2 -12
@@ -7,17 +7,7 @@#include "fsmonitor.h" #include "config.h" #include "progress.h" - -#ifdef NO_PTHREADS -static void preload_index (struct index_state *index, -const struct pathspec *pathspec, -unsigned int refresh_flags) -{ - ;/* nothing */ -} -#else - -#include <pthread.h> +#include "thread-utils.h" /* * Mostly randomly chosen maximum thread counts: we
@@ -104,7 +94,7 @@ static void preload_index(struct index_state *index,const struct pathspec *pathspec, unsigned int refresh_flags) { - int threads, i, work, offset; + int cpus, threads, i, work, offset;struct thread_data data[MAX_PARALLEL ];struct progress_data pd;
@@ -114,6 +104,9 @@ static void preload_index(struct index_state *index, threads = index->cache_nr /THREAD_COST ; if ((index->cache_nr > 1) && (threads < 2) &&
git_env_bool("GIT_TEST_PRELOAD_INDEX", 0))
threads = 2;
+ cpus = online_cpus();
+ if (threads > cpus)
+ threads = cpus;
if (threads < 2)
return;
trace_performance_enter();@@ -151,7 +144,6 @@ static void preload_index(struct index_state *index,
trace_performance_leave("preload index"); }
-#endif
int read_index_preload(struct index_state *index,
const struct pathspec *pathspec,
Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS
Quoted 11 lines · Ben Peart
(quoted diff) +3
-0@@ -114,6 +104,9 @@ static void preload_index(struct index_state *index, threads = index->cache_nr / THREAD_COST; if ((index->cache_nr > 1) && (threads < 2) && git_env_bool("GIT_TEST_PRELOAD_INDEX", 0)) threads = 2; + cpus = online_cpus(); + if (threads > cpus) + threads = cpus; if (threads < 2) return;trace_performance_enter();
Capping the number of threads to online_cpus() does not always make sense. In this case (or at least the original use case where we stat() over nfs) we want to issue as many requests to nfs server as possible to reduce latency and it has nothing to do with how many cores we have. Using more threads than cores might make sense since threads are blocked by nfs client, but we still want to send more to the server.
Signature · 1 line
Duy
Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS
Quoted 11 lines · Duy Nguyen
Quoted 10 lines · Ben Peart
(quoted diff) +3
-0@@ -114,6 +104,9 @@ static void preload_index(struct index_state *index, threads = index->cache_nr / THREAD_COST; if ((index->cache_nr > 1) && (threads < 2) && git_env_bool("GIT_TEST_PRELOAD_INDEX", 0)) threads = 2; + cpus = online_cpus(); + if (threads > cpus) + threads = cpus; if (threads < 2) return;
Quoted 9 lines
Quoted 1 line
trace_performance_enter();Capping the number of threads to
online_cpus()does not always make sense. In this case (or at least the original use case where westat()over nfs) we want to issue as many requests to nfs server as possible to reduce latency and it has nothing to do with how many cores we have. Using more threads than cores might make sense since threads are blocked by nfs client, but we still want to send more to the server.
That makes sense. Some test will be necessary then.
I guess HAVE_THREADS is functionally the same as online_cpus() == 1. For some reason, I prefer the latter - probably because I'm typically already calling it and so it doesn't feel like a 'special' value/test I have to remember. I just know I need to make sure the logic works correctly with any number of cps greater than zero. :-)
Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS
Quoted 22 lines · Ben Peart
Quoted 11 lines · Duy Nguyen
Quoted 10 lines · Ben Peart
(quoted diff) +3
-0@@ -114,6 +104,9 @@ static void preload_index(struct index_state *index, threads = index->cache_nr / THREAD_COST; if ((index->cache_nr > 1) && (threads < 2) && git_env_bool("GIT_TEST_PRELOAD_INDEX", 0)) threads = 2; + cpus = online_cpus(); + if (threads > cpus) + threads = cpus; if (threads < 2) return;Quoted 9 lines
Quoted 1 line
trace_performance_enter();Capping the number of threads to
online_cpus()does not always make sense. In this case (or at least the original use case where westat()over nfs) we want to issue as many requests to nfs server as possible to reduce latency and it has nothing to do with how many cores we have. Using more threads than cores might make sense since threads are blocked by nfs client, but we still want to send more to the server.
Quoted 7 lines
That makes sense. Some test will be necessary then.
I guess
HAVE_THREADSis functionally the same asonline_cpus()== 1. For some reason, I prefer the latter - probably because I'm typically already calling it and so it doesn't feel like a 'special' value/test I have to remember. I just know I need to make sure the logic works correctly with any number of cps greater than zero. :-)
I don't think they're functionally the same. If you see online_cpus() == 1, you are in one of two cases:
- the system supports threads, but CPU-bound tasks should stick to a single thread
- the system does not even support threading, and calling pthread_create() will give you ENOSYS
In the first case, if your task is _not_ CPU bound (i.e., the stat() thing), then you want to distinguish those cases.
I'm not sure if I'm violently agreeing, but it just wasn't clear to me from the above discussion that we all agreed that HAVE_THREADS is still necessary in some cases.
(I do think in cases where it is not, that we would do just as well to avoid it; if that is all you were saying, then I agree. ;) ).
-Peff
PATCH v2 7/10 pack-objects: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- builtin/pack-objects.c | 26 ++------------------------ pack-objects.h | 6 ------ 2 files changed, 2 insertions(+), 30 deletions(-)
builtin/pack-objects.c +2 -24
@@ -1953,8 +1953,6 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,return 0 ; } -#ifndef NO_PTHREADS -/* Protect access to object database */ static pthread_mutex_t read_mutex;#define read_lock () pthread_mutex_lock(&read_mutex)
@@ -1979,16 +1977,6 @@ static pthread_mutex_t progress_mutex;* ahead in the list because they can be stolen and would need * progress_mutex for protection. */ -#else - -#define read_lock () (void)0 -#define read_unlock () (void)0 -#define cache_lock () (void)0 -#define cache_unlock () (void)0 -#define progress_lock () (void)0 -#define progress_unlock () (void)0 - -#endif /* * Return the size of the object without doing any delta
@@ -2347,8 +2335,6 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,free (array); } -#ifndef NO_PTHREADS -static void try_to_free_from_threads (size_t size) {read_lock ();
@@ -2578,10 +2564,6 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,free (p); } -#else -#define ll_find_deltas (l, s, w, d, p) find_deltas(l, &s, w, d, p) -#endif -static void add_tag_chain (const struct object_id *oid) {struct tag *tag;
@@ -2734,12 +2716,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)if (delta_search_threads <0 )die (_ ("invalid number of threads specified (%d)" ), delta_search_threads); -#ifdef NO_PTHREADS -if (delta_search_threads !=1 ) { +if (!HAVE_THREADS && delta_search_threads !=1 ) {warning (_ ("no threads support, ignoring %s" ), k); delta_search_threads =0 ; } -#endif return 0 ; }if (!strcmp (k,"pack.indexversion" )) {
@@ -3402,10 +3382,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)if (!delta_search_threads)/* --threads=0 means autodetect */ delta_search_threads =online_cpus (); -#ifdef NO_PTHREADS -if (delta_search_threads !=1 ) +if (!HAVE_THREADS && delta_search_threads !=1 )warning (_ ("no threads support, ignoring --threads" )); -#endif if (!pack_to_stdout && !pack_size_limit) pack_size_limit = pack_size_limit_cfg;if (pack_to_stdout && pack_size_limit)
pack-objects.h +0 -6
@@ -145,9 +145,7 @@ struct packing_data {struct packed_git **in_pack_by_idx;struct packed_git **in_pack; -#ifndef NO_PTHREADS pthread_mutex_t lock; -#endif /* * This list contains entries for bases which we know the other side
@@ -169,15 +167,11 @@ void prepare_packing_data(struct packing_data *pdata);static inline void packing_data_lock (struct packing_data *pdata) { -#ifndef NO_PTHREADS pthread_mutex_lock (&pdata->lock); -#endif }static inline void packing_data_unlock (struct packing_data *pdata) { -#ifndef NO_PTHREADS pthread_mutex_unlock (&pdata->lock); -#endif } struct object_entry *packlist_alloc(struct packing_data *pdata,
Signature · 1 line
2.19.1.647.g708186aaf9
PATCH v2 8/10 read-cache.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- read-cache.c | 49 ++++++++++++++++++------------------------------- 1 file changed, 18 insertions(+), 31 deletions(-)
read-cache.c +18 -31
@@ -1920,19 +1920,15 @@ struct index_entry_offset_tablestruct 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, conreturn 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_tastrbuf_add (sb, &buffer,sizeof (uint32_t )); } } -#endif
Signature · 1 line
2.19.1.647.g708186aaf9
Re: [PATCH v2 08/10] read-cache.c: remove #ifdef NO_PTHREADS
Quoted 11 lines · Nguyễn Thái Ngọc Duy
read-cache.c +2
-8-#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;
read-cache.c int do_read_index(struct index_state *istate, const char *path, int must_exist) · line 2187
Quoted 10 lines
read-cache.c +9
-0+/* 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 ; }
I'd have thought we could just rely on online_cpus() returning 1 here to avoid having to ask "do we even have thread support?". But I guess that TODO comment implies that we might one day two 2 threads on a single CPU.
-Peff
Re: [PATCH v2 08/10] read-cache.c: remove #ifdef NO_PTHREADS
Quoted 29 lines · Jeff King
Quoted 11 lines · Nguyễn Thái Ngọc Duy
read-cache.c +2
-8-#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;read-cache.c int do_read_index(struct index_state *istate, const char *path, int must_exist) · line 2187
Quoted 10 lines
read-cache.c +9
-0+/* 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 ; }I'd have thought we could just rely on
online_cpus()returning 1 here to avoid having to ask "do we even have thread support?". But I guess that TODO comment implies that we might one day two 2 threads on a single CPU.
Quoted 2 lines
-Peff
See my earlier response - the HAVE_THREADS tests are not needed.
We can remove the "TODO" comment - I tested it and it wasn't faster to have more threads than CPUs.
Re: [PATCH v2 08/10] read-cache.c: remove #ifdef NO_PTHREADS
In reply to Jeff KingQuoted 29 lines · Jeff King
Quoted 11 lines · Nguyễn Thái Ngọc Duy
(quoted diff) +2
-8-#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;Review
Quoted 10 lines
(quoted diff) +9
-0+ /* 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; }I'd have thought we could just rely on
online_cpus()returning 1 here to avoid having to ask "do we even have thread support?". But I guess that TODO comment implies that we might one day two 2 threads on a single CPU.
Quoted 2 lines
-Peff
And here is the patch I created when testing out the idea of removing NO_PTHREADS. It doesn't require any 'HAVE_THREADS' tests.
read-cache.c +0 -1
@@ -1920,19 +1920,15 @@ struct index_entry_offset_tablestruct 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, structindex_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, intmust_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,16 +2172,12 @@ 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();
-
- /* TODO: does creating more threads than cores help? */
- if (!nr_threads) {
+ if (!nr_threads)
nr_threads = istate->cache_nr / THREAD_COST;
- cpus = online_cpus();
- if (nr_threads > cpus)
- nr_threads = cpus;
- }
+ cpus = online_cpus();
+ if (nr_threads > cpus)+ nr_threads = cpus;
if (nr_threads > 1) {
extension_offset = read_eoie_extension(mmap, mmap_size);
@@ -2219,22 +2206,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) {
int ret = pthread_join(p.pthread, NULL);
if (ret)
die(_("unable to join load_index_extensions thread: %s"), strerror(ret));
- }
-#endif
- if (!extension_offset) {
+ } else {
p.src_offset = src_offset;
load_index_extensions(&p);
}@@ -2756,7 +2737,6 @@ 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 (nr_threads != 1) {
int ieot_blocks, cpus;@@ -2787,7 +2767,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,7 +2850,6 @@ 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) {
struct strbuf sb = STRBUF_INIT;@@ -2883,7 +2861,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 +3446,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 +3518,3 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta
strbuf_add(sb, &buffer, sizeof(uint32_t));
} }
-#endif
PATCH v2 9/10 Clean up pthread_create() error handling
In reply to Nguyễn Thái Ngọc DuyNormally pthread_create() rarely fails and sometimes error handling can be neglected. But with new pthreads wrapper, pthread_create() will return ENOSYS on a system without thread support.
Threaded code _is_ protected by HAVE_THREADS and pthread_create() should never run in the first place. But the situation could change in the future and bugs may sneak in. Make sure that all pthread_create() checks and handles the result.
While at there, mark these strings for translation if they aren't.
--- name-hash.c | 16 ++++++++++------ preload-index.c | 8 ++++++-- run-command.c | 2 +- 3 files changed, 17 insertions(+), 9 deletions(-)
name-hash.c +10 -6
@@ -494,6 +494,7 @@ static inline void lazy_update_dir_ref_counts(static void threaded_lazy_init_name_hash (struct index_state *istate) { +int err;int nr_each;int k_start;int t;
@@ -526,8 +527,9 @@ static void threaded_lazy_init_name_hash(if (k_start > istate->cache_nr) k_start = istate->cache_nr; td_dir_t->k_end = k_start; -if (pthread_create (&td_dir_t->pthread,NULL , lazy_dir_thread_proc, td_dir_t)) -die ("unable to create lazy_dir_thread" ); + err =pthread_create (&td_dir_t->pthread,NULL , lazy_dir_thread_proc, td_dir_t); +if (err) +die (_ ("unable to create lazy_dir thread: %s" ),strerror (err)); }for (t =0 ; t < lazy_nr_dir_threads; t++) {struct lazy_dir_thread_data *td_dir_t = td_dir + t;
@@ -547,13 +549,15 @@ static void threaded_lazy_init_name_hash(*/ td_name->istate = istate; td_name->lazy_entries = lazy_entries; -if (pthread_create (&td_name->pthread,NULL , lazy_name_thread_proc, td_name)) -die ("unable to create lazy_name_thread" ); + err =pthread_create (&td_name->pthread,NULL , lazy_name_thread_proc, td_name); +if (err) +die (_ ("unable to create lazy_name thread: %s" ),strerror (err));lazy_update_dir_ref_counts (istate, lazy_entries); -if (pthread_join (td_name->pthread,NULL )) -die ("unable to join lazy_name_thread" ); + err =pthread_join (td_name->pthread,NULL ); +if (err) +die (_ ("unable to join lazy_name thread: %s" ),strerror (err));cleanup_dir_mutex ();
preload-index.c +6 -2
@@ -121,6 +121,8 @@ static void preload_index(struct index_state *index,for (i =0 ; i < threads; i++) {struct thread_data *p = data+i; +int err; + p->index = index;if (pathspec)copy_pathspec (&p->pathspec, pathspec);
@@ -129,8 +131,10 @@ static void preload_index(struct index_state *index,if (pd.progress) p->progress = &pd; offset += work; -if (pthread_create (&p->pthread,NULL , preload_thread, p)) -die ("unable to create threaded lstat" ); + err =pthread_create (&p->pthread,NULL , preload_thread, p); + +if (err) +die (_ ("unable to create threaded lstat: %s" ),strerror (err)); }for (i =0 ; i < threads; i++) {struct thread_data *p = data+i;
run-command.c +1 -1
@@ -1213,7 +1213,7 @@ int start_async(struct async *async) {int err =pthread_create (&async->tid,NULL , run_thread, async);if (err) { -error_errno ("cannot create thread" ); +error (_ ("cannot create async thread: %s" ),strerror (err)); goto error; } }
Signature · 1 line
2.19.1.647.g708186aaf9
PATCH v2 10/10 read-cache.c: initialize copy_len to shut up gcc 8
In reply to Nguyễn Thái Ngọc DuyIt was reported that when building with NO_PTHREADS=1, -Wmaybe-uninitialized is triggered. Just initialize the variable from the beginning to shut the compiler up (because this warning is enabled in config.dev)
--- read-cache.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
read-cache.c +1 -3
@@ -1746,7 +1746,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,size_t len;const char *name;unsigned int flags; -size_t copy_len; +size_t copy_len =0 ;/* * Adjacent cache entries tend to share the leading paths, so it makes * sense to only store the differences in later entries. In the v4
@@ -1786,8 +1786,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,die (_ ("malformed name field in the index, near path '%s'" ), previous_ce->name); copy_len = previous_len - strip_len; - }else { - copy_len =0 ; } name = (const char *)cp; }
Signature · 1 line
2.19.1.647.g708186aaf9
Re: [PATCH v2 10/10] read-cache.c: initialize copy_len to shut up gcc 8
Quoted 24 lines · Nguyễn Thái Ngọc Duy
It was reported that when building with NO_PTHREADS=1, -Wmaybe-uninitialized is triggered. Just initialize the variable from the beginning to shut the compiler up (because this warning is enabled in config.dev)
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- read-cache.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)read-cache.c +1
-1@@ -1746,7 +1746,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,size_t len;const char *name;unsigned int flags; -size_t copy_len; +size_t copy_len =0 ;/* * Adjacent cache entries tend to share the leading paths, so it makes * sense to only store the differences in later entries. In the v4
read-cache.c static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool, · line 1791
Quoted 8 lines
read-cache.c +0
-2@@ -1786,8 +1786,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,die (_ ("malformed name field in the index, near path '%s'" ), previous_ce->name); copy_len = previous_len - strip_len; - }else { - copy_len =0 ; } name = (const char *)cp; }
Yes, this silences the compiler warning I saw (and is exactly the same patch I wrote to get past it the other day ;) ).
-Peff
v3 cover Reduce #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc DuyChanges since v2
- more cleanups in grep.c, read-cache.c and index-pack.c
- the send-pack.c changes are back, but this time I just add async_with_fork() to move NO_PTHREADS back in run-command.c
For grep.c and read-cache.c, changes are split in two patches. The first one is a dumb, mechanical conversion from #ifdef to if and is straightforward. The second one makes "no thread" use "one thread" code path and needs more careful review.
Diff against nd/pthreads
builtin/grep.c +27 -31
@@ -69,13 +69,11 @@ static pthread_mutex_t grep_mutex;static inline void grep_lock (void ) { -assert (num_threads);pthread_mutex_lock (&grep_mutex); }static inline void grep_unlock (void ) { -assert (num_threads);pthread_mutex_unlock (&grep_mutex); }
@@ -234,7 +232,7 @@ static int wait_all(void)int i;if (!HAVE_THREADS ) -return 0 ; +BUG ("Never call this function unless you have started threads" );grep_lock (); all_work_added =1 ;
@@ -279,14 +277,14 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)if (num_threads <0 )die (_ ("invalid number of threads specified (%d) for %s" ), num_threads, var); -else if (!HAVE_THREADS && num_threads && num_threads !=1 ) { +else if (!HAVE_THREADS && num_threads >1 ) {/* * TRANSLATORS: %s is the configuration * variable for tweaking threads, currently * grep.threads */ warning (_ ("no threads support, ignoring %s" ), var); - num_threads =0 ; + num_threads =1 ; } }
@@ -323,7 +321,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,grep_source_init (&gs,GREP_SOURCE_OID , pathbuf.buf, path, oid);strbuf_release (&pathbuf); -if (HAVE_THREADS && num_threads) { +if (num_threads >1 ) {/* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear()
@@ -353,7 +351,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)grep_source_init (&gs,GREP_SOURCE_FILE , buf.buf, filename, filename);strbuf_release (&buf); -if (HAVE_THREADS && num_threads) { +if (num_threads >1 ) {/* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear()
@@ -1025,36 +1023,34 @@ int cmd_grep(int argc, const char **argv, const char *prefix) pathspec.recursive =1 ; pathspec.recurse_submodules = !!recurse_submodules; -if (HAVE_THREADS ) { -if (list.nr || cached || show_in_pager) - num_threads =0 ; -else if (num_threads ==0 ) - num_threads =GREP_NUM_THREADS_DEFAULT ; -else if (num_threads <0 ) -die (_ ("invalid number of threads specified (%d)" ), num_threads); -if (num_threads ==1 ) - num_threads =0 ; +if (list.nr || cached || show_in_pager) { +if (num_threads >1 ) +warning (_ ("invalid option combination, ignoring --threads" )); + num_threads =1 ; + }else if (!HAVE_THREADS && num_threads >1 ) { +warning (_ ("no threads support, ignoring --threads" )); + num_threads =1 ; + }else if (num_threads <0 ) +die (_ ("invalid number of threads specified (%d)" ), num_threads); +else if (num_threads ==0 ) + num_threads =HAVE_THREADS ?GREP_NUM_THREADS_DEFAULT :1 ; + +if (num_threads >1 ) { +if (!HAVE_THREADS ) +BUG ("Somebody got num_threads calculation wrong!" ); +if (!(opt.name_only || opt.unmatch_name_only || opt.count) + && (opt.pre_context || opt.post_context || + opt.file_break || opt.funcbody)) + skip_first_line =1 ; +start_threads (&opt); }else { -if (num_threads) -warning (_ ("no threads support, ignoring --threads" )); - num_threads =0 ; - } - -if (!num_threads)/* * The compiled patterns on the main path are only * used when not using threading. Otherwise -* start_threads() below calls compile_grep_patterns() +* start_threads() above calls compile_grep_patterns() * for each thread. */ compile_grep_patterns (&opt); - -if (HAVE_THREADS && num_threads) { -if (!(opt.name_only || opt.unmatch_name_only || opt.count) - && (opt.pre_context || opt.post_context || - opt.file_break || opt.funcbody)) - skip_first_line =1 ; -start_threads (&opt); }if (show_in_pager && (cached || list.nr))
@@ -1106,7 +1102,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) hit =grep_objects (&opt, &pathspec, &list); } -if (num_threads) +if (num_threads >1 ) hit |=wait_all ();if (hit && show_in_pager)run_pager (&opt, prefix);
builtin/index-pack.c +4 -7
@@ -1501,9 +1501,8 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)if (nr_threads <0 )die (_ ("invalid number of threads specified (%d)" ), nr_threads); -if (!HAVE_THREADS ) { -if (nr_threads !=1 ) -warning (_ ("no threads support, ignoring %s" ), k); +if (!HAVE_THREADS && nr_threads !=1 ) { +warning (_ ("no threads support, ignoring %s" ), k); nr_threads =1 ; }return 0 ;
@@ -1693,10 +1692,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) nr_threads =strtoul (arg+10 , &end,0 );if (!arg[10 ] || *end || nr_threads <0 )usage (index_pack_usage); -if (!HAVE_THREADS ) { -if (nr_threads !=1 ) -warning (_ ("no threads support, " -"ignoring %s" ), arg); +if (!HAVE_THREADS && nr_threads !=1 ) { +warning (_ ("no threads support, ignoring %s" ), arg); nr_threads =1 ; } }else if (starts_with (arg,"--pack_header=" )) {
read-cache.c +13 -15
@@ -2170,20 +2170,19 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) src_offset =sizeof (*hdr); -if (HAVE_THREADS ) { - nr_threads =git_config_get_index_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; - } - }else { - nr_threads =1 ; +/* 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; } +if (!HAVE_THREADS ) + nr_threads =1 ; +if (nr_threads >1 ) { extension_offset =read_eoie_extension (mmap, mmap_size);if (extension_offset) {
@@ -2216,12 +2215,11 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) istate->timestamp.nsec =ST_MTIME_NSEC (st);/* if we created a thread, join it otherwise load the extensions on the primary thread */ -if (HAVE_THREADS && extension_offset) { +if (extension_offset) {int ret =pthread_join (p.pthread,NULL );if (ret)die (_ ("unable to join load_index_extensions thread: %s" ),strerror (ret)); - } -if (!extension_offset) { + }else { p.src_offset = src_offset;load_index_extensions (&p); }
@@ -2860,7 +2858,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. */ -if (HAVE_THREADS && ieot) { +if (ieot) {struct strbuf sb =STRBUF_INIT ;write_ieot_extension (&sb, ieot);
run-command.c +9 -0
@@ -1246,6 +1246,15 @@ int finish_async(struct async *async)#endif } +int async_with_fork (void ) +{ +#ifdef NO_PTHREADS +return 1 ; +#else +return 0 ; +#endif +} +const char *find_hook (const char *name) {static struct strbuf path =STRBUF_INIT ;
run-command.h +2 -3
@@ -1,9 +1,7 @@#ifndef RUN_COMMAND_H #define RUN_COMMAND_H -#ifndef NO_PTHREADS -#include <pthread.h> -#endif +#include "thread-utils.h" #include "argv-array.h"
@@ -143,6 +141,7 @@ struct async {int start_async (struct async *async);int finish_async (struct async *async);int in_async (void ); +int async_with_fork (void );void check_pipe (int err);/**
send-pack.c +2 -3
@@ -203,9 +203,8 @@ static int receive_status(int in, struct ref *refs)static int sideband_demux (int in,int out,void *data) {int *fd = data, ret; -#ifdef NO_PTHREADS -close (fd[1 ]); -#endif +if (async_with_fork ()) +close (fd[1 ]); ret =recv_sideband ("send-pack" , fd[0 ], out);close (out);return ret;
Nguyễn Thái Ngọc Duy (14):
thread-utils: macros to unconditionally compile pthreads API run-command.h: include thread-utils.h instead of pthread.h send-pack.c: move async's #ifdef NO_PTHREADS back to run-command.c
index-pack: remove #ifdef NO_PTHREADS
name-hash.c: remove #ifdef NO_PTHREADS
attr.c: remove #ifdef NO_PTHREADS
grep: remove #ifdef NO_PTHREADS
grep: clean up num_threads handling
preload-index.c: remove #ifdef NO_PTHREADS
pack-objects: remove #ifdef NO_PTHREADS
read-cache.c: remove #ifdef NO_PTHREADS
read-cache.c: reduce branching based on HAVE_THREADS read-cache.c: initialize copy_len to shut up gcc 8 Clean up pthread_create() error handling
Makefile | 2 +- attr.c | 14 -------- builtin/grep.c | 79 ++++++++++++++++-------------------------- builtin/index-pack.c | 63 ++++++++------------------------- builtin/pack-objects.c | 26 ++------------ grep.c | 6 ---- grep.h | 6 ---- name-hash.c | 38 ++++++++------------ pack-objects.h | 6 ---- preload-index.c | 23 +++++------- read-cache.c | 37 ++++++-------------- run-command.c | 11 +++++- run-command.h | 5 ++- send-pack.c | 5 ++- thread-utils.c | 48 +++++++++++++++++++++++++ thread-utils.h | 48 +++++++++++++++++++++++-- 16 files changed, 186 insertions(+), 231 deletions(-)
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 1/14 thread-utils: macros to unconditionally compile pthreads API
When built with NO_PTHREADS, the macros are used make the code build even though pthreads header and library may be missing. The code can still have different code paths for no threads support with HAVE_THREADS variable.
There are of course impacts on no-pthreads builds:
- data structure may get slightly bigger because all the mutexes and pthread_t are present (as an int)
- code execution is not impacted much. Locking (in hot path) is no-op. Other wrapper function calls really should not matter much.
- the binary size grows bigger because of threaded code. But at least on Linux this does not matter, if some code is not executed, it's not mapped in memory.
This is a preparation step to remove "#ifdef NO_PTHREADS" in the code mostly because of maintainability. As Jeff put it
Quoted 4 lines
it's probably OK to stop thinking of it as "non-threaded platforms are the default and must pay zero cost" and more as "threaded platforms are the default, and non-threaded ones are OK to pay a small cost as long as they still work".
--- Makefile | 2 +- thread-utils.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ thread-utils.h | 48 +++++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 94 insertions(+), 4 deletions(-)
Makefile +1 -1
@@ -991,6 +991,7 @@ LIB_OBJS += sub-process.o LIB_OBJS += symlinks.o LIB_OBJS += tag.o LIB_OBJS += tempfile.o +LIB_OBJS += thread-utils.o LIB_OBJS += tmp-objdir.o LIB_OBJS += trace.o LIB_OBJS += trailer.o
@@ -1674,7 +1675,6 @@ ifdef NO_PTHREADS else BASIC_CFLAGS += $(PTHREAD_CFLAGS) EXTLIBS += $(PTHREAD_LIBS) - LIB_OBJS += thread-utils.o endif ifdef HAVE_PATHS_H
thread-utils.c +48 -0
68 lines not on this page · raw message
thread-utils.h +45 -3
57 lines not on this page · raw message
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 2/14 run-command.h: include thread-utils.h instead of pthread.h
In reply to Nguyễn Thái Ngọc Duyrun-command.c may use threads for its async support. But instead of including pthread.h directly, let's include thread-utils.h.
run-command.c probably never needs the dummy bits in thread-utils.h when NO_PTHREADS is defined. But this makes sure we have consistent HAVE_THREADS behavior everywhere. From now on outside compat/, thread-utils.h is the only place that includes pthread.h
--- run-command.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
run-command.h +1 -3
@@ -1,9 +1,7 @@#ifndef RUN_COMMAND_H #define RUN_COMMAND_H -#ifndef NO_PTHREADS -#include <pthread.h> -#endif +#include "thread-utils.h" #include "argv-array.h"
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 3/14 send-pack.c: move async's #ifdef NO_PTHREADS back to run-command.c
In reply to Nguyễn Thái Ngọc DuyOn systems that do not support multithread, start_async() is implemented with fork(). This implementation details unfortunately leak out at least in send-pack.c [1].
To keep the code base clean of NO_PTHREADS, move the this #ifdef back to run-command.c. The new wrapper function async_with_fork() at least helps suggest that this special "close()" is related to async in fork mode.
[1] 09c9957cf7 (send-pack: avoid deadlock when pack-object dies early
- 2011-04-25)
--- run-command.c | 9 +++++++++ run-command.h | 1 + send-pack.c | 5 ++--- 3 files changed, 12 insertions(+), 3 deletions(-)
run-command.c +9 -0
@@ -1246,6 +1246,15 @@ int finish_async(struct async *async)#endif } +int async_with_fork (void ) +{ +#ifdef NO_PTHREADS +return 1 ; +#else +return 0 ; +#endif +} +const char *find_hook (const char *name) {static struct strbuf path =STRBUF_INIT ;
run-command.h +1 -0
@@ -141,6 +141,7 @@ struct async {int start_async (struct async *async);int finish_async (struct async *async);int in_async (void ); +int async_with_fork (void );void check_pipe (int err);/**
send-pack.c +2 -3
@@ -203,9 +203,8 @@ static int receive_status(int in, struct ref *refs)static int sideband_demux (int in,int out,void *data) {int *fd = data, ret; -#ifdef NO_PTHREADS -close (fd[1 ]); -#endif +if (async_with_fork ()) +close (fd[1 ]); ret =recv_sideband ("send-pack" , fd[0 ], out);close (out);return ret;
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 4/14 index-pack: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- builtin/index-pack.c | 63 ++++++++++---------------------------------- 1 file changed, 14 insertions(+), 49 deletions(-)
builtin/index-pack.c +14 -49
@@ -42,9 +42,7 @@ struct base_data { };struct thread_local { -#ifndef NO_PTHREADS pthread_t thread; -#endif struct base_data *base_cache;size_t base_cache_used;int pack_fd;
@@ -98,8 +96,6 @@ static uint32_t input_crc32;static int input_fd, output_fd;static const char *curr_pack; -#ifndef NO_PTHREADS -static struct thread_local *thread_data;static int nr_dispatched;static int threads_active;
@@ -179,26 +175,6 @@ static void cleanup_thread(void)free (thread_data); } -#else - -#define read_lock () -#define read_unlock () - -#define counter_lock () -#define counter_unlock () - -#define work_lock () -#define work_unlock () - -#define deepest_delta_lock () -#define deepest_delta_unlock () - -#define type_cas_lock () -#define type_cas_unlock () - -#endif - -static int mark_link (struct object *obj,int type,void *data,struct fsck_options *options) {if (!obj)
@@ -364,22 +340,20 @@ static NORETURN void bad_object(off_t offset, const char *format, ...)static inline struct thread_local *get_thread_data (void ) { -#ifndef NO_PTHREADS -if (threads_active) -return pthread_getspecific (key); -assert (!threads_active && -"This should only be reached when all threads are gone" ); -#endif +if (HAVE_THREADS ) { +if (threads_active) +return pthread_getspecific (key); +assert (!threads_active && +"This should only be reached when all threads are gone" ); + }return ¬hread_data; } -#ifndef NO_PTHREADS static void set_thread_data (struct thread_local *data) {if (threads_active)pthread_setspecific (key, data); } -#endif static struct base_data *alloc_base_data (void ) {
@@ -1092,7 +1066,6 @@ static void resolve_base(struct object_entry *obj)find_unresolved_deltas (base_obj); } -#ifndef NO_PTHREADS static void *threaded_second_pass (void *data) {set_thread_data (data);
@@ -1116,7 +1089,6 @@ static void *threaded_second_pass(void *data) }return NULL ; } -#endif /* * First pass:
@@ -1213,7 +1185,6 @@ static void resolve_deltas(void) progress =start_progress (_ ("Resolving deltas" ), nr_ref_deltas + nr_ofs_deltas); -#ifndef NO_PTHREADS nr_dispatched =0 ;if (nr_threads >1 ||getenv ("GIT_FORCE_THREADS" )) {init_thread ();
@@ -1229,7 +1200,6 @@ static void resolve_deltas(void)cleanup_thread ();return ; } -#endif for (i =0 ; i < nr_objects; i++) {struct object_entry *obj = &objects[i];
@@ -1531,11 +1501,10 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)if (nr_threads <0 )die (_ ("invalid number of threads specified (%d)" ), nr_threads); -#ifdef NO_PTHREADS -if (nr_threads !=1 ) +if (!HAVE_THREADS && nr_threads !=1 ) {warning (_ ("no threads support, ignoring %s" ), k); - nr_threads =1 ; -#endif + nr_threads =1 ; + }return 0 ; }return git_default_config (k, v, cb);
@@ -1723,12 +1692,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) nr_threads =strtoul (arg+10 , &end,0 );if (!arg[10 ] || *end || nr_threads <0 )usage (index_pack_usage); -#ifdef NO_PTHREADS -if (nr_threads !=1 ) -warning (_ ("no threads support, " -"ignoring %s" ), arg); - nr_threads =1 ; -#endif +if (!HAVE_THREADS && nr_threads !=1 ) { +warning (_ ("no threads support, ignoring %s" ), arg); + nr_threads =1 ; + } }else if (starts_with (arg,"--pack_header=" )) {struct pack_header *hdr;char *c;
@@ -1791,14 +1758,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)if (strict) opts.flags |=WRITE_IDX_STRICT ; -#ifndef NO_PTHREADS -if (!nr_threads) { +if (HAVE_THREADS && !nr_threads) { nr_threads =online_cpus ();/* An experiment showed that more threads does not mean faster */ if (nr_threads >3 ) nr_threads =3 ; } -#endif curr_pack =open_pack_file (pack_name);parse_pack_header ();
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 5/14 name-hash.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- name-hash.c | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-)
name-hash.c +4 -18
@@ -7,6 +7,7 @@*/ #define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" +#include "thread-utils.h" struct dir_entry {struct hashmap_entry ent;
@@ -131,22 +132,6 @@ static int cache_entry_cmp(const void *unused_cmp_data,static int lazy_try_threaded =1 ;static int lazy_nr_dir_threads; -#ifdef NO_PTHREADS - -static inline int lookup_lazy_params (struct index_state *istate) -{ -return 0 ; -} - -static inline void threaded_lazy_init_name_hash ( -struct index_state *istate) -{ -} - -#else - -#include "thread-utils.h" -/* * Set a minimum number of cache_entries that we will handle per * thread and use that to decide how many threads to run (upto
@@ -516,6 +501,9 @@ static void threaded_lazy_init_name_hash(struct lazy_dir_thread_data *td_dir;struct lazy_name_thread_data *td_name; +if (!HAVE_THREADS ) +return ; + k_start =0 ; nr_each =DIV_ROUND_UP (istate->cache_nr, lazy_nr_dir_threads);
@@ -574,8 +562,6 @@ static void threaded_lazy_init_name_hash(free (lazy_entries); } -#endif -static void lazy_init_name_hash (struct index_state *istate) {
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 6/14 attr.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- attr.c | 14 -------------- 1 file changed, 14 deletions(-)
attr.c +0 -14
@@ -41,23 +41,17 @@ const char *git_attr_name(const struct git_attr *attr)struct attr_hashmap {struct hashmap map; -#ifndef NO_PTHREADS pthread_mutex_t mutex; -#endif };static inline void hashmap_lock (struct attr_hashmap *map) { -#ifndef NO_PTHREADS pthread_mutex_lock (&map->mutex); -#endif }static inline void hashmap_unlock (struct attr_hashmap *map) { -#ifndef NO_PTHREADS pthread_mutex_unlock (&map->mutex); -#endif }/*
@@ -498,23 +492,17 @@ static struct check_vector {size_t nr;size_t alloc;struct attr_check **checks; -#ifndef NO_PTHREADS pthread_mutex_t mutex; -#endif } check_vector;static inline void vector_lock (void ) { -#ifndef NO_PTHREADS pthread_mutex_lock (&check_vector.mutex); -#endif }static inline void vector_unlock (void ) { -#ifndef NO_PTHREADS pthread_mutex_unlock (&check_vector.mutex); -#endif }static void check_vector_add (struct attr_check *c)
@@ -1181,8 +1169,6 @@ void git_all_attrs(const struct index_state *istate,void attr_start (void ) { -#ifndef NO_PTHREADS pthread_mutex_init (&g_attr_hashmap.mutex,NULL );pthread_mutex_init (&check_vector.mutex,NULL ); -#endif }
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 7/14 grep: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc DuyThis is a faithful conversion without attempting to improve anything. That comes later.
--- builtin/grep.c | 59 +++++++++++++++++++------------------------------- grep.c | 6 ----- grep.h | 6 ----- 3 files changed, 22 insertions(+), 49 deletions(-)
builtin/grep.c +22 -37
@@ -34,7 +34,6 @@ static int recurse_submodules;#define GREP_NUM_THREADS_DEFAULT 8static int num_threads; -#ifndef NO_PTHREADS static pthread_t *threads;/* We use one producer thread and THREADS consumer
@@ -234,6 +233,9 @@ static int wait_all(void)int hit =0 ;int i; +if (!HAVE_THREADS ) +return 0 ; +grep_lock (); all_work_added =1 ;
@@ -265,13 +267,6 @@ static int wait_all(void)return hit; } -#else /* !NO_PTHREADS */ - -static int wait_all (void ) -{ -return 0 ; -} -#endif static int grep_cmd_config (const char *var,const char *value,void *cb) {
@@ -284,8 +279,7 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)if (num_threads <0 )die (_ ("invalid number of threads specified (%d) for %s" ), num_threads, var); -#ifdef NO_PTHREADS - else if (num_threads && num_threads != 1) { +else if (!HAVE_THREADS && num_threads && num_threads !=1 ) {/* * TRANSLATORS: %s is the configuration * variable for tweaking threads, currently
@@ -294,7 +288,6 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)warning (_ ("no threads support, ignoring %s" ), var); num_threads =0 ; } -#endif }if (!strcmp (var,"submodule.recurse" ))
@@ -330,17 +323,14 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,grep_source_init (&gs,GREP_SOURCE_OID , pathbuf.buf, path, oid);strbuf_release (&pathbuf); -#ifndef NO_PTHREADS -if (num_threads) { +if (HAVE_THREADS && num_threads) {/* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear() */ add_work (opt, &gs);return 0 ; - } else -#endif - { + }else {int hit; hit =grep_source (opt, &gs);
@@ -363,17 +353,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)grep_source_init (&gs,GREP_SOURCE_FILE , buf.buf, filename, filename);strbuf_release (&buf); -#ifndef NO_PTHREADS -if (num_threads) { +if (HAVE_THREADS && num_threads) {/* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear() */ add_work (opt, &gs);return 0 ; - } else -#endif - { + }else {int hit; hit =grep_source (opt, &gs);
@@ -1038,20 +1025,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix) pathspec.recursive =1 ; pathspec.recurse_submodules = !!recurse_submodules; -#ifndef NO_PTHREADS -if (list.nr || cached || show_in_pager) - num_threads =0 ; -else if (num_threads ==0 ) - num_threads =GREP_NUM_THREADS_DEFAULT ; -else if (num_threads <0 ) -die (_ ("invalid number of threads specified (%d)" ), num_threads); -if (num_threads ==1 ) +if (HAVE_THREADS ) { +if (list.nr || cached || show_in_pager) + num_threads =0 ; +else if (num_threads ==0 ) + num_threads =GREP_NUM_THREADS_DEFAULT ; +else if (num_threads <0 ) +die (_ ("invalid number of threads specified (%d)" ), num_threads); +if (num_threads ==1 ) + num_threads =0 ; + }else { +if (num_threads) +warning (_ ("no threads support, ignoring --threads" )); num_threads =0 ; -#else -if (num_threads) -warning (_ ("no threads support, ignoring --threads" )); - num_threads =0 ; -#endif + }if (!num_threads)/*
@@ -1062,15 +1049,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)*/ compile_grep_patterns (&opt); -#ifndef NO_PTHREADS -if (num_threads) { +if (HAVE_THREADS && num_threads) {if (!(opt.name_only || opt.unmatch_name_only || opt.count) && (opt.pre_context || opt.post_context || opt.file_break || opt.funcbody)) skip_first_line =1 ;start_threads (&opt); } -#endif if (show_in_pager && (cached || list.nr))die (_ ("--open-files-in-pager only works on the worktree" ));
grep.c +0 -6
@@ -1513,7 +1513,6 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol, } } -#ifndef NO_PTHREADS int grep_use_locks;/*
@@ -1539,11 +1538,6 @@ static inline void grep_attr_unlock(void)*/ pthread_mutex_t grep_read_mutex; -#else -#define grep_attr_lock () -#define grep_attr_unlock () -#endif -static int match_funcname (struct grep_opt *opt,struct grep_source *gs,char *bol,char *eol) {xdemitconf_t *xecfg = opt->priv;
grep.h +0 -6
@@ -229,7 +229,6 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs);extern struct grep_opt *grep_opt_dup (const struct grep_opt *opt);extern int grep_threads_ok (const struct grep_opt *opt); -#ifndef NO_PTHREADS /* * Mutex used around access to the attributes machinery if * opt->use_threads. Must be initialized/destroyed by callers!
@@ -250,9 +249,4 @@ static inline void grep_read_unlock(void) pthread_mutex_unlock(&grep_read_mutex); } -#else -#define grep_read_lock () -#define grep_read_unlock () -#endif -#endif
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 8/14 grep: clean up num_threads handling
In reply to Nguyễn Thái Ngọc DuyWhen NO_PTHREADS is still used in this file, we have two separate code paths for thread and no thread support. The latter will always have num_threads remain zero while the former uses num_threads zero as "default number of threads".
With recent changes blur the line between thread and no-thread support, this num_threads handling becomes a bit strange so let's redefine it like this:
- num_threads == 0 means default number of threads and should become positive after all configuration and option parsing is done if multithread is supported.
- num_threads <= 1 runs no threads. It does not matter if the platform supports threading or not.
- num_threads > 1 will run multiple threads and is invalid if HAVE_THREADS is false. pthread API is only used in this case.
PS. a new warning is also added when num_threads is forced back to one because a thread-incompatible option is specified.
--- builtin/grep.c | 58 +++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 31 deletions(-)
builtin/grep.c +27 -31
@@ -69,13 +69,11 @@ static pthread_mutex_t grep_mutex;static inline void grep_lock (void ) { -assert (num_threads);pthread_mutex_lock (&grep_mutex); }static inline void grep_unlock (void ) { -assert (num_threads);pthread_mutex_unlock (&grep_mutex); }
@@ -234,7 +232,7 @@ static int wait_all(void)int i;if (!HAVE_THREADS ) -return 0 ; +BUG ("Never call this function unless you have started threads" );grep_lock (); all_work_added =1 ;
@@ -279,14 +277,14 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)if (num_threads <0 )die (_ ("invalid number of threads specified (%d) for %s" ), num_threads, var); -else if (!HAVE_THREADS && num_threads && num_threads !=1 ) { +else if (!HAVE_THREADS && num_threads >1 ) {/* * TRANSLATORS: %s is the configuration * variable for tweaking threads, currently * grep.threads */ warning (_ ("no threads support, ignoring %s" ), var); - num_threads =0 ; + num_threads =1 ; } }
@@ -323,7 +321,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,grep_source_init (&gs,GREP_SOURCE_OID , pathbuf.buf, path, oid);strbuf_release (&pathbuf); -if (HAVE_THREADS && num_threads) { +if (num_threads >1 ) {/* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear()
@@ -353,7 +351,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)grep_source_init (&gs,GREP_SOURCE_FILE , buf.buf, filename, filename);strbuf_release (&buf); -if (HAVE_THREADS && num_threads) { +if (num_threads >1 ) {/* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear()
@@ -1025,36 +1023,34 @@ int cmd_grep(int argc, const char **argv, const char *prefix) pathspec.recursive =1 ; pathspec.recurse_submodules = !!recurse_submodules; -if (HAVE_THREADS ) { -if (list.nr || cached || show_in_pager) - num_threads =0 ; -else if (num_threads ==0 ) - num_threads =GREP_NUM_THREADS_DEFAULT ; -else if (num_threads <0 ) -die (_ ("invalid number of threads specified (%d)" ), num_threads); -if (num_threads ==1 ) - num_threads =0 ; +if (list.nr || cached || show_in_pager) { +if (num_threads >1 ) +warning (_ ("invalid option combination, ignoring --threads" )); + num_threads =1 ; + }else if (!HAVE_THREADS && num_threads >1 ) { +warning (_ ("no threads support, ignoring --threads" )); + num_threads =1 ; + }else if (num_threads <0 ) +die (_ ("invalid number of threads specified (%d)" ), num_threads); +else if (num_threads ==0 ) + num_threads =HAVE_THREADS ?GREP_NUM_THREADS_DEFAULT :1 ; + +if (num_threads >1 ) { +if (!HAVE_THREADS ) +BUG ("Somebody got num_threads calculation wrong!" ); +if (!(opt.name_only || opt.unmatch_name_only || opt.count) + && (opt.pre_context || opt.post_context || + opt.file_break || opt.funcbody)) + skip_first_line =1 ; +start_threads (&opt); }else { -if (num_threads) -warning (_ ("no threads support, ignoring --threads" )); - num_threads =0 ; - } - -if (!num_threads)/* * The compiled patterns on the main path are only * used when not using threading. Otherwise -* start_threads() below calls compile_grep_patterns() +* start_threads() above calls compile_grep_patterns() * for each thread. */ compile_grep_patterns (&opt); - -if (HAVE_THREADS && num_threads) { -if (!(opt.name_only || opt.unmatch_name_only || opt.count) - && (opt.pre_context || opt.post_context || - opt.file_break || opt.funcbody)) - skip_first_line =1 ; -start_threads (&opt); }if (show_in_pager && (cached || list.nr))
@@ -1106,7 +1102,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) hit =grep_objects (&opt, &pathspec, &list); } -if (num_threads) +if (num_threads >1 ) hit |=wait_all ();if (hit && show_in_pager)run_pager (&opt, prefix);
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 9/14 preload-index.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- preload-index.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)
preload-index.c +2 -13
@@ -7,17 +7,7 @@#include "fsmonitor.h" #include "config.h" #include "progress.h" - -#ifdef NO_PTHREADS -static void preload_index (struct index_state *index, -const struct pathspec *pathspec, -unsigned int refresh_flags) -{ - ;/* nothing */ -} -#else - -#include <pthread.h> +#include "thread-utils.h" /* * Mostly randomly chosen maximum thread counts: we
@@ -108,7 +98,7 @@ static void preload_index(struct index_state *index,struct thread_data data[MAX_PARALLEL ];struct progress_data pd; -if (!core_preload_index) +if (!HAVE_THREADS || !core_preload_index)return ; threads = index->cache_nr /THREAD_COST ;
@@ -151,7 +141,6 @@ static void preload_index(struct index_state *index,trace_performance_leave ("preload index" ); } -#endif int read_index_preload(struct index_state *index, const struct pathspec *pathspec,
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 10/14 pack-objects: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc Duy--- builtin/pack-objects.c | 26 ++------------------------ pack-objects.h | 6 ------ 2 files changed, 2 insertions(+), 30 deletions(-)
builtin/pack-objects.c +2 -24
@@ -1953,8 +1953,6 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,return 0 ; } -#ifndef NO_PTHREADS -/* Protect access to object database */ static pthread_mutex_t read_mutex;#define read_lock () pthread_mutex_lock(&read_mutex)
@@ -1979,16 +1977,6 @@ static pthread_mutex_t progress_mutex;* ahead in the list because they can be stolen and would need * progress_mutex for protection. */ -#else - -#define read_lock () (void)0 -#define read_unlock () (void)0 -#define cache_lock () (void)0 -#define cache_unlock () (void)0 -#define progress_lock () (void)0 -#define progress_unlock () (void)0 - -#endif /* * Return the size of the object without doing any delta
@@ -2347,8 +2335,6 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,free (array); } -#ifndef NO_PTHREADS -static void try_to_free_from_threads (size_t size) {read_lock ();
@@ -2578,10 +2564,6 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,free (p); } -#else -#define ll_find_deltas (l, s, w, d, p) find_deltas(l, &s, w, d, p) -#endif -static void add_tag_chain (const struct object_id *oid) {struct tag *tag;
@@ -2734,12 +2716,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)if (delta_search_threads <0 )die (_ ("invalid number of threads specified (%d)" ), delta_search_threads); -#ifdef NO_PTHREADS -if (delta_search_threads !=1 ) { +if (!HAVE_THREADS && delta_search_threads !=1 ) {warning (_ ("no threads support, ignoring %s" ), k); delta_search_threads =0 ; } -#endif return 0 ; }if (!strcmp (k,"pack.indexversion" )) {
@@ -3402,10 +3382,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)if (!delta_search_threads)/* --threads=0 means autodetect */ delta_search_threads =online_cpus (); -#ifdef NO_PTHREADS -if (delta_search_threads !=1 ) +if (!HAVE_THREADS && delta_search_threads !=1 )warning (_ ("no threads support, ignoring --threads" )); -#endif if (!pack_to_stdout && !pack_size_limit) pack_size_limit = pack_size_limit_cfg;if (pack_to_stdout && pack_size_limit)
pack-objects.h +0 -6
@@ -145,9 +145,7 @@ struct packing_data {struct packed_git **in_pack_by_idx;struct packed_git **in_pack; -#ifndef NO_PTHREADS pthread_mutex_t lock; -#endif /* * This list contains entries for bases which we know the other side
@@ -169,15 +167,11 @@ void prepare_packing_data(struct packing_data *pdata);static inline void packing_data_lock (struct packing_data *pdata) { -#ifndef NO_PTHREADS pthread_mutex_lock (&pdata->lock); -#endif }static inline void packing_data_unlock (struct packing_data *pdata) { -#ifndef NO_PTHREADS pthread_mutex_unlock (&pdata->lock); -#endif } struct object_entry *packlist_alloc(struct packing_data *pdata,
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 11/14 read-cache.c: remove #ifdef NO_PTHREADS
In reply to Nguyễn Thái Ngọc DuyThis is a faithful conversion with no attempt to clean up whatsoever. Code indentation is left broken. There will be another commit to clean it up and un-indent if we just indent now. It's just more code noise.
--- read-cache.c | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-)
read-cache.c +10 -24
@@ -1920,19 +1920,15 @@ struct index_entry_offset_tablestruct 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, conreturn 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,7 +2172,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) src_offset =sizeof (*hdr); -#ifndef NO_PTHREADS +if (HAVE_THREADS ) { nr_threads =git_config_get_index_threads ();/* TODO: does creating more threads than cores help? */
@@ -2219,21 +2210,19 @@ 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 + }else { + src_offset +=load_all_cache_entries (istate, mmap, mmap_size, src_offset); + } 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,9 @@ 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 (); +if (nr_threads !=1 ) {int ieot_blocks, cpus;
@@ -2787,7 +2777,7 @@ 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 +2861,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 +2872,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 +3457,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 +3529,3 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_tastrbuf_add (sb, &buffer,sizeof (uint32_t )); } } -#endif
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 12/14 read-cache.c: reduce branching based on HAVE_THREADS
In reply to Nguyễn Thái Ngọc Duy--- read-cache.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
read-cache.c +9 -10
@@ -2172,7 +2172,6 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) src_offset =sizeof (*hdr); -if (HAVE_THREADS ) { nr_threads =git_config_get_index_threads ();/* TODO: does creating more threads than cores help? */
@@ -2183,6 +2182,9 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) nr_threads = cpus; } +if (!HAVE_THREADS ) + nr_threads =1 ; +if (nr_threads >1 ) { extension_offset =read_eoie_extension (mmap, mmap_size);if (extension_offset) {
@@ -2210,20 +2212,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); - } 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 */ -if (HAVE_THREADS && extension_offset) { +if (extension_offset) {int ret =pthread_join (p.pthread,NULL );if (ret)die (_ ("unable to join load_index_extensions thread: %s" ),strerror (ret)); - } -if (!extension_offset) { + }else { p.src_offset = src_offset;load_index_extensions (&p); }
@@ -2745,8 +2743,10 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,if (ce_write (&c, newfd, &hdr,sizeof (hdr)) <0 )return -1 ; -if (HAVE_THREADS ) { +if (HAVE_THREADS ) nr_threads =git_config_get_index_threads (); +else + nr_threads =1 ;if (nr_threads !=1 ) {int ieot_blocks, cpus;
@@ -2777,7 +2777,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, ieot_entries =DIV_ROUND_UP (entries, ieot_blocks); } } - } offset =lseek (newfd,0 ,SEEK_CUR );if (offset <0 ) {
@@ -2861,7 +2860,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. */ -if (HAVE_THREADS && ieot) { +if (ieot) {struct strbuf sb =STRBUF_INIT ;write_ieot_extension (&sb, ieot);
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 13/14 read-cache.c: initialize copy_len to shut up gcc 8
In reply to Nguyễn Thái Ngọc DuyIt was reported that when building with NO_PTHREADS=1, -Wmaybe-uninitialized is triggered. Just initialize the variable from the beginning to shut the compiler up (because this warning is enabled in config.dev)
--- read-cache.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
read-cache.c +1 -3
@@ -1746,7 +1746,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,size_t len;const char *name;unsigned int flags; -size_t copy_len; +size_t copy_len =0 ;/* * Adjacent cache entries tend to share the leading paths, so it makes * sense to only store the differences in later entries. In the v4
@@ -1786,8 +1786,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,die (_ ("malformed name field in the index, near path '%s'" ), previous_ce->name); copy_len = previous_len - strip_len; - }else { - copy_len =0 ; } name = (const char *)cp; }
Signature · 1 line
2.19.1.1005.gac84295441
PATCH v3 14/14 Clean up pthread_create() error handling
In reply to Nguyễn Thái Ngọc DuyNormally pthread_create() rarely fails. But with new pthreads wrapper, pthread_create() will return ENOSYS on a system without thread support.
Threaded code _is_ protected by HAVE_THREADS and pthread_create() should never run in the first place. But the situation could change in the future and bugs may sneak in. Make sure that all pthread_create() reports the error cause.
While at there, mark these strings for translation if they aren't.
--- name-hash.c | 16 ++++++++++------ preload-index.c | 8 ++++++-- run-command.c | 2 +- 3 files changed, 17 insertions(+), 9 deletions(-)
name-hash.c +10 -6
@@ -494,6 +494,7 @@ static inline void lazy_update_dir_ref_counts(static void threaded_lazy_init_name_hash (struct index_state *istate) { +int err;int nr_each;int k_start;int t;
@@ -526,8 +527,9 @@ static void threaded_lazy_init_name_hash(if (k_start > istate->cache_nr) k_start = istate->cache_nr; td_dir_t->k_end = k_start; -if (pthread_create (&td_dir_t->pthread,NULL , lazy_dir_thread_proc, td_dir_t)) -die ("unable to create lazy_dir_thread" ); + err =pthread_create (&td_dir_t->pthread,NULL , lazy_dir_thread_proc, td_dir_t); +if (err) +die (_ ("unable to create lazy_dir thread: %s" ),strerror (err)); }for (t =0 ; t < lazy_nr_dir_threads; t++) {struct lazy_dir_thread_data *td_dir_t = td_dir + t;
@@ -547,13 +549,15 @@ static void threaded_lazy_init_name_hash(*/ td_name->istate = istate; td_name->lazy_entries = lazy_entries; -if (pthread_create (&td_name->pthread,NULL , lazy_name_thread_proc, td_name)) -die ("unable to create lazy_name_thread" ); + err =pthread_create (&td_name->pthread,NULL , lazy_name_thread_proc, td_name); +if (err) +die (_ ("unable to create lazy_name thread: %s" ),strerror (err));lazy_update_dir_ref_counts (istate, lazy_entries); -if (pthread_join (td_name->pthread,NULL )) -die ("unable to join lazy_name_thread" ); + err =pthread_join (td_name->pthread,NULL ); +if (err) +die (_ ("unable to join lazy_name thread: %s" ),strerror (err));cleanup_dir_mutex ();
preload-index.c +6 -2
@@ -121,6 +121,8 @@ static void preload_index(struct index_state *index,for (i =0 ; i < threads; i++) {struct thread_data *p = data+i; +int err; + p->index = index;if (pathspec)copy_pathspec (&p->pathspec, pathspec);
@@ -129,8 +131,10 @@ static void preload_index(struct index_state *index,if (pd.progress) p->progress = &pd; offset += work; -if (pthread_create (&p->pthread,NULL , preload_thread, p)) -die ("unable to create threaded lstat" ); + err =pthread_create (&p->pthread,NULL , preload_thread, p); + +if (err) +die (_ ("unable to create threaded lstat: %s" ),strerror (err)); }for (i =0 ; i < threads; i++) {struct thread_data *p = data+i;
run-command.c +1 -1
@@ -1213,7 +1213,7 @@ int start_async(struct async *async) {int err =pthread_create (&async->tid,NULL , run_thread, async);if (err) { -error_errno ("cannot create thread" ); +error (_ ("cannot create async thread: %s" ),strerror (err)); goto error; } }
Signature · 1 line
2.19.1.1005.gac84295441
Quoted 5 lines · Nguyễn Thái Ngọc Duy
Changes since v2
- more cleanups in
grep.c,read-cache.candindex-pack.c
- thesend-pack.cchanges are back, but this time I just addasync_with_fork()to moveNO_PTHREADSback inrun-command.c
The patches all looked sensible; I'll wait for a few more days to see if anybody else spots something.
Thanks.