threads / patch / 49697

patch, 10 partsReduce #ifdef NO_PTHREADS

Subject: [PATCH 00/10] Reduce #ifdef NO_PTHREADS

## tl;dr

66 messages between Oct 27, 2018 and Nov 6, 2018. Diffs are folded; open one to read it.

replies: 65people: 4as markdown or json

Nguyễn Thái Ngọc Duy· Oct 27, 2018, 07:09 UTC · lore

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/20181018180522.17642-1-pclouds@gmail.com/
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(-)
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 07:09 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 01/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

> 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".
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Makefile       |  2 +-
 thread-utils.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 thread-utils.h | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 93 insertions(+), 4 deletions(-)
Show changes to 3 files +93 −4

Makefile, thread-utils.c, thread-utils.h

diff --git a/Makefile b/Makefile
index b08d5ea258..321540a736 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/thread-utils.c b/thread-utils.c
index a2135e0743..5329845691 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -20,6 +20,9 @@
 
 int online_cpus(void)
 {
+#ifdef NO_PTHREADS
+	return 1;
+#else
 #ifdef _SC_NPROCESSORS_ONLN
 	long ncpus;
 #endif
@@ -59,10 +62,12 @@ int online_cpus(void)
 #endif
 
 	return 1;
+#endif
 }
 
 int init_recursive_mutex(pthread_mutex_t *m)
 {
+#ifndef NO_PTHREADS
 	pthread_mutexattr_t a;
 	int ret;
 
@@ -74,4 +79,47 @@ int init_recursive_mutex(pthread_mutex_t *m)
 		pthread_mutexattr_destroy(&a);
 	}
 	return ret;
+#else
+	return 0;
+#endif
+}
+
+#ifdef NO_PTHREADS
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+int dummy_pthread_init(void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis or it may realize that functions like
+	 * pthread_mutex_init() is no-op, which means the (static)
+	 * variable is not used/initialized at all and trigger
+	 * -Wunused-variable
+	 */
+	return ENOSYS;
 }
+
+int dummy_pthread_join(pthread_t pthread, void **retval)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+#endif
diff --git a/thread-utils.h b/thread-utils.h
index d9a769d190..a12850e747 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -4,12 +4,53 @@
 #ifndef NO_PTHREADS
 #include <pthread.h>
 
-extern int online_cpus(void);
-extern int init_recursive_mutex(pthread_mutex_t*);
+#define HAVE_THREADS 1
 
 #else
 
-#define online_cpus() 1
+#define HAVE_THREADS 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)
+
+#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)
+
+#define pthread_key_create(key, attr) dummy_pthread_init(key)
+#define pthread_key_delete(key)
+
+#define pthread_create(thread, attr, fn, data) \
+	dummy_pthread_create(thread, attr, fn, data)
+#define pthread_join(thread, retval) \
+	dummy_pthread_join(thread, retval)
+
+#define pthread_setspecific(key, data)
+#define pthread_getspecific(key) NULL
+
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data);
+int dummy_pthread_join(pthread_t pthread, void **retval);
+
+int dummy_pthread_init(void *);
 
 #endif
+
+int online_cpus(void);
+int init_recursive_mutex(pthread_mutex_t*);
+
+
 #endif /* THREAD_COMPAT_H */
-- 
2.19.1.647.g708186aaf9
Jeff King· Oct 27, 2018, 07:31 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API

On Sat, Oct 27, 2018 at 09:09:54AM +0200, Nguyễn Thái Ngọc Duy wrote:
Show 13 quoted lines
> +/*
> + * 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.

Show 5 quoted lines
> +#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.
> +#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...

> +#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
> +#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
Duy Nguyen· Oct 27, 2018, 07:40 UTC · re: Jeff King · lore

Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API

On Sat, Oct 27, 2018 at 9:31 AM Jeff King <peff@peff.net> wrote:
Show 13 quoted lines
> > +#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

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.

-- 
Duy
Jeff King· Oct 27, 2018, 08:15 UTC · re: Duy Nguyen · lore

Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API

On Sat, Oct 27, 2018 at 09:40:13AM +0200, Duy Nguyen wrote:
Show 13 quoted 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)
> >
> >   void pthread_getspecific(key) pthread_specific_data
> 
> The 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.

Show 5 quoted lines
> 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.
Yeah, you might be right.
-Peff
Duy Nguyen· Oct 27, 2018, 14:43 UTC · re: Jeff King · lore

Re: [PATCH 01/10] thread-utils: macros to unconditionally compile pthreads API

On Sat, Oct 27, 2018 at 10:15 AM Jeff King <peff@peff.net> wrote:
Show 20 quoted lines
>
> On Sat, Oct 27, 2018 at 09:40:13AM +0200, Duy Nguyen wrote:
>
> > > 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
> >
> > The 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(.., &nothread_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

-- 
Duy
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 07:09 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 03/10] name-hash.c: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)
Show changes to name-hash.c +4 −18
diff --git a/name-hash.c b/name-hash.c
index 1fcda73cb3..b3c9ac791d 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -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)
 {
 
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 07:09 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 04/10] attr.c: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 attr.c | 14 --------------
 1 file changed, 14 deletions(-)
Show changes to attr.c +0 −14
diff --git a/attr.c b/attr.c
index 60d284796d..eaece6658d 100644
--- a/attr.c
+++ b/attr.c
@@ -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
 }
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 07:09 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 05/10] send-pack.c: remove #ifdef NO_PTHREADS

While at there correct "#include cache.h" position. It must be one of the first includes.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 send-pack.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to send-pack.c +4 −4
diff --git a/send-pack.c b/send-pack.c
index e920ca57df..fa7dc44b36 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -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;
-- 
2.19.1.647.g708186aaf9
Jeff King· Oct 27, 2018, 07:39 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 05/10] send-pack.c: remove #ifdef NO_PTHREADS

On Sat, Oct 27, 2018 at 09:09:58AM +0200, Nguyễn Thái Ngọc Duy wrote:
Show 6 quoted lines
> While at there correct "#include cache.h" position. It must be one of
> the first includes.
> [...]
> @@ -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.

Show 12 quoted lines
> @@ -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
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 07:09 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 06/10] grep: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 58 +++++++++++++++++---------------------------------
 grep.c         |  6 ------
 grep.h         |  6 ------
 3 files changed, 20 insertions(+), 50 deletions(-)
Show changes to 3 files +20 −50

builtin/grep.c, grep.c, grep.h

diff --git a/builtin/grep.c b/builtin/grep.c
index d8508ddf79..29221e1003 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -34,7 +34,6 @@ static int recurse_submodules;
 #define GREP_NUM_THREADS_DEFAULT 8
 static 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);
diff --git a/grep.c b/grep.c
index f6bd89e40b..4db1510d16 100644
--- a/grep.c
+++ b/grep.c
@@ -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;
diff --git a/grep.h b/grep.h
index 1a57d12b90..fb04893721 100644
--- a/grep.h
+++ b/grep.h
@@ -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
-- 
2.19.1.647.g708186aaf9
Jeff King· Oct 27, 2018, 07:44 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

On Sat, Oct 27, 2018 at 09:09:59AM +0200, Nguyễn Thái Ngọc Duy wrote:
Show 23 quoted lines
> diff --git a/builtin/grep.c b/builtin/grep.c
> index d8508ddf79..29221e1003 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -34,7 +34,6 @@ static int recurse_submodules;
>  #define GREP_NUM_THREADS_DEFAULT 8
>  static 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

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:

> -	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
Junio C Hamano· Oct 29, 2018, 02:16 UTC · re: Jeff King · lore

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> 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:
>
>> -	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.

Show 5 quoted 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
Jeff King· Oct 29, 2018, 14:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

On Mon, Oct 29, 2018 at 11:16:39AM +0900, Junio C Hamano wrote:
Show 29 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > 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:
> >
> >> -	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.

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
Duy Nguyen· Oct 29, 2018, 16:01 UTC · re: Jeff King · lore

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

On Mon, Oct 29, 2018 at 3:25 PM Jeff King <peff@peff.net> wrote:
> 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?)

-- 
Duy
Jeff King· Oct 29, 2018, 16:20 UTC · re: Duy Nguyen · lore

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

On Mon, Oct 29, 2018 at 05:01:41PM +0100, Duy Nguyen wrote:
Show 9 quoted lines
> On Mon, Oct 29, 2018 at 3:25 PM Jeff King <peff@peff.net> wrote:
> > 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
Junio C Hamano· Oct 30, 2018, 01:27 UTC · re: Jeff King · lore

Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> On Mon, Oct 29, 2018 at 05:01:41PM +0100, Duy Nguyen wrote:
>
>> On Mon, Oct 29, 2018 at 3:25 PM Jeff King <peff@peff.net> wrote:
>> > 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. :)
Likewise.
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 07:09 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 02/10] index-pack: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c | 68 ++++++++++++--------------------------------
 1 file changed, 18 insertions(+), 50 deletions(-)
Show changes to builtin/index-pack.c +18 −50
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2004e25da2..bbd66ca025 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -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 &nothread_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();
-- 
2.19.1.647.g708186aaf9
Jeff King· Oct 27, 2018, 07:34 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 02/10] index-pack: remove #ifdef NO_PTHREADS

On Sat, Oct 27, 2018 at 09:09:55AM +0200, Nguyễn Thái Ngọc Duy wrote:
Show 24 quoted lines
>  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 &nothread_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
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 07:10 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 07/10] preload-index.c: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 preload-index.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)
Show changes to preload-index.c +2 −13
diff --git a/preload-index.c b/preload-index.c
index 9e7152ab14..0e24886aca 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -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,
-- 
2.19.1.647.g708186aaf9
Ben Peart· Oct 29, 2018, 16:52 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 07/10] preload-index.c: remove #ifdef NO_PTHREADS

On 10/27/2018 3:10 AM, Nguyễn Thái Ngọc Duy wrote:
Show 34 quoted lines
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   preload-index.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/preload-index.c b/preload-index.c
> index 9e7152ab14..0e24886aca 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -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)

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.

Show 12 quoted lines
>   		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,
> 
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 07:10 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 08/10] pack-objects: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c | 26 ++------------------------
 pack-objects.h         |  6 ------
 2 files changed, 2 insertions(+), 30 deletions(-)
Show changes to 2 files +2 −30

builtin/pack-objects.c, pack-objects.h

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b059b86aee..12edd6da16 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -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)
diff --git a/pack-objects.h b/pack-objects.h
index 2ca39cfcfe..3a42727c7d 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -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,
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 07:10 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c | 49 ++++++++++++++++++-------------------------------
 1 file changed, 18 insertions(+), 31 deletions(-)
Show changes to read-cache.c +18 −31
diff --git a/read-cache.c b/read-cache.c
index d57958233e..ba870bc3fd 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1920,19 +1920,15 @@ struct index_entry_offset_table
 	struct index_entry_offset entries[FLEX_ARRAY];
 };
 
-#ifndef NO_PTHREADS
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset);
 static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot);
-#endif
 
 static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
 static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset);
 
 struct load_index_extensions
 {
-#ifndef NO_PTHREADS
 	pthread_t pthread;
-#endif
 	struct index_state *istate;
 	const char *mmap;
 	size_t mmap_size;
@@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct index_state *istate,
 	return consumed;
 }
 
-#ifndef NO_PTHREADS
-
 /*
  * Mostly randomly chosen maximum thread counts: we
  * cap the parallelism to online_cpus() threads, and we want
@@ -2122,7 +2116,6 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
 
 	return consumed;
 }
-#endif
 
 /* remember to discard_cache() before reading a different cache! */
 int do_read_index(struct index_state *istate, const char *path, int must_exist)
@@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	size_t mmap_size;
 	struct load_index_extensions p;
 	size_t extension_offset = 0;
-#ifndef NO_PTHREADS
 	int nr_threads, cpus;
 	struct index_entry_offset_table *ieot = NULL;
-#endif
 
 	if (istate->initialized)
 		return istate->cache_nr;
@@ -2181,15 +2172,18 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 
 	src_offset = sizeof(*hdr);
 
-#ifndef NO_PTHREADS
-	nr_threads = git_config_get_index_threads();
+	if (HAVE_THREADS) {
+		nr_threads = git_config_get_index_threads();
 
-	/* TODO: does creating more threads than cores help? */
-	if (!nr_threads) {
-		nr_threads = istate->cache_nr / THREAD_COST;
-		cpus = online_cpus();
-		if (nr_threads > cpus)
-			nr_threads = cpus;
+		/* TODO: does creating more threads than cores help? */
+		if (!nr_threads) {
+			nr_threads = istate->cache_nr / THREAD_COST;
+			cpus = online_cpus();
+			if (nr_threads > cpus)
+				nr_threads = cpus;
+		}
+	} else {
+		nr_threads = 1;
 	}
 
 	if (nr_threads > 1) {
@@ -2219,21 +2213,16 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	} else {
 		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
 	}
-#else
-	src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
-#endif
 
 	istate->timestamp.sec = st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
 
 	/* if we created a thread, join it otherwise load the extensions on the primary thread */
-#ifndef NO_PTHREADS
-	if (extension_offset) {
+	if (HAVE_THREADS && extension_offset) {
 		int ret = pthread_join(p.pthread, NULL);
 		if (ret)
 			die(_("unable to join load_index_extensions thread: %s"), strerror(ret));
 	}
-#endif
 	if (!extension_offset) {
 		p.src_offset = src_offset;
 		load_index_extensions(&p);
@@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
 		return -1;
 
-#ifndef NO_PTHREADS
-	nr_threads = git_config_get_index_threads();
+	if (HAVE_THREADS)
+		nr_threads = git_config_get_index_threads();
+	else
+		nr_threads = 1;
+
 	if (nr_threads != 1) {
 		int ieot_blocks, cpus;
 
@@ -2787,7 +2779,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 			ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
 		}
 	}
-#endif
 
 	offset = lseek(newfd, 0, SEEK_CUR);
 	if (offset < 0) {
@@ -2871,8 +2862,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	 * strip_extensions parameter as we need it when loading the shared
 	 * index.
 	 */
-#ifndef NO_PTHREADS
-	if (ieot) {
+	if (HAVE_THREADS && ieot) {
 		struct strbuf sb = STRBUF_INIT;
 
 		write_ieot_extension(&sb, ieot);
@@ -2883,7 +2873,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 		if (err)
 			return -1;
 	}
-#endif
 
 	if (!strip_extensions && istate->split_index) {
 		struct strbuf sb = STRBUF_INIT;
@@ -3469,7 +3458,6 @@ static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context,
 	strbuf_add(sb, hash, the_hash_algo->rawsz);
 }
 
-#ifndef NO_PTHREADS
 #define IEOT_VERSION	(1)
 
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset)
@@ -3542,4 +3530,3 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta
 	       strbuf_add(sb, &buffer, sizeof(uint32_t));
        }
 }
-#endif
-- 
2.19.1.647.g708186aaf9
Ben Peart· Oct 29, 2018, 17:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS

On 10/27/2018 3:10 AM, Nguyễn Thái Ngọc Duy wrote:
Show 64 quoted lines
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   read-cache.c | 49 ++++++++++++++++++-------------------------------
>   1 file changed, 18 insertions(+), 31 deletions(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index d57958233e..ba870bc3fd 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1920,19 +1920,15 @@ struct index_entry_offset_table
>   	struct index_entry_offset entries[FLEX_ARRAY];
>   };
>   
> -#ifndef NO_PTHREADS
>   static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset);
>   static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot);
> -#endif
>   
>   static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
>   static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset);
>   
>   struct load_index_extensions
>   {
> -#ifndef NO_PTHREADS
>   	pthread_t pthread;
> -#endif
>   	struct index_state *istate;
>   	const char *mmap;
>   	size_t mmap_size;
> @@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct index_state *istate,
>   	return consumed;
>   }
>   
> -#ifndef NO_PTHREADS
> -
>   /*
>    * Mostly randomly chosen maximum thread counts: we
>    * cap the parallelism to online_cpus() threads, and we want
> @@ -2122,7 +2116,6 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
>   
>   	return consumed;
>   }
> -#endif
>   
>   /* remember to discard_cache() before reading a different cache! */
>   int do_read_index(struct index_state *istate, const char *path, int must_exist)
> @@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
>   	size_t mmap_size;
>   	struct load_index_extensions p;
>   	size_t extension_offset = 0;
> -#ifndef NO_PTHREADS
>   	int nr_threads, cpus;
>   	struct index_entry_offset_table *ieot = NULL;
> -#endif
>   
>   	if (istate->initialized)
>   		return istate->cache_nr;
> @@ -2181,15 +2172,18 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
>   
>   	src_offset = sizeof(*hdr);
>   
> -#ifndef NO_PTHREADS
> -	nr_threads = git_config_get_index_threads();
> +	if (HAVE_THREADS) {

In this case, nr_threads is already capped to online_cpus() below so this HAVE_THREADS test isn't needed.

Show 35 quoted lines
> +		nr_threads = git_config_get_index_threads();
>   
> -	/* TODO: does creating more threads than cores help? */
> -	if (!nr_threads) {
> -		nr_threads = istate->cache_nr / THREAD_COST;
> -		cpus = online_cpus();
> -		if (nr_threads > cpus)
> -			nr_threads = cpus;
> +		/* TODO: does creating more threads than cores help? */
> +		if (!nr_threads) {
> +			nr_threads = istate->cache_nr / THREAD_COST;
> +			cpus = online_cpus();
> +			if (nr_threads > cpus)
> +				nr_threads = cpus;
> +		}
> +	} else {
> +		nr_threads = 1;
>   	}
>   
>   	if (nr_threads > 1) {
> @@ -2219,21 +2213,16 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
>   	} else {
>   		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
>   	}
> -#else
> -	src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
> -#endif
>   
>   	istate->timestamp.sec = st.st_mtime;
>   	istate->timestamp.nsec = ST_MTIME_NSEC(st);
>   
>   	/* if we created a thread, join it otherwise load the extensions on the primary thread */
> -#ifndef NO_PTHREADS
> -	if (extension_offset) {
> +	if (HAVE_THREADS && extension_offset) {

Here extension_offset won't be set if there wasn't a thread created so the HAVE_THREADS test is not needed.

Show 15 quoted lines
>   		int ret = pthread_join(p.pthread, NULL);
>   		if (ret)
>   			die(_("unable to join load_index_extensions thread: %s"), strerror(ret));
>   	}
> -#endif
>   	if (!extension_offset) {
>   		p.src_offset = src_offset;
>   		load_index_extensions(&p);
> @@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>   	if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
>   		return -1;
>   
> -#ifndef NO_PTHREADS
> -	nr_threads = git_config_get_index_threads();
> +	if (HAVE_THREADS)

This shouldn't be needed either. My assumption was that if someone explicitly asked for multiple threads we're better off failing than silently ignoring their request. The default/automatic setting will detect the number of cpus and behave correctly.

Show 22 quoted lines
> +		nr_threads = git_config_get_index_threads();
> +	else
> +		nr_threads = 1;
> +
>   	if (nr_threads != 1) {
>   		int ieot_blocks, cpus;
>   
> @@ -2787,7 +2779,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>   			ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
>   		}
>   	}
> -#endif
>   
>   	offset = lseek(newfd, 0, SEEK_CUR);
>   	if (offset < 0) {
> @@ -2871,8 +2862,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>   	 * strip_extensions parameter as we need it when loading the shared
>   	 * index.
>   	 */
> -#ifndef NO_PTHREADS
> -	if (ieot) {
> +	if (HAVE_THREADS && ieot) {

Again, this one isn't needed. If there is only 1 thread, we don't read the eoie or the ieot (see the code just above this) so the ieot test is sufficient.

Show 25 quoted lines
>   		struct strbuf sb = STRBUF_INIT;
>   
>   		write_ieot_extension(&sb, ieot);
> @@ -2883,7 +2873,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>   		if (err)
>   			return -1;
>   	}
> -#endif
>   
>   	if (!strip_extensions && istate->split_index) {
>   		struct strbuf sb = STRBUF_INIT;
> @@ -3469,7 +3458,6 @@ static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context,
>   	strbuf_add(sb, hash, the_hash_algo->rawsz);
>   }
>   
> -#ifndef NO_PTHREADS
>   #define IEOT_VERSION	(1)
>   
>   static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset)
> @@ -3542,4 +3530,3 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta
>   	       strbuf_add(sb, &buffer, sizeof(uint32_t));
>          }
>   }
> -#endif
> 
Duy Nguyen· Oct 29, 2018, 17:21 UTC · re: Ben Peart · lore

Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS

On Mon, Oct 29, 2018 at 6:05 PM Ben Peart <peartben@gmail.com> wrote:
Show 12 quoted lines
> > @@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
> >       if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
> >               return -1;
> >
> > -#ifndef NO_PTHREADS
> > -     nr_threads = git_config_get_index_threads();
> > +     if (HAVE_THREADS)
>
> This shouldn't be needed either.  My assumption was that if someone
> explicitly asked for multiple threads we're better off failing than
> silently ignoring their request.  The default/automatic setting will
> detect the number of cpus and behave correctly.

No. I can have ~/.gitconfig shared between different machines. One with multithread support and one without. I should not have to fall back to conditional includes in order to use the same config file on the machine without multithread.

-- 
Duy
Ben Peart· Oct 29, 2018, 17:58 UTC · re: Duy Nguyen · lore

Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS

On 10/29/2018 1:21 PM, Duy Nguyen wrote:
Show 19 quoted lines
> On Mon, Oct 29, 2018 at 6:05 PM Ben Peart <peartben@gmail.com> wrote:
>>> @@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>>>        if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
>>>                return -1;
>>>
>>> -#ifndef NO_PTHREADS
>>> -     nr_threads = git_config_get_index_threads();
>>> +     if (HAVE_THREADS)
>>
>> This shouldn't be needed either.  My assumption was that if someone
>> explicitly asked for multiple threads we're better off failing than
>> silently ignoring their request.  The default/automatic setting will
>> detect the number of cpus and behave correctly.
> 
> No. I can have ~/.gitconfig shared between different machines. One
> with multithread support and one without. I should not have to fall
> back to conditional includes in order to use the same config file on
> the machine without multithread.
> 

If you want to share the ~/.gitconfig across machines that have different numbers of CPUs, it makes more sense to me to leave the setting at the "auto" setting rather than specifically overriding it to a number that won't work on at least one of your machines. Then it all "just works" without the need to conditional includes.

Junio C Hamano· Oct 30, 2018, 01:44 UTC · re: Duy Nguyen · lore

Re: [PATCH 09/10] read-cache.c: remove #ifdef NO_PTHREADS

Duy Nguyen <pclouds@gmail.com> writes:
Show 9 quoted 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.

Nguyễn Thái Ngọc Duy· Oct 27, 2018, 07:10 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 10/10] Clean up pthread_create() error handling

Normally 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.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c     | 16 ++++++++++------
 preload-index.c |  8 ++++++--
 run-command.c   |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)
Show changes to 3 files +17 −9

name-hash.c, preload-index.c, run-command.c

diff --git a/name-hash.c b/name-hash.c
index b3c9ac791d..623ca6923a 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -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();
 
diff --git a/preload-index.c b/preload-index.c
index 0e24886aca..ddca1c216e 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -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;
diff --git a/run-command.c b/run-command.c
index 84b883c213..26154ba257 100644
--- a/run-command.c
+++ b/run-command.c
@@ -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;
 		}
 	}
-- 
2.19.1.647.g708186aaf9
Jeff King· Oct 27, 2018, 07:24 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 00/10] Reduce #ifdef NO_PTHREADS

On Sat, Oct 27, 2018 at 09:09:53AM +0200, Nguyễn Thái Ngọc Duy wrote:
Show 6 quoted lines
> 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.

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
Jeff King· Oct 27, 2018, 08:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 00/10] Reduce #ifdef NO_PTHREADS

On Sat, Oct 27, 2018 at 09:09:53AM +0200, Nguyễn Thái Ngọc Duy wrote:
Show 20 quoted lines
> 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/20181018180522.17642-1-pclouds@gmail.com/
> 
> 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
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
Duy Nguyen· Oct 27, 2018, 17:07 UTC · re: Jeff King · lore

Re: [PATCH 00/10] Reduce #ifdef NO_PTHREADS

On Sat, Oct 27, 2018 at 10:13 AM Jeff King <peff@peff.net> wrote:
Show 37 quoted lines
>
> On Sat, Oct 27, 2018 at 09:09:53AM +0200, Nguyễn Thái Ngọc Duy wrote:
>
> > 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/20181018180522.17642-1-pclouds@gmail.com/
> >
> > 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
>
> 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 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.

-- 
Duy
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 17:29 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 00/10] Reduce #ifdef NO_PTHREADS

The 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 8
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 17:29 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 01/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

> 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".
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Makefile       |  2 +-
 thread-utils.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 thread-utils.h | 48 +++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 94 insertions(+), 4 deletions(-)
Show changes to 3 files +94 −4

Makefile, thread-utils.c, thread-utils.h

diff --git a/Makefile b/Makefile
index b08d5ea258..321540a736 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/thread-utils.c b/thread-utils.c
index a2135e0743..5329845691 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -20,6 +20,9 @@
 
 int online_cpus(void)
 {
+#ifdef NO_PTHREADS
+	return 1;
+#else
 #ifdef _SC_NPROCESSORS_ONLN
 	long ncpus;
 #endif
@@ -59,10 +62,12 @@ int online_cpus(void)
 #endif
 
 	return 1;
+#endif
 }
 
 int init_recursive_mutex(pthread_mutex_t *m)
 {
+#ifndef NO_PTHREADS
 	pthread_mutexattr_t a;
 	int ret;
 
@@ -74,4 +79,47 @@ int init_recursive_mutex(pthread_mutex_t *m)
 		pthread_mutexattr_destroy(&a);
 	}
 	return ret;
+#else
+	return 0;
+#endif
+}
+
+#ifdef NO_PTHREADS
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+int dummy_pthread_init(void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis or it may realize that functions like
+	 * pthread_mutex_init() is no-op, which means the (static)
+	 * variable is not used/initialized at all and trigger
+	 * -Wunused-variable
+	 */
+	return ENOSYS;
 }
+
+int dummy_pthread_join(pthread_t pthread, void **retval)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+#endif
diff --git a/thread-utils.h b/thread-utils.h
index d9a769d190..4961487ed9 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -4,12 +4,54 @@
 #ifndef NO_PTHREADS
 #include <pthread.h>
 
-extern int online_cpus(void);
-extern int init_recursive_mutex(pthread_mutex_t*);
+#define HAVE_THREADS 1
 
 #else
 
-#define online_cpus() 1
+#define HAVE_THREADS 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_key_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)
+
+#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)
+
+#define pthread_key_create(key, attr) dummy_pthread_init(key)
+#define pthread_key_delete(key)
+
+#define pthread_create(thread, attr, fn, data) \
+	dummy_pthread_create(thread, attr, fn, data)
+#define pthread_join(thread, retval) \
+	dummy_pthread_join(thread, retval)
+
+#define pthread_setspecific(key, data)
+#define pthread_getspecific(key) NULL
+
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data);
+int dummy_pthread_join(pthread_t pthread, void **retval);
+
+int dummy_pthread_init(void *);
 
 #endif
+
+int online_cpus(void);
+int init_recursive_mutex(pthread_mutex_t*);
+
+
 #endif /* THREAD_COMPAT_H */
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 17:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 02/10] index-pack: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c | 68 ++++++++++++--------------------------------
 1 file changed, 18 insertions(+), 50 deletions(-)
Show changes to builtin/index-pack.c +18 −50
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2004e25da2..bbd66ca025 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -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 &nothread_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();
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 17:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 04/10] attr.c: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 attr.c | 14 --------------
 1 file changed, 14 deletions(-)
Show changes to attr.c +0 −14
diff --git a/attr.c b/attr.c
index 60d284796d..eaece6658d 100644
--- a/attr.c
+++ b/attr.c
@@ -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
 }
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 17:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 03/10] name-hash.c: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)
Show changes to name-hash.c +4 −18
diff --git a/name-hash.c b/name-hash.c
index 1fcda73cb3..b3c9ac791d 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -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)
 {
 
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 17:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 05/10] grep: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 59 +++++++++++++++++++-------------------------------
 grep.c         |  6 -----
 grep.h         |  6 -----
 3 files changed, 22 insertions(+), 49 deletions(-)
Show changes to 3 files +22 −49

builtin/grep.c, grep.c, grep.h

diff --git a/builtin/grep.c b/builtin/grep.c
index d8508ddf79..6dd15dbaa2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -34,7 +34,6 @@ static int recurse_submodules;
 #define GREP_NUM_THREADS_DEFAULT 8
 static 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"));
diff --git a/grep.c b/grep.c
index f6bd89e40b..4db1510d16 100644
--- a/grep.c
+++ b/grep.c
@@ -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;
diff --git a/grep.h b/grep.h
index 1a57d12b90..fb04893721 100644
--- a/grep.h
+++ b/grep.h
@@ -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
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 17:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 preload-index.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)
Show changes to preload-index.c +2 −13
diff --git a/preload-index.c b/preload-index.c
index 9e7152ab14..0e24886aca 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -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,
-- 
2.19.1.647.g708186aaf9
Ben Peart· Oct 29, 2018, 17:21 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS

On 10/27/2018 1:30 PM, Nguyễn Thái Ngọc Duy wrote:
Show 46 quoted lines
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   preload-index.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/preload-index.c b/preload-index.c
> index 9e7152ab14..0e24886aca 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -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,
> 

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
Show changes to preload-index.c +5 −13
diff --git a/preload-index.c b/preload-index.c
index 9e7152ab14..266bc9d8d7 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -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,
Duy Nguyen· Oct 29, 2018, 17:26 UTC · re: Ben Peart · lore

Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS

On Mon, Oct 29, 2018 at 6:21 PM Ben Peart <peartben@gmail.com> wrote:
Show 11 quoted lines
> @@ -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.

-- 
Duy
Ben Peart· Oct 29, 2018, 18:05 UTC · re: Duy Nguyen · lore

Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS

On 10/29/2018 1:26 PM, Duy Nguyen wrote:
Show 20 quoted lines
> On Mon, Oct 29, 2018 at 6:21 PM Ben Peart <peartben@gmail.com> wrote:
>> @@ -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.
> 
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. :-)

Jeff King· Oct 29, 2018, 20:11 UTC · re: Ben Peart · lore

Re: [PATCH v2 06/10] preload-index.c: remove #ifdef NO_PTHREADS

On Mon, Oct 29, 2018 at 02:05:58PM -0400, Ben Peart wrote:
Show 29 quoted lines
> On 10/29/2018 1:26 PM, Duy Nguyen wrote:
> > On Mon, Oct 29, 2018 at 6:21 PM Ben Peart <peartben@gmail.com> wrote:
> > > @@ -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.
> > 
> 
> 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. :-)

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
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 17:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 07/10] pack-objects: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c | 26 ++------------------------
 pack-objects.h         |  6 ------
 2 files changed, 2 insertions(+), 30 deletions(-)
Show changes to 2 files +2 −30

builtin/pack-objects.c, pack-objects.h

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b059b86aee..12edd6da16 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -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)
diff --git a/pack-objects.h b/pack-objects.h
index 2ca39cfcfe..3a42727c7d 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -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,
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 17:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 08/10] read-cache.c: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c | 49 ++++++++++++++++++-------------------------------
 1 file changed, 18 insertions(+), 31 deletions(-)
Show changes to read-cache.c +18 −31
diff --git a/read-cache.c b/read-cache.c
index d57958233e..ba870bc3fd 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1920,19 +1920,15 @@ struct index_entry_offset_table
 	struct index_entry_offset entries[FLEX_ARRAY];
 };
 
-#ifndef NO_PTHREADS
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset);
 static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot);
-#endif
 
 static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
 static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset);
 
 struct load_index_extensions
 {
-#ifndef NO_PTHREADS
 	pthread_t pthread;
-#endif
 	struct index_state *istate;
 	const char *mmap;
 	size_t mmap_size;
@@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct index_state *istate,
 	return consumed;
 }
 
-#ifndef NO_PTHREADS
-
 /*
  * Mostly randomly chosen maximum thread counts: we
  * cap the parallelism to online_cpus() threads, and we want
@@ -2122,7 +2116,6 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
 
 	return consumed;
 }
-#endif
 
 /* remember to discard_cache() before reading a different cache! */
 int do_read_index(struct index_state *istate, const char *path, int must_exist)
@@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	size_t mmap_size;
 	struct load_index_extensions p;
 	size_t extension_offset = 0;
-#ifndef NO_PTHREADS
 	int nr_threads, cpus;
 	struct index_entry_offset_table *ieot = NULL;
-#endif
 
 	if (istate->initialized)
 		return istate->cache_nr;
@@ -2181,15 +2172,18 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 
 	src_offset = sizeof(*hdr);
 
-#ifndef NO_PTHREADS
-	nr_threads = git_config_get_index_threads();
+	if (HAVE_THREADS) {
+		nr_threads = git_config_get_index_threads();
 
-	/* TODO: does creating more threads than cores help? */
-	if (!nr_threads) {
-		nr_threads = istate->cache_nr / THREAD_COST;
-		cpus = online_cpus();
-		if (nr_threads > cpus)
-			nr_threads = cpus;
+		/* TODO: does creating more threads than cores help? */
+		if (!nr_threads) {
+			nr_threads = istate->cache_nr / THREAD_COST;
+			cpus = online_cpus();
+			if (nr_threads > cpus)
+				nr_threads = cpus;
+		}
+	} else {
+		nr_threads = 1;
 	}
 
 	if (nr_threads > 1) {
@@ -2219,21 +2213,16 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	} else {
 		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
 	}
-#else
-	src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
-#endif
 
 	istate->timestamp.sec = st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
 
 	/* if we created a thread, join it otherwise load the extensions on the primary thread */
-#ifndef NO_PTHREADS
-	if (extension_offset) {
+	if (HAVE_THREADS && extension_offset) {
 		int ret = pthread_join(p.pthread, NULL);
 		if (ret)
 			die(_("unable to join load_index_extensions thread: %s"), strerror(ret));
 	}
-#endif
 	if (!extension_offset) {
 		p.src_offset = src_offset;
 		load_index_extensions(&p);
@@ -2756,8 +2745,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
 		return -1;
 
-#ifndef NO_PTHREADS
-	nr_threads = git_config_get_index_threads();
+	if (HAVE_THREADS)
+		nr_threads = git_config_get_index_threads();
+	else
+		nr_threads = 1;
+
 	if (nr_threads != 1) {
 		int ieot_blocks, cpus;
 
@@ -2787,7 +2779,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 			ieot_entries = DIV_ROUND_UP(entries, ieot_blocks);
 		}
 	}
-#endif
 
 	offset = lseek(newfd, 0, SEEK_CUR);
 	if (offset < 0) {
@@ -2871,8 +2862,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	 * strip_extensions parameter as we need it when loading the shared
 	 * index.
 	 */
-#ifndef NO_PTHREADS
-	if (ieot) {
+	if (HAVE_THREADS && ieot) {
 		struct strbuf sb = STRBUF_INIT;
 
 		write_ieot_extension(&sb, ieot);
@@ -2883,7 +2873,6 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 		if (err)
 			return -1;
 	}
-#endif
 
 	if (!strip_extensions && istate->split_index) {
 		struct strbuf sb = STRBUF_INIT;
@@ -3469,7 +3458,6 @@ static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context,
 	strbuf_add(sb, hash, the_hash_algo->rawsz);
 }
 
-#ifndef NO_PTHREADS
 #define IEOT_VERSION	(1)
 
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset)
@@ -3542,4 +3530,3 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta
 	       strbuf_add(sb, &buffer, sizeof(uint32_t));
        }
 }
-#endif
-- 
2.19.1.647.g708186aaf9
Jeff King· Oct 29, 2018, 14:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 08/10] read-cache.c: remove #ifdef NO_PTHREADS

On Sat, Oct 27, 2018 at 07:30:06PM +0200, Nguyễn Thái Ngọc Duy wrote:
Show 21 quoted lines
> -#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;
>  	}

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
Ben Peart· Oct 29, 2018, 17:07 UTC · re: Jeff King · lore

Re: [PATCH v2 08/10] read-cache.c: remove #ifdef NO_PTHREADS

On 10/29/2018 10:30 AM, Jeff King wrote:
Show 31 quoted lines
> On Sat, Oct 27, 2018 at 07:30:06PM +0200, Nguyễn Thái Ngọc Duy wrote:
> 
>> -#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;
>>   	}
> 
> 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
> 
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.

Ben Peart· Oct 29, 2018, 17:23 UTC · re: Jeff King · lore

Re: [PATCH v2 08/10] read-cache.c: remove #ifdef NO_PTHREADS

On 10/29/2018 10:30 AM, Jeff King wrote:
Show 31 quoted lines
> On Sat, Oct 27, 2018 at 07:30:06PM +0200, Nguyễn Thái Ngọc Duy wrote:
> 
>> -#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;
>>   	}
> 
> 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
> 

And here is the patch I created when testing out the idea of removing NO_PTHREADS. It doesn't require any 'HAVE_THREADS' tests.

Show changes to read-cache.c +5 −30
diff --git a/read-cache.c b/read-cache.c
index 1df5c16dbc..1f088799fc 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1920,19 +1920,15 @@ struct index_entry_offset_table
         struct index_entry_offset entries[FLEX_ARRAY];
  };

-#ifndef NO_PTHREADS
  static struct index_entry_offset_table *read_ieot_extension(const char 
*mmap, size_t mmap_size, size_t offset
);
  static void write_ieot_extension(struct strbuf *sb, struct 
index_entry_offset_table *ieot);
-#endif

  static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
  static void write_eoie_extension(struct strbuf *sb, git_hash_ctx 
*eoie_context, size_t offset);

  struct load_index_extensions
  {
-#ifndef NO_PTHREADS
         pthread_t pthread;
-#endif
         struct index_state *istate;
         const char *mmap;
         size_t mmap_size;
@@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct 
index_state *istate,
         return consumed;
  }

-#ifndef NO_PTHREADS
-
  /*
   * Mostly randomly chosen maximum thread counts: we
   * cap the parallelism to online_cpus() threads, and we want
@@ -2122,7 +2116,6 @@ static unsigned long 
load_cache_entries_threaded(struct index_state *istate, con


         return consumed;
  }
-#endif

  /* remember to discard_cache() before reading a different cache! */
  int do_read_index(struct index_state *istate, const char *path, int 
must_exist)
@@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, 
const char *path, int must_exist)

         size_t mmap_size;
         struct load_index_extensions p;
         size_t extension_offset = 0;
-#ifndef NO_PTHREADS
         int nr_threads, cpus;
         struct index_entry_offset_table *ieot = NULL;
-#endif

         if (istate->initialized)
                 return istate->cache_nr;
@@ -2181,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
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 17:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 09/10] Clean up pthread_create() error handling

Normally 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.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c     | 16 ++++++++++------
 preload-index.c |  8 ++++++--
 run-command.c   |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)
Show changes to 3 files +17 −9

name-hash.c, preload-index.c, run-command.c

diff --git a/name-hash.c b/name-hash.c
index b3c9ac791d..623ca6923a 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -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();
 
diff --git a/preload-index.c b/preload-index.c
index 0e24886aca..ddca1c216e 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -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;
diff --git a/run-command.c b/run-command.c
index 84b883c213..26154ba257 100644
--- a/run-command.c
+++ b/run-command.c
@@ -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;
 		}
 	}
-- 
2.19.1.647.g708186aaf9
Nguyễn Thái Ngọc Duy· Oct 27, 2018, 17:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 10/10] read-cache.c: initialize copy_len to shut up gcc 8

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 <pclouds@gmail.com>
---
 read-cache.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
Show changes to read-cache.c +1 −3
diff --git a/read-cache.c b/read-cache.c
index ba870bc3fd..4307b9a7bf 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -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;
 	}
-- 
2.19.1.647.g708186aaf9
Jeff King· Oct 29, 2018, 14:31 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 10/10] read-cache.c: initialize copy_len to shut up gcc 8

On Sat, Oct 27, 2018 at 07:30:08PM +0200, Nguyễn Thái Ngọc Duy wrote:
Show 32 quoted lines
> 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 <pclouds@gmail.com>
> ---
>  read-cache.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index ba870bc3fd..4307b9a7bf 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -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;
>  	}

Yes, this silences the compiler warning I saw (and is exactly the same patch I wrote to get past it the other day ;) ).

-Peff
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 00/14] Reduce #ifdef NO_PTHREADS

Changes 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
Show changes to 6 files +57 −59

builtin/grep.c, builtin/index-pack.c, read-cache.c, run-command.c, run-command.h, send-pack.c

diff --git a/builtin/grep.c b/builtin/grep.c
index 6dd15dbaa2..de3f568cee 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -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);
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index bbd66ca025..682042579b 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -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=")) {
diff --git a/read-cache.c b/read-cache.c
index 4307b9a7bf..c510f598b1 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -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);
diff --git a/run-command.c b/run-command.c
index 26154ba257..decf3239bd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -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;
diff --git a/run-command.h b/run-command.h
index 3932420ec8..68f5369fc2 100644
--- a/run-command.h
+++ b/run-command.h
@@ -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);
 
 /**
diff --git a/send-pack.c b/send-pack.c
index e920ca57df..f692686770 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -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(-)
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 01/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

> 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".
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Makefile       |  2 +-
 thread-utils.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 thread-utils.h | 48 +++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 94 insertions(+), 4 deletions(-)
Show changes to 3 files +94 −4

Makefile, thread-utils.c, thread-utils.h

diff --git a/Makefile b/Makefile
index b08d5ea258..321540a736 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/thread-utils.c b/thread-utils.c
index a2135e0743..5329845691 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -20,6 +20,9 @@
 
 int online_cpus(void)
 {
+#ifdef NO_PTHREADS
+	return 1;
+#else
 #ifdef _SC_NPROCESSORS_ONLN
 	long ncpus;
 #endif
@@ -59,10 +62,12 @@ int online_cpus(void)
 #endif
 
 	return 1;
+#endif
 }
 
 int init_recursive_mutex(pthread_mutex_t *m)
 {
+#ifndef NO_PTHREADS
 	pthread_mutexattr_t a;
 	int ret;
 
@@ -74,4 +79,47 @@ int init_recursive_mutex(pthread_mutex_t *m)
 		pthread_mutexattr_destroy(&a);
 	}
 	return ret;
+#else
+	return 0;
+#endif
+}
+
+#ifdef NO_PTHREADS
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+int dummy_pthread_init(void *data)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis or it may realize that functions like
+	 * pthread_mutex_init() is no-op, which means the (static)
+	 * variable is not used/initialized at all and trigger
+	 * -Wunused-variable
+	 */
+	return ENOSYS;
 }
+
+int dummy_pthread_join(pthread_t pthread, void **retval)
+{
+	/*
+	 * Do nothing.
+	 *
+	 * The main purpose of this function is to break compiler's
+	 * flow analysis and avoid -Wunused-variable false warnings.
+	 */
+	return ENOSYS;
+}
+
+#endif
diff --git a/thread-utils.h b/thread-utils.h
index d9a769d190..4961487ed9 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -4,12 +4,54 @@
 #ifndef NO_PTHREADS
 #include <pthread.h>
 
-extern int online_cpus(void);
-extern int init_recursive_mutex(pthread_mutex_t*);
+#define HAVE_THREADS 1
 
 #else
 
-#define online_cpus() 1
+#define HAVE_THREADS 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_key_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)
+
+#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)
+
+#define pthread_key_create(key, attr) dummy_pthread_init(key)
+#define pthread_key_delete(key)
+
+#define pthread_create(thread, attr, fn, data) \
+	dummy_pthread_create(thread, attr, fn, data)
+#define pthread_join(thread, retval) \
+	dummy_pthread_join(thread, retval)
+
+#define pthread_setspecific(key, data)
+#define pthread_getspecific(key) NULL
+
+int dummy_pthread_create(pthread_t *pthread, const void *attr,
+			 void *(*fn)(void *), void *data);
+int dummy_pthread_join(pthread_t pthread, void **retval);
+
+int dummy_pthread_init(void *);
 
 #endif
+
+int online_cpus(void);
+int init_recursive_mutex(pthread_mutex_t*);
+
+
 #endif /* THREAD_COMPAT_H */
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 02/14] run-command.h: include thread-utils.h instead of pthread.h

run-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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 run-command.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
Show changes to run-command.h +1 −3
diff --git a/run-command.h b/run-command.h
index 3932420ec8..9b7f38202c 100644
--- a/run-command.h
+++ b/run-command.h
@@ -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"
 
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 03/14] send-pack.c: move async's #ifdef NO_PTHREADS back to run-command.c

On 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)
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 run-command.c | 9 +++++++++
 run-command.h | 1 +
 send-pack.c   | 5 ++---
 3 files changed, 12 insertions(+), 3 deletions(-)
Show changes to 3 files +12 −3

run-command.c, run-command.h, send-pack.c

diff --git a/run-command.c b/run-command.c
index 84b883c213..3c3b8814df 100644
--- a/run-command.c
+++ b/run-command.c
@@ -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;
diff --git a/run-command.h b/run-command.h
index 9b7f38202c..68f5369fc2 100644
--- a/run-command.h
+++ b/run-command.h
@@ -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);
 
 /**
diff --git a/send-pack.c b/send-pack.c
index e920ca57df..f692686770 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -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;
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 04/14] index-pack: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c | 63 ++++++++++----------------------------------
 1 file changed, 14 insertions(+), 49 deletions(-)
Show changes to builtin/index-pack.c +14 −49
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2004e25da2..682042579b 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -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 &nothread_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();
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 05/14] name-hash.c: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)
Show changes to name-hash.c +4 −18
diff --git a/name-hash.c b/name-hash.c
index 1fcda73cb3..b3c9ac791d 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -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)
 {
 
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 06/14] attr.c: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 attr.c | 14 --------------
 1 file changed, 14 deletions(-)
Show changes to attr.c +0 −14
diff --git a/attr.c b/attr.c
index 60d284796d..eaece6658d 100644
--- a/attr.c
+++ b/attr.c
@@ -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
 }
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 07/14] grep: remove #ifdef NO_PTHREADS

This is a faithful conversion without attempting to improve anything. That comes later.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 59 +++++++++++++++++++-------------------------------
 grep.c         |  6 -----
 grep.h         |  6 -----
 3 files changed, 22 insertions(+), 49 deletions(-)
Show changes to 3 files +22 −49

builtin/grep.c, grep.c, grep.h

diff --git a/builtin/grep.c b/builtin/grep.c
index d8508ddf79..6dd15dbaa2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -34,7 +34,6 @@ static int recurse_submodules;
 #define GREP_NUM_THREADS_DEFAULT 8
 static 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"));
diff --git a/grep.c b/grep.c
index f6bd89e40b..4db1510d16 100644
--- a/grep.c
+++ b/grep.c
@@ -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;
diff --git a/grep.h b/grep.h
index 1a57d12b90..fb04893721 100644
--- a/grep.h
+++ b/grep.h
@@ -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
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 09/14] preload-index.c: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 preload-index.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)
Show changes to preload-index.c +2 −13
diff --git a/preload-index.c b/preload-index.c
index 9e7152ab14..0e24886aca 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -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,
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 08/14] grep: clean up num_threads handling

When 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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 58 +++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 31 deletions(-)
Show changes to builtin/grep.c +27 −31
diff --git a/builtin/grep.c b/builtin/grep.c
index 6dd15dbaa2..de3f568cee 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -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);
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 10/14] pack-objects: remove #ifdef NO_PTHREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/pack-objects.c | 26 ++------------------------
 pack-objects.h         |  6 ------
 2 files changed, 2 insertions(+), 30 deletions(-)
Show changes to 2 files +2 −30

builtin/pack-objects.c, pack-objects.h

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b059b86aee..12edd6da16 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -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)
diff --git a/pack-objects.h b/pack-objects.h
index 2ca39cfcfe..3a42727c7d 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -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,
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 11/14] read-cache.c: remove #ifdef NO_PTHREADS

This 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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)
Show changes to read-cache.c +10 −24
diff --git a/read-cache.c b/read-cache.c
index d57958233e..40fc0cb65f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1920,19 +1920,15 @@ struct index_entry_offset_table
 	struct index_entry_offset entries[FLEX_ARRAY];
 };
 
-#ifndef NO_PTHREADS
 static struct index_entry_offset_table *read_ieot_extension(const char *mmap, size_t mmap_size, size_t offset);
 static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot);
-#endif
 
 static size_t read_eoie_extension(const char *mmap, size_t mmap_size);
 static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset);
 
 struct load_index_extensions
 {
-#ifndef NO_PTHREADS
 	pthread_t pthread;
-#endif
 	struct index_state *istate;
 	const char *mmap;
 	size_t mmap_size;
@@ -2010,8 +2006,6 @@ static unsigned long load_all_cache_entries(struct index_state *istate,
 	return consumed;
 }
 
-#ifndef NO_PTHREADS
-
 /*
  * Mostly randomly chosen maximum thread counts: we
  * cap the parallelism to online_cpus() threads, and we want
@@ -2122,7 +2116,6 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
 
 	return consumed;
 }
-#endif
 
 /* remember to discard_cache() before reading a different cache! */
 int do_read_index(struct index_state *istate, const char *path, int must_exist)
@@ -2135,10 +2128,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	size_t mmap_size;
 	struct load_index_extensions p;
 	size_t extension_offset = 0;
-#ifndef NO_PTHREADS
 	int nr_threads, cpus;
 	struct index_entry_offset_table *ieot = NULL;
-#endif
 
 	if (istate->initialized)
 		return istate->cache_nr;
@@ -2181,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_ta
 	       strbuf_add(sb, &buffer, sizeof(uint32_t));
        }
 }
-#endif
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 12/14] read-cache.c: reduce branching based on HAVE_THREADS

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)
Show changes to read-cache.c +9 −10
diff --git a/read-cache.c b/read-cache.c
index 40fc0cb65f..00cd416816 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -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);
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 13/14] read-cache.c: initialize copy_len to shut up gcc 8

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 <pclouds@gmail.com>
---
 read-cache.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
Show changes to read-cache.c +1 −3
diff --git a/read-cache.c b/read-cache.c
index 00cd416816..c510f598b1 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -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;
 	}
-- 
2.19.1.1005.gac84295441
Nguyễn Thái Ngọc Duy· Nov 3, 2018, 08:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 14/14] Clean up pthread_create() error handling

Normally 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.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 name-hash.c     | 16 ++++++++++------
 preload-index.c |  8 ++++++--
 run-command.c   |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)
Show changes to 3 files +17 −9

name-hash.c, preload-index.c, run-command.c

diff --git a/name-hash.c b/name-hash.c
index b3c9ac791d..623ca6923a 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -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();
 
diff --git a/preload-index.c b/preload-index.c
index 0e24886aca..ddca1c216e 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -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;
diff --git a/run-command.c b/run-command.c
index 3c3b8814df..decf3239bd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -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;
 		}
 	}
-- 
2.19.1.1005.gac84295441
Junio C Hamano· Nov 6, 2018, 04:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v3 00/14] Reduce #ifdef NO_PTHREADS

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 5 quoted lines
> Changes 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

The patches all looked sensible; I'll wait for a few more days to see if anybody else spots something.

Thanks.

← back to recent threads