[review] Implement a thread pool

Message ID gerrit.1571543710000.I597bb642780cb9d578ca92373d2a638efb44fe52@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Oct. 20, 2019, 3:55 a.m. UTC
  Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172
......................................................................

Implement a thread pool

This adds a simple thread pool to gdb.  In the end, this seemed
preferable to the approach taken in an earlier version of this series;
namely, starting threads in the parallel-foreach implementation.  This
approach reduces the overhead of starting new threads, and also lets
the user control (in a subsequent patch) exactly how many worker
threads are running.

gdb/ChangeLog
2019-10-19  Christian Biesinger  <cbiesinger@google.com>
	    Tom Tromey  <tom@tromey.com>

	* gdbsupport/thread-pool.h: New file.
	* gdbsupport/thread-pool.c: New file.
	* Makefile.in (COMMON_SFILES): Add thread-pool.c.
	(HFILES_NO_SRCDIR): Add thread-pool.h.

Change-Id: I597bb642780cb9d578ca92373d2a638efb44fe52
---
M gdb/ChangeLog
M gdb/Makefile.in
A gdb/gdbsupport/thread-pool.c
A gdb/gdbsupport/thread-pool.h
4 files changed, 228 insertions(+), 0 deletions(-)
  

Comments

Simon Marchi (Code Review) Oct. 20, 2019, 9:55 a.m. UTC | #1
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172/1/gdb/gdbsupport/thread-pool.h 
File gdb/gdbsupport/thread-pool.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172/1/gdb/gdbsupport/thread-pool.h@80 
PS1, Line 80:   std::queue<task> m_tasks;
I would still argue that at least the queue should be heap-allocated/leaked as well, so that the background threads do not access freed memory when gdb exits and the main thread destructs the threadpool. Does this not cause an address sanitizer error for you?
  
Simon Marchi (Code Review) Oct. 20, 2019, 3:36 p.m. UTC | #2
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172/1/gdb/gdbsupport/thread-pool.h 
File gdb/gdbsupport/thread-pool.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172/1/gdb/gdbsupport/thread-pool.h@80 
PS1, Line 80:   std::queue<task> m_tasks;
> I would still argue that at least the queue should be heap-allocated/leaked as well, so that the background threads do not access freed memory when gdb exits and the main thread destructs the threadpool. Does this not cause an address sanitizer error for you?

I didn't try asan.

I just didn't understand when you brought this up earlier.
I think I'll go with your original suggestion of heap-allocating
the entire thread pool and then just "leaking" the whole thing.
Sorry about that.
  
Simon Marchi (Code Review) Oct. 21, 2019, 1:29 a.m. UTC | #3
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172/1/gdb/gdbsupport/thread-pool.h 
File gdb/gdbsupport/thread-pool.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172/1/gdb/gdbsupport/thread-pool.h@80 
PS1, Line 80:   std::queue<task> m_tasks;
> > I would still argue that at least the queue should be heap-allocated/leaked as well, so that the b […]
Sorry for explaining it poorly. Thanks for fixing; looks good to me.
  
Simon Marchi (Code Review) Nov. 22, 2019, 9:40 p.m. UTC | #4
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172
......................................................................


Patch Set 3: Code-Review+2

(3 comments)

LGTM.  Some cosmetic comments below.

| --- /dev/null
| +++ gdb/gdbsupport/thread-pool.h
| @@ -1,0 +45,19 @@ public:
| +
| +  ~thread_pool ();
| +  DISABLE_COPY_AND_ASSIGN (thread_pool);
| +
| +  /* Set the thread count of this thread pool.  By default, no threads
| +     are created -- the thread count must be set first.  */
| +  void set_thread_count (size_t num_threads);
| +
| +  /* Return the number of executing threads.  */
| +  size_t count () const

PS3, Line 54:

A little odd that the setter is set_thread_count, and the getter is
just count.  Shouldn't it be either "set_thread_count/thread_count" or
"set_count/count"?

| +  {
| +    return m_count;
| +  }
| +
| +  /* Post a task to the thread pool.  A future is returned, which can
| +     be used to wait for the result.  */
| +  std::future<void> post_task (std::function<void ()> func);
| +
| +private:

 ...

| @@ -1,0 +65,22 @@ private:
| +  thread_pool () = default;
| +
| +  /* The callback for each worker thread.  */
| +  void thread_function ();
| +
| +  /* An optional is used to represent a task.  If the optional is
| +     empty, then this means that the receiving thread should
| +     terminate.  If the optional is non-empty, then it is an actual
| +     task to evaluate.  */
| +  typedef optional<std::packaged_task<void ()>> task;

PS3, Line 74:

IMO this typedef obfuscates more than it helps.  I'm thinking that:

typedef std::packaged_task<void ()> task;

and then using "optional<task>" where "task" is used today would end
up being clearer.  I think it's 2 places only.

Not that much of a big deal, for sure.

| +
| +  /* The current thread count.  */
| +  size_t m_count = 0;

PS3, Line 77:

If you change the getter to thread_count, then I'd also advocate
calling this "m_thread_count".

| +
| +  /* The tasks that have not been processed yet.  */
| +  std::queue<task> m_tasks;
| +
| +  /* A condition variable and mutex that are used for communication
| +     between the main thread and the worker threads.  */
| +  std::condition_variable m_tasks_cv;
| +  std::mutex m_tasks_mutex;
| +};
  
Simon Marchi (Code Review) Nov. 26, 2019, 6:59 p.m. UTC | #5
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/172
......................................................................


Patch Set 4: Code-Review+2
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f04d2bd..ab67332 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@ 
+2019-10-19  Christian Biesinger  <cbiesinger@google.com>
+	    Tom Tromey  <tom@tromey.com>
+
+	* gdbsupport/thread-pool.h: New file.
+	* gdbsupport/thread-pool.c: New file.
+	* Makefile.in (COMMON_SFILES): Add thread-pool.c.
+	(HFILES_NO_SRCDIR): Add thread-pool.h.
+
 2019-10-19  Tom Tromey  <tom@tromey.com>
 
 	* event-top.h (thread_local_segv_handler): Declare.
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 5e01c32..3be798a 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -984,6 +984,7 @@ 
 	gdbsupport/signals.c \
 	gdbsupport/signals-state-save-restore.c \
 	gdbsupport/tdesc.c \
+	gdbsupport/thread-pool.c \
 	gdbsupport/xml-utils.c \
 	complaints.c \
 	completer.c \
@@ -1485,6 +1486,7 @@ 
 	gdbsupport/signals-state-save-restore.h \
 	gdbsupport/symbol.h \
 	gdbsupport/tdesc.h \
+	gdbsupport/thread-pool.h \
 	gdbsupport/version.h \
 	gdbsupport/x86-xstate.h \
 	gdbsupport/xml-utils.h \
diff --git a/gdb/gdbsupport/thread-pool.c b/gdb/gdbsupport/thread-pool.c
new file mode 100644
index 0000000..993e097
--- /dev/null
+++ b/gdb/gdbsupport/thread-pool.c
@@ -0,0 +1,119 @@ 
+/* Thread pool
+
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "common-defs.h"
+#include "gdbsupport/thread-pool.h"
+#include "gdbsupport/alt-stack.h"
+#include "gdbsupport/block-signals.h"
+#include <algorithm>
+
+namespace gdb
+{
+
+thread_pool thread_pool::g_thread_pool;
+
+thread_pool::thread_pool ()
+  : m_tasks_cv (new std::condition_variable)
+{
+}
+
+thread_pool::~thread_pool ()
+{
+  /* Because this is a singleton, we don't need to clean up.  The
+     threads are detached so that they won't prevent process exit.
+     And, cleaning up here would be actively harmful in at least one
+     case -- see the comment by m_tasks_cv.  */
+}
+
+void
+thread_pool::set_thread_count (size_t num_threads)
+{
+  std::lock_guard<std::mutex> guard (m_tasks_mutex);
+
+  /* If the new size is larger, start some new threads.  */
+  if (m_count < num_threads)
+    {
+      /* Ensure that signals used by gdb are blocked in the new
+	 threads.  */
+      block_signals blocker;
+      for (size_t i = m_count; i < num_threads; ++i)
+	{
+	  std::thread thread (&thread_pool::thread_function, this);
+	  thread.detach ();
+	}
+    }
+  /* If the new size is smaller, terminate some existing threads.  */
+  if (num_threads < m_count)
+    {
+      for (size_t i = num_threads; i < m_count; ++i)
+	m_tasks.emplace ();
+      m_tasks_cv->notify_all ();
+    }
+
+  m_count = num_threads;
+}
+
+std::future<void>
+thread_pool::post_task (std::function<void ()> func)
+{
+  std::packaged_task<void ()> t (func);
+  std::future<void> f = t.get_future ();
+
+  if (m_count == 0)
+    {
+      /* Just execute it now.  */
+      t ();
+    }
+  else
+    {
+      std::lock_guard<std::mutex> guard (m_tasks_mutex);
+      m_tasks.emplace (std::move (t));
+      m_tasks_cv->notify_one ();
+    }
+  return f;
+}
+
+void
+thread_pool::thread_function ()
+{
+  /* Ensure that SIGSEGV is delivered to an alternate signal
+     stack.  */
+  gdb::alternate_signal_stack signal_stack;
+
+  while (true)
+    {
+      task t;
+
+      {
+	/* We want to hold the lock while examining the task list, but
+	   not while invoking the task function.  */
+	std::unique_lock<std::mutex> guard (m_tasks_mutex);
+	while (m_tasks.empty ())
+	  m_tasks_cv->wait (guard);
+	t = std::move (m_tasks.front());
+	m_tasks.pop ();
+      }
+
+      if (!t.has_value ())
+	break;
+      (*t) ();
+    }
+}
+
+}
diff --git a/gdb/gdbsupport/thread-pool.h b/gdb/gdbsupport/thread-pool.h
new file mode 100644
index 0000000..d760ad1
--- /dev/null
+++ b/gdb/gdbsupport/thread-pool.h
@@ -0,0 +1,99 @@ 
+/* Thread pool
+
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef GDBSUPPORT_THREAD_POOL_H
+#define GDBSUPPORT_THREAD_POOL_H
+
+#include <queue>
+#include <thread>
+#include <vector>
+#include <functional>
+#include <mutex>
+#include <condition_variable>
+#include <future>
+#include "gdbsupport/gdb_optional.h"
+
+namespace gdb
+{
+
+/* A thread pool.
+
+   There is a single global thread pool, see g_thread_pool.  Tasks can
+   be submitted to the thread pool.  They will be processed in worker
+   threads as time allows.  */
+class thread_pool
+{
+public:
+  /* The sole global thread pool.  */
+  static thread_pool g_thread_pool;
+
+  ~thread_pool ();
+  DISABLE_COPY_AND_ASSIGN (thread_pool);
+
+  /* Set the thread count of this thread pool.  By default, no threads
+     are created -- the thread count must be set first.  */
+  void set_thread_count (size_t num_threads);
+
+  /* Return the number of executing threads.  */
+  size_t count () const
+  {
+    return m_count;
+  }
+
+  /* Post a task to the thread pool.  A future is returned, which can
+     be used to wait for the result.  */
+  std::future<void> post_task (std::function<void ()> func);
+
+private:
+
+  thread_pool ();
+
+  /* The callback for each worker thread.  */
+  void thread_function ();
+
+  /* An optional is used to represent a task.  If the optional is
+     empty, then this means that the receiving thread should
+     terminate.  If the optional is non-empty, then it is an actual
+     task to evaluate.  */
+  typedef optional<std::packaged_task<void ()>> task;
+
+  /* The current thread count.  */
+  size_t m_count = 0;
+
+  /* The tasks that have not been processed yet.  */
+  std::queue<task> m_tasks;
+  /* A condition variable and mutex that are used for communication
+     between the main thread and the worker threads.
+
+     Note that this is a pointer.  The thread pool detach()s its
+     threads, so that the threads will not prevent the process from
+     exiting.  However, it was discovered that if any detached threads
+     were still waiting on a condition variable, then the condition
+     variable's destructor would wait for the threads to exit --
+     defeating the purpose.
+
+     Allocating the condition variable on the heap and simply
+     "leaking" it avoids this problem.  */
+  std::condition_variable *m_tasks_cv;
+  std::mutex m_tasks_mutex;
+};
+
+}
+
+#endif /* GDBSUPPORT_THREAD_POOL_H */