RFA [threads]: Thread cache

[Daniel Jacobowitz <drow at mvista dot com>]

I've figured out how to fix print-threads.exp (see my ramblings on gdb@
yesterday for a bad description of the problem; better coming soon). 
However, to do it, I discovered that it was actually _required_ that we
cache certain information from libthread_db, instead of merely beneficial.

So I implemented the cache.  This patch is the entire cache mechanism,
except for updating the comment at the top of the file saying we need one.
Before I get to the patch itself, some numbers:

My test for these things is starting GDB, loading Mozilla, running until the
first dialog box, clicking exit, and waiting until the GDB prompt comes
back.  CPU time, not wall clock time (I'm not that reliable!).

Before: from 13.7 to 15.0 seconds.  The test is not the most reliable, as
anywhere from seven to ten threads seem to be created before it asks me what
profile I want [why?]; but always at least seven.  Strace reveals on the
order of 283,500 pread() calls, i.e. target memory reads.

After: 10.6 seconds, and 74400 pread() calls, i.e. less by almost a factor
of four.

[I have other patches in progress to shave possibly as much as another four
seconds off this testcase.]


Now, on to the patch itself.  I replace all calls to td_ta_map_id2thr_p
and most calls to td_thr_get_info_p [Hmm, I don't see any reason not to
convert the others too; I will do that in a separate patch if this one is
approved, and see how much more it takes off the runtime] with calls to
wrapper functions which cache the data in the struct private_thread_info.
The cache is invalidated at every resume(); there's some information that we
could keep if we are guaranteed a 1-to-1 threads implementation with no
migration, like LinuxThreads or NPTL, but I'm being conservative for now.

The comment above thread_get_info_callback is slightly misleading; it's not
used as a callback for the thread_db iterator routine until my next patch. 
For now it's just a helper for thread_from_lwp.

The call to target_pid_to_str is moved below the call to add_thread in
attach_thread(), since the cache requires that a struct thread_info * exist
for the ptid being printed.

Oh, and in thread_db_pid_to_str I replace an error () with putting "Missing"
into the string; there's no point in target_pid_to_str failing, since it's
only used for display.

I also remove some dead code which used to share the cache structure with
lin-lwp.c; it's been commented out for a year.

The rest is mechanical changes to use the new functions.

Here it is.  Comments?  OK?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-01-10  Daniel Jacobowitz  <drow@mvista.com>

	* lin-lwp.c (struct private_thread_info, find_lwp_callback): Remove.
	(resume_callback): Remove dead code.

	* thread-db.c (attach_thread): Prototype.
	(struct private_thread_info): Remove lwpid.  Add thread handle (th),
	thread information (ti), and valid flags (th_valid, ti_valid).
	(attach_thread): Move target_pid_to_str call to after the thread
	is added to GDB's list.  Initialize the cache.
	(thread_get_info_callback, thread_db_map_id2thr)
	(thread_db_get_info): New functions.
	(thread_from_lwp, lwp_from_thread, thread_db_fetch_registers)
	(thread_db_store_registers, thread_db_thread_alive)
	(thread_db_get_thread_local_address): Use them.
	(thread_db_pid_to_str): Likewise.  Return "Missing" instead
	of calling error() for threads in unknown state.

	(clear_lwpid_callback): New function.
	(thread_db_resume): Use it to clear the cache.

--- gdb/thread-db.c	2003-01-10 15:15:11.000000000 -0500
+++ gdb/thread-db.c	2003-01-10 15:09:03.000000000 -0500
@@ -126,6 +126,8 @@
 
 /* Prototypes for local functions.  */
 static void thread_db_find_new_threads (void);
+static void attach_thread (ptid_t ptid, const td_thrhandle_t *th_p,
+			   const td_thrinfo_t *ti_p, int verbose);
 
 
 /* Building process ids.  */
@@ -141,10 +143,17 @@
 #define BUILD_THREAD(tid, pid)	ptid_build (pid, 0, tid)
 
 
+/* Use "struct private_thread_info" to cache thread state.  This is
+   a substantial optimization.  */
+
 struct private_thread_info
 {
-  /* Cached LWP id.  Must come first, see lin-lwp.c.  */
-  lwpid_t lwpid;
+  /* Cached thread state.  */
+  unsigned int th_valid : 1;
+  unsigned int ti_valid : 1;
+
+  td_thrhandle_t th;
+  td_thrinfo_t ti;
 };
 
 
@@ -228,15 +237,100 @@
     }
 }
 
+/* A callback function for td_ta_thr_iter, which we use to map all
+   threads to LWPs.  
+
+   THP is a handle to the current thread; if INFOP is not NULL, the
+   struct thread_info associated with this thread is returned in
+   *INFOP.  */
+
+static int
+thread_get_info_callback (const td_thrhandle_t *thp, void *infop)
+{
+  td_thrinfo_t ti;
+  td_err_e err;
+  struct thread_info *thread_info;
+  ptid_t thread_ptid;
+
+  err = td_thr_get_info_p (thp, &ti);
+  if (err != TD_OK)
+    error ("thread_get_info_callback: cannot get thread info: %s", 
+	   thread_db_err_str (err));
+
+  /* Fill the cache.  */
+  thread_ptid = BUILD_THREAD (ti.ti_tid, GET_PID (inferior_ptid));
+  thread_info = find_thread_pid (thread_ptid);
+
+  if (thread_info == NULL)
+    {
+      /* New thread.  Attach to it now (why wait?).  */
+      attach_thread (thread_ptid, thp, &ti, 1);
+      thread_info = find_thread_pid (thread_ptid);
+      gdb_assert (thread_info != NULL);
+    }
+
+  memcpy (&thread_info->private->th, thp, sizeof (*thp));
+  thread_info->private->th_valid = 1;
+  memcpy (&thread_info->private->ti, &ti, sizeof (ti));
+  thread_info->private->ti_valid = 1;
+
+  if (infop != NULL)
+    *(struct thread_info **) infop = thread_info;
+
+  return 0;
+}
+
+/* Accessor functions for the thread_db information, with caching.  */
 
+static void
+thread_db_map_id2thr (struct thread_info *thread_info, int fatal)
+{
+  td_err_e err;
+
+  if (thread_info->private->th_valid)
+    return;
+
+  err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (thread_info->ptid),
+			    &thread_info->private->th);
+  if (err != TD_OK)
+    {
+      if (fatal)
+	error ("Cannot find thread %ld: %s",
+	       (long) GET_THREAD (thread_info->ptid), thread_db_err_str (err));
+    }
+  else
+    thread_info->private->th_valid = 1;
+}
+
+static td_thrinfo_t *
+thread_db_get_info (struct thread_info *thread_info)
+{
+  td_err_e err;
+
+  if (thread_info->private->ti_valid)
+    return &thread_info->private->ti;
+
+  if (! thread_info->private->th_valid)
+    thread_db_map_id2thr (thread_info, 1);
+
+  err = td_thr_get_info_p (&thread_info->private->th, &thread_info->private->ti);
+  if (err != TD_OK)
+    error ("thread_db_get_info: cannot get thread info: %s", 
+	   thread_db_err_str (err));
+
+  thread_info->private->ti_valid = 1;
+  return &thread_info->private->ti;
+}
+
 /* Convert between user-level thread ids and LWP ids.  */
 
 static ptid_t
 thread_from_lwp (ptid_t ptid)
 {
-  td_thrinfo_t ti;
   td_thrhandle_t th;
   td_err_e err;
+  struct thread_info *thread_info;
+  ptid_t thread_ptid;
 
   if (GET_LWP (ptid) == 0)
     ptid = BUILD_LWP (GET_PID (ptid), GET_PID (ptid));
@@ -248,35 +342,26 @@
     error ("Cannot find user-level thread for LWP %ld: %s",
 	   GET_LWP (ptid), thread_db_err_str (err));
 
-  err = td_thr_get_info_p (&th, &ti);
-  if (err != TD_OK)
-    error ("thread_from_lwp: cannot get thread info: %s", 
-	   thread_db_err_str (err));
+  thread_info = NULL;
+  thread_get_info_callback (&th, &thread_info);
+  gdb_assert (thread_info && thread_info->private->ti_valid);
 
-  return BUILD_THREAD (ti.ti_tid, GET_PID (ptid));
+  return BUILD_THREAD (thread_info->private->ti.ti_tid, GET_PID (ptid));
 }
 
 static ptid_t
 lwp_from_thread (ptid_t ptid)
 {
-  td_thrinfo_t ti;
-  td_thrhandle_t th;
-  td_err_e err;
+  struct thread_info *thread_info;
+  ptid_t thread_ptid;
 
   if (!is_thread (ptid))
     return ptid;
 
-  err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (ptid), &th);
-  if (err != TD_OK)
-    error ("Cannot find thread %ld: %s",
-	   (long) GET_THREAD (ptid), thread_db_err_str (err));
-
-  err = td_thr_get_info_p (&th, &ti);
-  if (err != TD_OK)
-    error ("lwp_from_thread: cannot get thread info: %s", 
-	   thread_db_err_str (err));
+  thread_info = find_thread_pid (ptid);
+  thread_db_get_info (thread_info);
 
-  return BUILD_LWP (ti.ti_lid, GET_PID (ptid));
+  return BUILD_LWP (thread_info->private->ti.ti_lid, GET_PID (ptid));
 }
 
 
@@ -581,13 +666,13 @@
 
   check_thread_signals ();
 
-  if (verbose)
-    printf_unfiltered ("[New %s]\n", target_pid_to_str (ptid));
-
   /* Add the thread to GDB's thread list.  */
   tp = add_thread (ptid);
   tp->private = xmalloc (sizeof (struct private_thread_info));
-  tp->private->lwpid = ti_p->ti_lid;
+  memset (tp->private, 0, sizeof (struct private_thread_info));
+
+  if (verbose)
+    printf_unfiltered ("[New %s]\n", target_pid_to_str (ptid));
 
   if (ti_p->ti_state == TD_THR_UNKNOWN || ti_p->ti_state == TD_THR_ZOMBIE)
     return;			/* A zombie thread -- do not attach.  */
@@ -644,6 +729,19 @@
   target_beneath->to_detach (args, from_tty);
 }
 
+static int
+clear_lwpid_callback (struct thread_info *thread, void *dummy)
+{
+  /* If we know that our thread implementation is 1-to-1, we could save
+     a certain amount of information; it's not clear how much, so we
+     are always conservative.  */
+
+  thread->private->th_valid = 0;
+  thread->private->ti_valid = 0;
+
+  return 0;
+}
+
 static void
 thread_db_resume (ptid_t ptid, int step, enum target_signal signo)
 {
@@ -654,6 +752,9 @@
   else if (is_thread (ptid))
     ptid = lwp_from_thread (ptid);
 
+  /* Clear cached data which may not be valid after the resume.  */
+  iterate_over_threads (clear_lwpid_callback, NULL);
+
   target_beneath->to_resume (ptid, step, signo);
 
   do_cleanups (old_chain);
@@ -787,7 +888,7 @@
 static void
 thread_db_fetch_registers (int regno)
 {
-  td_thrhandle_t th;
+  struct thread_info *thread_info;
   prgregset_t gregset;
   gdb_prfpregset_t fpregset;
   td_err_e err;
@@ -799,17 +900,15 @@
       return;
     }
 
-  err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (inferior_ptid), &th);
-  if (err != TD_OK)
-    error ("Cannot find thread %ld: %s",
-	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
+  thread_info = find_thread_pid (inferior_ptid);
+  thread_db_map_id2thr (thread_info, 1);
 
-  err = td_thr_getgregs_p (&th, gregset);
+  err = td_thr_getgregs_p (&thread_info->private->th, gregset);
   if (err != TD_OK)
     error ("Cannot fetch general-purpose registers for thread %ld: %s",
 	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
 
-  err = td_thr_getfpregs_p (&th, &fpregset);
+  err = td_thr_getfpregs_p (&thread_info->private->th, &fpregset);
   if (err != TD_OK)
     error ("Cannot get floating-point registers for thread %ld: %s",
 	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
@@ -824,10 +923,10 @@
 static void
 thread_db_store_registers (int regno)
 {
-  td_thrhandle_t th;
   prgregset_t gregset;
   gdb_prfpregset_t fpregset;
   td_err_e err;
+  struct thread_info *thread_info;
 
   if (!is_thread (inferior_ptid))
     {
@@ -836,10 +935,8 @@
       return;
     }
 
-  err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (inferior_ptid), &th);
-  if (err != TD_OK)
-    error ("Cannot find thread %ld: %s",
-	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
+  thread_info = find_thread_pid (inferior_ptid);
+  thread_db_map_id2thr (thread_info, 1);
 
   if (regno != -1)
     {
@@ -853,11 +950,11 @@
   fill_gregset ((gdb_gregset_t *) gregset, -1);
   fill_fpregset (&fpregset, -1);
 
-  err = td_thr_setgregs_p (&th, gregset);
+  err = td_thr_setgregs_p (&thread_info->private->th, gregset);
   if (err != TD_OK)
     error ("Cannot store general-purpose registers for thread %ld: %s",
 	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
-  err = td_thr_setfpregs_p (&th, &fpregset);
+  err = td_thr_setfpregs_p (&thread_info->private->th, &fpregset);
   if (err != TD_OK)
     error ("Cannot store floating-point registers  for thread %ld: %s",
 	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
@@ -915,24 +1012,31 @@
 thread_db_thread_alive (ptid_t ptid)
 {
   td_thrhandle_t th;
-  td_thrinfo_t ti;
   td_err_e err;
 
   if (is_thread (ptid))
     {
-      err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (ptid), &th);
-      if (err != TD_OK)
-	return 0;
+      struct thread_info *thread_info;
+      thread_info = find_thread_pid (ptid);
 
-      err = td_thr_validate_p (&th);
-      if (err != TD_OK)
+      thread_db_map_id2thr (thread_info, 0);
+      if (! thread_info->private->th_valid)
 	return 0;
 
-      err = td_thr_get_info_p (&th, &ti);
+      err = td_thr_validate_p (&thread_info->private->th);
       if (err != TD_OK)
 	return 0;
 
-      if (ti.ti_state == TD_THR_UNKNOWN || ti.ti_state == TD_THR_ZOMBIE)
+      if (! thread_info->private->ti_valid)
+	{
+	  err = td_thr_get_info_p (&thread_info->private->th, &thread_info->private->ti);
+	  if (err != TD_OK)
+	    return 0;
+	  thread_info->private->ti_valid = 1;
+	}
+
+      if (thread_info->private->ti.ti_state == TD_THR_UNKNOWN
+	  || thread_info->private->ti.ti_state == TD_THR_ZOMBIE)
 	return 0;		/* A zombie thread.  */
 
       return 1;
@@ -986,29 +1090,29 @@
   if (is_thread (ptid))
     {
       static char buf[64];
-      td_thrhandle_t th;
-      td_thrinfo_t ti;
+      td_thrinfo_t *ti_p;
       td_err_e err;
+      struct thread_info *thread_info;
 
-      err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (ptid), &th);
-      if (err != TD_OK)
-	error ("Cannot find thread %ld: %s",
-	       (long) GET_THREAD (ptid), thread_db_err_str (err));
+      thread_info = find_thread_pid (ptid);
+      thread_db_map_id2thr (thread_info, 0);
+      if (! thread_info->private->th_valid)
+	{
+	  snprintf (buf, sizeof (buf), "Thread %ld (Missing)", GET_THREAD (ptid));
+	  return buf;
+	}
 
-      err = td_thr_get_info_p (&th, &ti);
-      if (err != TD_OK)
-	error ("thread_db_pid_to_str: cannot get thread info for %ld: %s",
-	       (long) GET_THREAD (ptid), thread_db_err_str (err));
+      ti_p = thread_db_get_info (thread_info);
 
-      if (ti.ti_state == TD_THR_ACTIVE && ti.ti_lid != 0)
+      if (ti_p->ti_state == TD_THR_ACTIVE && ti_p->ti_lid != 0)
 	{
 	  snprintf (buf, sizeof (buf), "Thread %ld (LWP %d)",
-		    (long) ti.ti_tid, ti.ti_lid);
+		    (long) ti_p->ti_tid, ti_p->ti_lid);
 	}
       else
 	{
 	  snprintf (buf, sizeof (buf), "Thread %ld (%s)",
-		    (long) ti.ti_tid, thread_db_state_str (ti.ti_state));
+		    (long) ti_p->ti_tid, thread_db_state_str (ti_p->ti_state));
 	}
 
       return buf;
@@ -1031,9 +1135,9 @@
     {
       int objfile_is_library = (objfile->flags & OBJF_SHARED);
       td_err_e err;
-      td_thrhandle_t th;
       void *address;
       CORE_ADDR lm;
+      struct thread_info *thread_info;
 
       /* glibc doesn't provide the needed interface.  */
       if (! td_thr_tls_get_addr_p)
@@ -1054,13 +1158,12 @@
 	}
 
       /* Get info about the thread.  */
-      err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (ptid), &th);
-      if (err != TD_OK)
-	error ("Cannot find thread %ld: %s",
-	       (long) GET_THREAD (ptid), thread_db_err_str (err));
-      
+      thread_info = find_thread_pid (ptid);
+      thread_db_map_id2thr (thread_info, 1);
+
       /* Finally, get the address of the variable.  */
-      err = td_thr_tls_get_addr_p (&th, (void *) lm, offset, &address);
+      err = td_thr_tls_get_addr_p (&thread_info->private->th, (void *) lm,
+				   offset, &address);
 
 #ifdef THREAD_DB_HAS_TD_NOTALLOC
       /* The memory hasn't been allocated, yet.  */
Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.41
diff -u -p -r1.41 lin-lwp.c
--- lin-lwp.c	9 Jan 2003 19:14:46 -0000	1.41
+++ lin-lwp.c	10 Jan 2003 20:25:13 -0000
@@ -536,25 +536,6 @@ lin_lwp_detach (char *args, int from_tty
 }
 
 
-struct private_thread_info
-{
-  int lwpid;
-};
-
-/* Return non-zero if TP corresponds to the LWP specified by DATA
-   (which is assumed to be a pointer to a `struct lwp_info'.  */
-
-static int
-find_lwp_callback (struct thread_info *tp, void *data)
-{
-  struct lwp_info *lp = data;
-
-  if (tp->private->lwpid == GET_LWP (lp->ptid))
-    return 1;
-
-  return 0;
-}
-
 /* Resume LP.  */
 
 static int
@@ -563,27 +544,6 @@ resume_callback (struct lwp_info *lp, vo
   if (lp->stopped && lp->status == 0)
     {
       struct thread_info *tp;
-
-#if 0
-      /* FIXME: kettenis/2000-08-26: This should really be handled
-         properly by core GDB.  */
-
-      tp = find_thread_pid (lp->ptid);
-      if (tp == NULL)
-	tp = iterate_over_threads (find_lwp_callback, lp);
-      gdb_assert (tp);
-
-      /* If we were previously stepping the thread, and now continue
-         the thread we must invalidate the stepping range.  However,
-         if there is a step_resume breakpoint for this thread, we must
-         preserve the stepping range to make it possible to continue
-         stepping once we hit it.  */
-      if (tp->step_range_end && tp->step_resume_breakpoint == NULL)
-	{
-	  gdb_assert (lp->step);
-	  tp->step_range_start = tp->step_range_end = 0;
-	}
-#endif
 
       child_resume (pid_to_ptid (GET_LWP (lp->ptid)), 0, TARGET_SIGNAL_0);
       if (debug_lin_lwp)