[01/10] thread-utils: macros to unconditionally compile pthreads API
diff mbox series

Message ID 20181027071003.1347-2-pclouds@gmail.com
State New
Headers show
Series
  • Reduce #ifdef NO_PTHREADS
Related show

Commit Message

Duy Nguyen Oct. 27, 2018, 7:09 a.m. UTC
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(-)

Comments

Jeff King Oct. 27, 2018, 7:31 a.m. UTC | #1
On Sat, Oct 27, 2018 at 09:09:54AM +0200, Nguyễn Thái Ngọc Duy wrote:

> +/*
> + * 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.

> +#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, 7:40 a.m. UTC | #2
On Sat, Oct 27, 2018 at 9:31 AM Jeff King <peff@peff.net> wrote:
> > +#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.
Jeff King Oct. 27, 2018, 8:15 a.m. UTC | #3
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.

> 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, 2:43 p.m. UTC | #4
On Sat, Oct 27, 2018 at 10:15 AM Jeff King <peff@peff.net> wrote:
>
> 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

Patch
diff mbox series

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 */