Re: new module 'thread-optim'

[Bruno Haible]

Paul Eggert wrote:
> True enough, but how about suggesting that people write something like the 
> following?
> 
>     bool mt = gl_multithreaded ();
>     if (mt) gl_lock_lock (file_cleanup_list_lock);
>     ...
>     if (mt) gl_lock_unlock (file_cleanup_list_lock);
> 
> This is nearly as concise as the IF_MT_DECL and IF_MT macros, allows for 
> further 
> enhancements by builtin_expect etc., and avoids the macro hackery.

Indeed. With only function-like macros, the source code is more transparent
and probably as future-proof.

I'm applying your suggestion.


2020-08-12  Bruno Haible  <bruno@clisp.org>

        thread-optim: Export function-like macros only.
        Suggested by Paul Eggert.
        * lib/thread-optim.h (gl_multithreaded): New macro.
        (IF_MT_DECL, IF_MT): Remove macros.
        * doc/multithread.texi (Multithreading Optimizations): Add a small
        example.
        * lib/fatal-signal.c: Update all uses.
        * lib/clean-temp.c: Likewise.
        * lib/localename.c: Likewise.
        * modules/localename (Depends-on): Add stdbool.

diff --git a/lib/thread-optim.h b/lib/thread-optim.h
index 0cdcfec..b649c50 100644
--- a/lib/thread-optim.h
+++ b/lib/thread-optim.h
@@ -26,23 +26,25 @@
 
 /* Typical use: In a block or function, use
 
-     IF_MT_DECL;
+     bool mt = gl_multithreaded ();
      ...
-     IF_MT
+     if (mt)
        if (pthread_mutex_lock (&lock)) abort ();
      ...
-     IF_MT
+     if (mt)
        if (pthread_mutex_unlock (&lock)) abort ();
 
-   The macro IF_MT_DECL establishes local variables for use by IF_MT.
+   The gl_multithreaded () invocation determines whether the program currently
+   is multithreaded.
 
-   IF_MT STATEMENT executes STATEMENT in the multithreaded cases, and skips it
-   in the single-threaded case.
+   if (mt) STATEMENT executes STATEMENT in the multithreaded case, and skips
+   it in the single-threaded case.
 
-   The code between IF_MT_DECL and IF_MT must not create threads or invoke
-   functions that may indirectly create threads (e.g. 'dlopen' may, indirectly
-   through C++ initializers of global variables in the shared library being
-   opened, create threads).
+   The code between the gl_multithreaded () invocation and any use of the
+   variable 'mt' must not create threads or invoke functions that may
+   indirectly create threads (e.g. 'dlopen' may, indirectly through C++
+   initializers of global variables in the shared library being opened,
+   create threads).
 
    The lock here is meant to synchronize threads in the same process.  The
    same optimization cannot be applied to locks that synchronize different
@@ -50,11 +52,9 @@
 
 #if HAVE_SYS_SINGLE_THREADED_H /* glibc >= 2.32 */
 # include <sys/single_threaded.h>
-# define IF_MT_DECL  char optimize_for_single_thread = __libc_single_threaded
-# define IF_MT       if (!optimize_for_single_thread)
+# define gl_multithreaded()  !__libc_single_threaded
 #else
-# define IF_MT_DECL  (void)0
-# define IF_MT
+# define gl_multithreaded()  1
 #endif
 
 #endif /* _THREAD_OPTIM_H */
diff --git a/doc/multithread.texi b/doc/multithread.texi
index 022a6cb..9b1bc2e 100644
--- a/doc/multithread.texi
+++ b/doc/multithread.texi
@@ -249,8 +249,14 @@ to all the Gnulib multithreading API (locks, thread-local 
storage, and more).
 The @code{thread-optim} module, on glibc @geq{} 2.32 systems, allows your code
 to skip locking between threads (regardless which of the three multithreading
 APIs you use).  You need extra code for this: include the
-@code{"thread-optim.h"} header file, and use the macros @code{IF_MT_DECL}
-and @code{IF_MT}.
+@code{"thread-optim.h"} header file, and use the macro @code{gl_multithreaded}
+like this:
+@smallexample
+bool mt = gl_multithreaded ();
+if (mt) gl_lock_lock (some_lock);
+...
+if (mt) gl_lock_unlock (some_lock);
+@end smallexample
 @item
 The @code{unlocked-io} module is applicable only if all the programs in your
 package are single-threaded.  It optimizes the operations on @code{FILE}
diff --git a/lib/clean-temp.c b/lib/clean-temp.c
index 605390b..4a992c4 100644
--- a/lib/clean-temp.c
+++ b/lib/clean-temp.c
@@ -407,9 +407,9 @@ init_clean_temp (void)
 void
 register_temporary_file (const char *absolute_file_name)
 {
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (file_cleanup_list_lock);
+  if (mt) gl_lock_lock (file_cleanup_list_lock);
 
   /* Make sure that this facility and the file_cleanup_list are initialized.  
*/
   if (file_cleanup_list == NULL)
@@ -424,7 +424,7 @@ register_temporary_file (const char *absolute_file_name)
   if (gl_list_search (file_cleanup_list, absolute_file_name) == NULL)
     gl_list_add_first (file_cleanup_list, xstrdup (absolute_file_name));
 
-  IF_MT gl_lock_unlock (file_cleanup_list_lock);
+  if (mt) gl_lock_unlock (file_cleanup_list_lock);
 }
 
 /* Unregister the given ABSOLUTE_FILE_NAME as being a file that needs to be
@@ -433,9 +433,9 @@ register_temporary_file (const char *absolute_file_name)
 void
 unregister_temporary_file (const char *absolute_file_name)
 {
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (file_cleanup_list_lock);
+  if (mt) gl_lock_lock (file_cleanup_list_lock);
 
   gl_list_t list = file_cleanup_list;
   if (list != NULL)
@@ -450,7 +450,7 @@ unregister_temporary_file (const char *absolute_file_name)
         }
     }
 
-  IF_MT gl_lock_unlock (file_cleanup_list_lock);
+  if (mt) gl_lock_unlock (file_cleanup_list_lock);
 }
 
 /* Remove a file, with optional error message.
@@ -498,9 +498,9 @@ struct temp_dir *
 create_temp_dir (const char *prefix, const char *parentdir,
                  bool cleanup_verbose)
 {
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (dir_cleanup_list_lock);
+  if (mt) gl_lock_lock (dir_cleanup_list_lock);
 
   struct tempdir * volatile *tmpdirp = NULL;
   struct tempdir *tmpdir;
@@ -607,12 +607,12 @@ create_temp_dir (const char *prefix, const char 
*parentdir,
      block because then the cleanup handler would not remove the directory
      if xstrdup fails.  */
   tmpdir->dirname = xstrdup (tmpdirname);
-  IF_MT gl_lock_unlock (dir_cleanup_list_lock);
+  if (mt) gl_lock_unlock (dir_cleanup_list_lock);
   freea (xtemplate);
   return (struct temp_dir *) tmpdir;
 
  quit:
-  IF_MT gl_lock_unlock (dir_cleanup_list_lock);
+  if (mt) gl_lock_unlock (dir_cleanup_list_lock);
   freea (xtemplate);
   return NULL;
 }
@@ -625,15 +625,15 @@ register_temp_file (struct temp_dir *dir,
                     const char *absolute_file_name)
 {
   struct tempdir *tmpdir = (struct tempdir *)dir;
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (dir_cleanup_list_lock);
+  if (mt) gl_lock_lock (dir_cleanup_list_lock);
 
   /* Add absolute_file_name to tmpdir->files, without duplicates.  */
   if (gl_list_search (tmpdir->files, absolute_file_name) == NULL)
     gl_list_add_first (tmpdir->files, xstrdup (absolute_file_name));
 
-  IF_MT gl_lock_unlock (dir_cleanup_list_lock);
+  if (mt) gl_lock_unlock (dir_cleanup_list_lock);
 }
 
 /* Unregister the given ABSOLUTE_FILE_NAME as being a file inside DIR, that
@@ -644,9 +644,9 @@ unregister_temp_file (struct temp_dir *dir,
                       const char *absolute_file_name)
 {
   struct tempdir *tmpdir = (struct tempdir *)dir;
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (dir_cleanup_list_lock);
+  if (mt) gl_lock_lock (dir_cleanup_list_lock);
 
   gl_list_t list = tmpdir->files;
   gl_list_node_t node;
@@ -660,7 +660,7 @@ unregister_temp_file (struct temp_dir *dir,
       free (old_string);
     }
 
-  IF_MT gl_lock_unlock (dir_cleanup_list_lock);
+  if (mt) gl_lock_unlock (dir_cleanup_list_lock);
 }
 
 /* Register the given ABSOLUTE_DIR_NAME as being a subdirectory inside DIR,
@@ -671,15 +671,15 @@ register_temp_subdir (struct temp_dir *dir,
                       const char *absolute_dir_name)
 {
   struct tempdir *tmpdir = (struct tempdir *)dir;
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (dir_cleanup_list_lock);
+  if (mt) gl_lock_lock (dir_cleanup_list_lock);
 
   /* Add absolute_dir_name to tmpdir->subdirs, without duplicates.  */
   if (gl_list_search (tmpdir->subdirs, absolute_dir_name) == NULL)
     gl_list_add_first (tmpdir->subdirs, xstrdup (absolute_dir_name));
 
-  IF_MT gl_lock_unlock (dir_cleanup_list_lock);
+  if (mt) gl_lock_unlock (dir_cleanup_list_lock);
 }
 
 /* Unregister the given ABSOLUTE_DIR_NAME as being a subdirectory inside DIR,
@@ -691,9 +691,9 @@ unregister_temp_subdir (struct temp_dir *dir,
                         const char *absolute_dir_name)
 {
   struct tempdir *tmpdir = (struct tempdir *)dir;
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (dir_cleanup_list_lock);
+  if (mt) gl_lock_lock (dir_cleanup_list_lock);
 
   gl_list_t list = tmpdir->subdirs;
   gl_list_node_t node;
@@ -707,7 +707,7 @@ unregister_temp_subdir (struct temp_dir *dir,
       free (old_string);
     }
 
-  IF_MT gl_lock_unlock (dir_cleanup_list_lock);
+  if (mt) gl_lock_unlock (dir_cleanup_list_lock);
 }
 
 /* Remove a directory, with optional error message.
@@ -803,9 +803,9 @@ cleanup_temp_dir_contents (struct temp_dir *dir)
 int
 cleanup_temp_dir (struct temp_dir *dir)
 {
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (dir_cleanup_list_lock);
+  if (mt) gl_lock_lock (dir_cleanup_list_lock);
 
   struct tempdir *tmpdir = (struct tempdir *)dir;
   int err = 0;
@@ -832,7 +832,7 @@ cleanup_temp_dir (struct temp_dir *dir)
         gl_list_free (tmpdir->subdirs);
         free (tmpdir->dirname);
         free (tmpdir);
-        IF_MT gl_lock_unlock (dir_cleanup_list_lock);
+        if (mt) gl_lock_unlock (dir_cleanup_list_lock);
         return err;
       }
 
@@ -879,9 +879,9 @@ supports_delete_on_close ()
 static void
 register_fd (int fd)
 {
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (descriptors_lock);
+  if (mt) gl_lock_lock (descriptors_lock);
 
   if (descriptors == NULL)
     descriptors = gl_list_create_empty (GL_LINKED_LIST, NULL, NULL, NULL,
@@ -895,7 +895,7 @@ register_fd (int fd)
 
   gl_list_add_first (descriptors, element);
 
-  IF_MT gl_lock_unlock (descriptors_lock);
+  if (mt) gl_lock_unlock (descriptors_lock);
 }
 
 /* Open a temporary file in a temporary directory.
@@ -1041,9 +1041,9 @@ close_temp (int fd)
   int result = 0;
   int saved_errno = 0;
 
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (descriptors_lock);
+  if (mt) gl_lock_lock (descriptors_lock);
 
   gl_list_t list = descriptors;
   if (list == NULL)
@@ -1089,7 +1089,7 @@ close_temp (int fd)
     /* descriptors should already contain fd.  */
     abort ();
 
-  IF_MT gl_lock_unlock (descriptors_lock);
+  if (mt) gl_lock_unlock (descriptors_lock);
 
   errno = saved_errno;
   return result;
@@ -1105,9 +1105,9 @@ fclose_variant_temp (FILE *fp, int (*fclose_variant) 
(FILE *))
   int result = 0;
   int saved_errno = 0;
 
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (descriptors_lock);
+  if (mt) gl_lock_lock (descriptors_lock);
 
   gl_list_t list = descriptors;
   if (list == NULL)
@@ -1153,7 +1153,7 @@ fclose_variant_temp (FILE *fp, int (*fclose_variant) 
(FILE *))
     /* descriptors should have contained fd.  */
     abort ();
 
-  IF_MT gl_lock_unlock (descriptors_lock);
+  if (mt) gl_lock_unlock (descriptors_lock);
 
   errno = saved_errno;
   return result;
diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
index 541c4cb..14ecfe7 100644
--- a/lib/fatal-signal.c
+++ b/lib/fatal-signal.c
@@ -214,9 +214,9 @@ gl_lock_define_initialized (static, at_fatal_signal_lock)
 void
 at_fatal_signal (action_t action)
 {
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (at_fatal_signal_lock);
+  if (mt) gl_lock_lock (at_fatal_signal_lock);
 
   static bool cleanup_initialized = false;
   if (!cleanup_initialized)
@@ -263,7 +263,7 @@ at_fatal_signal (action_t action)
   actions[actions_count].action = action;
   actions_count++;
 
-  IF_MT gl_lock_unlock (at_fatal_signal_lock);
+  if (mt) gl_lock_unlock (at_fatal_signal_lock);
 }
 
 
@@ -303,9 +303,9 @@ static unsigned int fatal_signals_block_counter = 0;
 void
 block_fatal_signals (void)
 {
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (fatal_signals_block_lock);
+  if (mt) gl_lock_lock (fatal_signals_block_lock);
 
   if (fatal_signals_block_counter++ == 0)
     {
@@ -313,16 +313,16 @@ block_fatal_signals (void)
       sigprocmask (SIG_BLOCK, &fatal_signal_set, NULL);
     }
 
-  IF_MT gl_lock_unlock (fatal_signals_block_lock);
+  if (mt) gl_lock_unlock (fatal_signals_block_lock);
 }
 
 /* Stop delaying the catchable fatal signals.  */
 void
 unblock_fatal_signals (void)
 {
-  IF_MT_DECL;
+  bool mt = gl_multithreaded ();
 
-  IF_MT gl_lock_lock (fatal_signals_block_lock);
+  if (mt) gl_lock_lock (fatal_signals_block_lock);
 
   if (fatal_signals_block_counter == 0)
     /* There are more calls to unblock_fatal_signals() than to
@@ -334,7 +334,7 @@ unblock_fatal_signals (void)
       sigprocmask (SIG_UNBLOCK, &fatal_signal_set, NULL);
     }
 
-  IF_MT gl_lock_unlock (fatal_signals_block_lock);
+  if (mt) gl_lock_unlock (fatal_signals_block_lock);
 }
 
 
diff --git a/lib/localename.c b/lib/localename.c
index 2e76c11..5731ceb 100644
--- a/lib/localename.c
+++ b/lib/localename.c
@@ -28,6 +28,7 @@
 #endif
 
 #include <limits.h>
+#include <stdbool.h>
 #include <stddef.h>
 #include <stdlib.h>
 #include <locale.h>
@@ -2699,9 +2700,9 @@ struniq (const char *string)
     return "C";
   memcpy (new_node->contents, string, size);
   {
-    IF_MT_DECL;
+    bool mt = gl_multithreaded ();
     /* Lock while inserting new_node.  */
-    IF_MT gl_lock_lock (struniq_lock);
+    if (mt) gl_lock_lock (struniq_lock);
     /* Check whether another thread already added the string while we were
        waiting on the lock.  */
     for (p = struniq_hash_table[slot]; p != NULL; p = p->next)
@@ -2717,7 +2718,7 @@ struniq (const char *string)
     struniq_hash_table[slot] = new_node;
    done:
     /* Unlock after new_node is inserted.  */
-    IF_MT gl_lock_unlock (struniq_lock);
+    if (mt) gl_lock_unlock (struniq_lock);
   }
   return new_node->contents;
 }
diff --git a/modules/localename b/modules/localename
index 28710d0..b0e84e3 100644
--- a/modules/localename
+++ b/modules/localename
@@ -13,6 +13,7 @@ m4/lcmessage.m4
 
 Depends-on:
 extensions
+stdbool
 locale
 flexmember
 strdup