Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>
To: gdb-patches@sourceware.org
Subject: [PATCH 11/11] gdb: optimize all_matching_threads_iterator
Date: Tue, 22 Jun 2021 12:57:04 -0400	[thread overview]
Message-ID: <20210622165704.2404007-12-simon.marchi@polymtl.ca> (raw)
In-Reply-To: <20210622165704.2404007-1-simon.marchi@polymtl.ca>

all_matching_threads_iterator is used extensively in some pretty fast
paths, often under the all_non_exited_threads function.

If a filter target and thread-specific ptid are given, it iterates on
all threads of all inferiors of that target, to ultimately yield exactly
on thread.  And this happens quite often, which means we unnecessarily
spend time iterating on threads to find the one we are looking for.  The
same thing happens if an inferior-specific ptid is given, although there
the iterator yields all the threads of that inferior.

In those cases, the callers of all_non_exited_threads could have
different behaviors depending on the kind of ptid, to avoid this
inefficiency, but that would be very tedious.  Using
all_non_exited_threads has the advantage that one simple implementation
can work seamlessly on multiple threads or on one specific thread, just
by playing with the ptid.

Instead, optimize all_matching_threads_iterator directly to detect these
different cases and limiting what we iterate on to just what we need.

 - if filter_ptid is minus_one_ptid, do as we do now: filter inferiors
   based on filter_target, iterate on all of the matching inferiors'
   threads
 - if filter_ptid is a pid-only ptid (then a filter_target must
   necessarily be given), look up that inferior and iterate on all its
   threads
 - otherwise, filter_ptid is a thread-specific ptid, so look up that
   specific thread and "iterate" only on it

For the last case, what was an iteration on all threads of the filter
target now becomes a call to find_thread_ptid, which is quite efficient
now thanks to inferior::ptid_thread_map.

gdb/ChangeLog:

	* thread-iter.h (class all_matching_threads_iterator)
	<all_matching_threads_iterator>: Use default.
	<enum class mode>: New.
	<m_inf, m_thr>: Initialize.
	<m_filter_ptid>: Remove.
	* thread-iter.c (all_matching_threads_iterator::m_inf_matches):
	Don't filter on m_filter_ptid.
	(all_matching_threads_iterator::all_matching_threads_iterator):
	Choose path based on filter_ptid (all threads, all threads of
	inferior, single thread).
	(all_matching_threads_iterator::advance): Likewise.

Change-Id: Ic6a19845f5f760fa1b8eac8145793c0ff431bbc9
---
 gdb/thread-iter.c | 138 ++++++++++++++++++++++++++++++----------------
 gdb/thread-iter.h |  29 ++++++----
 2 files changed, 107 insertions(+), 60 deletions(-)

diff --git a/gdb/thread-iter.c b/gdb/thread-iter.c
index 31b7a36eaada..e56ccd857b0a 100644
--- a/gdb/thread-iter.c
+++ b/gdb/thread-iter.c
@@ -75,39 +75,56 @@ all_threads_iterator::advance ()
 bool
 all_matching_threads_iterator::m_inf_matches ()
 {
-  return ((m_filter_target == nullptr
-	   || m_filter_target == m_inf->process_target ())
-	  && (m_filter_ptid == minus_one_ptid
-	      || m_filter_ptid.pid () == m_inf->pid));
+  return (m_filter_target == nullptr
+	  || m_filter_target == m_inf->process_target ());
 }
 
 /* See thread-iter.h.  */
 
 all_matching_threads_iterator::all_matching_threads_iterator
   (process_stratum_target *filter_target, ptid_t filter_ptid)
-    : m_filter_target (filter_target),
-      m_filter_ptid (filter_ptid)
+  : m_filter_target (filter_target)
 {
-  gdb_assert ((filter_target == nullptr && filter_ptid == minus_one_ptid)
-	      || filter_target->stratum () == process_stratum);
-
-  for (inferior &inf : inferior_list)
+  if (filter_ptid == minus_one_ptid)
     {
-      m_inf = &inf;
-      if (m_inf_matches ())
-	for (auto thr_iter = m_inf->thread_list.begin ();
-	     thr_iter != m_inf->thread_list.end ();
-	     ++thr_iter)
-	  {
-	    if (thr_iter->ptid.matches (m_filter_ptid))
-	      {
-		m_thr = &*thr_iter;
-		return;
-	      }
-	  }
+      /* Iterate on all threads of all inferiors, possibly filtering on
+         FILTER_TARGET.  */
+      m_mode = mode::ALL_THREADS;
+
+      /* Seek the first thread of the first matching inferior.  */
+      for (inferior &inf : inferior_list)
+	{
+	  m_inf = &inf;
+
+	  if (!m_inf_matches ()
+	      || inf.thread_list.empty ())
+	    continue;
+
+	  m_thr = &inf.thread_list.front ();
+	  return;
+	}
     }
+  else
+    {
+      gdb_assert (filter_target != nullptr);
 
-  m_thr = nullptr;
+      if (filter_ptid.is_pid ())
+	{
+	  /* Iterate on all threads of the given inferior.  */
+	  m_mode = mode::ALL_THREADS_OF_INFERIOR;
+
+	  m_inf = find_inferior_pid (filter_target, filter_ptid.pid ());
+	  if (m_inf != nullptr)
+	    m_thr = &m_inf->thread_list.front ();
+	}
+      else
+	{
+	  /* Iterate on a single thread.  */
+	  m_mode = mode::SINGLE_THREAD;
+
+	  m_thr = find_thread_ptid (filter_target, filter_ptid);
+	}
+    }
 }
 
 /* See thread-iter.h.  */
@@ -115,32 +132,57 @@ all_matching_threads_iterator::all_matching_threads_iterator
 void
 all_matching_threads_iterator::advance ()
 {
-  intrusive_list<inferior>::iterator inf_iter (m_inf);
-  intrusive_list<thread_info>::iterator thr_iter (m_thr);
+  switch (m_mode)
+    {
+    case mode::ALL_THREADS:
+      {
+	intrusive_list<inferior>::iterator inf_iter (m_inf);
+	intrusive_list<thread_info>::iterator thr_iter
+	  = m_inf->thread_list.iterator_to (*m_thr);
+
+	/* The loop below is written in the natural way as-if we'd always
+	   start at the beginning of the inferior list.  This fast forwards
+	   the algorithm to the actual current position.  */
+	goto start;
+
+	for (; inf_iter != inferior_list.end (); ++inf_iter)
+	  {
+	    m_inf = &*inf_iter;
 
-  /* The loop below is written in the natural way as-if we'd always
-     start at the beginning of the inferior list.  This fast forwards
-     the algorithm to the actual current position.  */
-  goto start;
+	    if (!m_inf_matches ())
+	      continue;
 
-  for (; inf_iter != inferior_list.end (); ++inf_iter)
-    {
-      m_inf = &*inf_iter;
-      if (m_inf_matches ())
-	{
-	  thr_iter = m_inf->thread_list.begin ();
-	  while (thr_iter != m_inf->thread_list.end ())
-	    {
-	      if (thr_iter->ptid.matches (m_filter_ptid))
-		{
-		  m_thr = &*thr_iter;
-		  return;
-		}
-	    start:
-	      ++thr_iter;
-	    }
-	}
-    }
+	    thr_iter = m_inf->thread_list.begin ();
+	    while (thr_iter != m_inf->thread_list.end ())
+	      {
+		m_thr = &*thr_iter;
+		return;
 
-  m_thr = nullptr;
+	      start:
+		++thr_iter;
+	      }
+	  }
+      }
+      m_thr = nullptr;
+      break;
+
+    case mode::ALL_THREADS_OF_INFERIOR:
+      {
+	intrusive_list<thread_info>::iterator thr_iter
+	  = m_inf->thread_list.iterator_to (*m_thr);
+	++thr_iter;
+	if (thr_iter != m_inf->thread_list.end ())
+	  m_thr = &*thr_iter;
+	else
+	  m_thr = nullptr;
+	break;
+      }
+
+    case mode::SINGLE_THREAD:
+      m_thr = nullptr;
+      break;
+
+    default:
+      gdb_assert_not_reached ("invalid mode value");
+    }
 }
diff --git a/gdb/thread-iter.h b/gdb/thread-iter.h
index 2e43034550e8..6700f5593b91 100644
--- a/gdb/thread-iter.h
+++ b/gdb/thread-iter.h
@@ -99,12 +99,7 @@ class all_matching_threads_iterator
 				 ptid_t filter_ptid);
 
   /* Create a one-past-end iterator.  */
-  all_matching_threads_iterator ()
-    : m_inf (nullptr),
-      m_thr (nullptr),
-      m_filter_target (nullptr),
-      m_filter_ptid (minus_one_ptid)
-  {}
+  all_matching_threads_iterator () = default;
 
   thread_info *operator* () const { return m_thr; }
 
@@ -124,20 +119,30 @@ class all_matching_threads_iterator
   /* Advance to next thread, skipping filtered threads.  */
   void advance ();
 
-  /* True if M_INF matches the process identified by
-     M_FILTER_PTID.  */
+  /* True if M_INF has the process target M_FILTER_TARGET.  */
   bool m_inf_matches ();
 
 private:
+  enum class mode
+  {
+    /* All threads, possibly filtered down to a single target.  */
+    ALL_THREADS,
+
+    /* All threads of the given inferior.  */
+    ALL_THREADS_OF_INFERIOR,
+
+    /* A specific thread.  */
+    SINGLE_THREAD,
+  } m_mode;
+
   /* The current inferior.  */
-  inferior *m_inf;
+  inferior *m_inf = nullptr;
 
   /* The current thread.  */
-  thread_info *m_thr;
+  thread_info *m_thr = nullptr;
 
-  /* The filter.  */
+  /* The target we filter on (may be nullptr).  */
   process_stratum_target *m_filter_target;
-  ptid_t m_filter_ptid;
 };
 
 /* Filter for filtered_iterator.  Filters out exited threads.  */
-- 
2.32.0


  parent reply	other threads:[~2021-06-22 17:03 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-22 16:56 [PATCH 00/11] Various thread lists optimizations Simon Marchi via Gdb-patches
2021-06-22 16:56 ` [PATCH 01/11] gdb: introduce iterator_range, remove next_adapter Simon Marchi via Gdb-patches
2021-07-05 15:41   ` Pedro Alves
2021-07-06 19:16     ` Simon Marchi via Gdb-patches
2021-06-22 16:56 ` [PATCH 02/11] gdb: introduce intrusive_list, make thread_info use it Simon Marchi via Gdb-patches
2021-06-22 23:13   ` Lancelot SIX via Gdb-patches
2021-06-23  0:48     ` Simon Marchi via Gdb-patches
2021-07-05 15:44   ` Pedro Alves
2021-07-06 19:38     ` Simon Marchi via Gdb-patches
2021-07-06 20:45       ` Simon Marchi via Gdb-patches
2021-07-06 21:04         ` Pedro Alves
2021-07-06 21:38           ` Simon Marchi via Gdb-patches
2021-07-06 21:02       ` Pedro Alves
2021-07-06 21:45         ` Simon Marchi via Gdb-patches
2021-07-07 11:46           ` Pedro Alves
2021-07-07 13:52             ` Simon Marchi via Gdb-patches
2021-06-22 16:56 ` [PATCH 03/11] gdb: make inferior_list use intrusive_list Simon Marchi via Gdb-patches
2021-07-05 15:44   ` Pedro Alves
2021-07-14  6:34     ` Tom de Vries
2021-07-14 16:11       ` Simon Marchi via Gdb-patches
2021-07-14 20:15         ` [PATCH] gdb: make all_inferiors_safe actually work Simon Marchi via Gdb-patches
2021-07-15 10:15           ` Tom de Vries
2021-07-17 12:54             ` Simon Marchi via Gdb-patches
2021-06-22 16:56 ` [PATCH 04/11] gdb: use intrusive list for step-over chain Simon Marchi via Gdb-patches
2021-07-05 15:45   ` Pedro Alves
2021-07-06 20:59     ` Simon Marchi via Gdb-patches
2021-06-22 16:56 ` [PATCH 05/11] gdb: add setter / getter for thread_info resumed state Simon Marchi via Gdb-patches
2021-07-05 15:45   ` Pedro Alves
2021-06-22 16:56 ` [PATCH 06/11] gdb: make thread_info::suspend private, add getters / setters Simon Marchi via Gdb-patches
2021-07-05 15:45   ` Pedro Alves
2021-06-22 16:57 ` [PATCH 07/11] gdb: maintain per-process-target list of resumed threads with pending wait status Simon Marchi via Gdb-patches
2021-07-05 15:51   ` Pedro Alves
2021-07-06 21:25     ` Simon Marchi via Gdb-patches
2021-07-07 12:01       ` Pedro Alves
2021-07-12 22:28         ` Simon Marchi via Gdb-patches
2021-07-12 22:34           ` Simon Marchi via Gdb-patches
2021-07-13 12:21           ` Pedro Alves
2021-06-22 16:57 ` [PATCH 08/11] gdb: optimize check for resumed threads with pending wait status in maybe_set_commit_resumed_all_targets Simon Marchi via Gdb-patches
2021-07-05 15:51   ` Pedro Alves
2021-06-22 16:57 ` [PATCH 09/11] gdb: optimize selection of resumed thread with pending event Simon Marchi via Gdb-patches
2021-07-05 15:51   ` Pedro Alves
2021-06-22 16:57 ` [PATCH 10/11] gdb: maintain ptid -> thread map, optimize find_thread_ptid Simon Marchi via Gdb-patches
2021-07-05 15:52   ` Pedro Alves
2021-07-06 21:31     ` Simon Marchi via Gdb-patches
2021-07-07 12:13       ` Pedro Alves
2021-06-22 16:57 ` Simon Marchi via Gdb-patches [this message]
2021-07-05 15:52   ` [PATCH 11/11] gdb: optimize all_matching_threads_iterator Pedro Alves
2021-07-14  9:40     ` Tom de Vries
2021-07-13  0:47 ` [PATCH 00/11] Various thread lists optimizations Simon Marchi via Gdb-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210622165704.2404007-12-simon.marchi@polymtl.ca \
    --to=gdb-patches@sourceware.org \
    --cc=simon.marchi@polymtl.ca \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox