Re: [PATCH v1][RFC] Speed up next/step while debugging multithreaded programs on AIX.

[Abhay Kandpal <abhay@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 15289 bytes --]

Hi Aditya,

On 09/09/26 11:13, Aditya Vidyadhar Kamath wrote:
> From: Aditya Vidyadhar Kamath<aditya.kamath1@ibm.com>
>
> During go compiler debugging on AIX my collegue and I noticed that
> the next command was pretty slow compared to Linux - around 7 seconds
> per step when program has atleast 3 or more threads running. A simple
> walk through a function took around a minute. In compiler codes this is
> slow.
>
> TO measure this properly I wrote a benchmark pasted below. The program spawns
> 20 worker threads , then stops inside a function with 10 simple
> that GDB steps over one by one. The GDB batch script breaks at that
> function and issues 10 next commands and then quits. Timing the whole
> run gives a fair picture of how much overhead each `next` carries.
> Both the .c file and batch file are pasted below.
>
> Before this patch, on AIX 7.3 ppc64 with 20 background threads:
>
> real    1m10.05s
> user    0m17.59s
> sys     0m20.43s
>
> That is roughly 7 seconds per `next` step.
>
> After this patch:
>
> real    0m20.37s
> user    0m5.94s
> sys     0m6.53s
>
> About 2 seconds per step.  3.5x faster for the same workload.
>
> Now why was it slow?
>
> Every time the inferior stops -- even for a single-step trap from next
> - the AIX thread layer called pd_update(), which did two very expensive
> things unconditionally:
>
> 1. pthdb_session_update ()
>
> This function walks through all of the pthread library's internal
> data structures inside the inferior process, reading them out via
> pdc_read_data() callbacks. With 20 threads that is over 300 memory
> reads from the inferior on every single stop, even when absolutely
> nothing about the threads has changed.
>
> 2. sync_threadlists()
>
> This follows the session update with a full rebuild of GDB's thread
> list: iterate every thread via pthdb_pthread(), call
> pthdb_pthread_tid() for each one, allocate two sorted arrays, diff
> them against GDB's internal list, and reconcile the differences.
> Again, all of this on every stop, even when the thread population
> has not changed at all since the last stop.
>
> On top of that, get_signaled_thread() was calling getthrds() asking for
> one kernel thread descriptor at a time, which costs one syscall per
> thread in the process.
>
> None of this work is wrong but it is needed when threads actually come
> and go.  But during a next or step sequence through a function, the
> thread list does not change. Paying for a full rescan on every stop is
> something not needed I think.
>
> So what changes this patch does is:
>
> 1. Batch getthrds() calls get_signaled_thread ()
>
> The original code passed 1 as the count argument to getthrds(), so
> it did one syscall per kernel thread to find the one that stopped.
> Changed to fetch 64 descriptors per call and scan the batch in a
> loop. This alone cuts the getthrds() cost from O(N) syscalls to
> O(N/64).
>
> 2. Track the thread count - aix_thread_variables, pd_activate
>
> Added a last_thread_count field to aix_thread_variables.  It records
> how many threads libpthdebug reported the last time a full sync was
> done. Initialised to -1 in the constructor so the very
> first stop always does a full sync. Also reset to -1 in pd_activate()
> so that whenever a new debug session is opened the first stop is
> always fully synced regardless of the stop kind.
>
> 3. Skip the sync on clean step stops
>
> pd_update() now takes a bool step_stop parameter.  When that is true
> and last_thread_count is not -1, it skips pthdb_session_update() and
> sync_threadlists() entirely.
>
> When a sync is needed (not a step stop, or first stop after
> activation), a new helper count_pthdb_threads() does a lightweight
> pass through pthdb_pthread() just to count threads. It only calls
> sync_threadlists() if that count differs from last_thread_count.
> This avoids the expensive pthdb_pthread_tid() calls and array
> allocations on stops where the thread count has not changed.
>
> 4. Detect step stops in wait() and pass the flag down
>
> wait() already has the stop kind and signal number before it calls
> pd_update(). A TARGET_WAITKIND_STOPPED with GDB_SIGNAL_TRAP is what
> the kernel reports for a completed single-step. wait() now computes
> step_stop from those two fields and passes it to pd_update().
>
> int current_count = count_pthdb_threads (data);
>            if (current_count != data->last_thread_count)
>
> This makes sure we never miss any genuinely coming new threads and are
> in sync.
>
> So these are the  things I thought and did. Let me know what you think.
>
> ==================
> Benchmark program (bench_next.c):
>
>      /* Spawns NUM_THREADS worker threads that spin, then steps through
>         10 assignments in do_steps() so GDB can time each next.  */
>      #include <stdio.h>
>      #include <stdlib.h>
>      #include <pthread.h>
>      #include <unistd.h>
>
>      #ifndef NUM_THREADS
>      #define NUM_THREADS 20
>      #endif
>
>      static volatile int keep_running = 1;
>
>      static void *
>      worker (void *arg)
>      {
>        while (keep_running)
>          sched_yield ();
>        return NULL;
>      }
>
>      void
>      do_steps (void)
>      {
>        volatile int a = 1;
>        volatile int b = 2;
>        volatile int c = a + b;
>        volatile int d = c * 2;
>        volatile int e = d - a;
>        volatile int f = e + c;
>        volatile int g = f / 2;
>        volatile int h = g + 1;
>        volatile int i2 = h * h;
>        volatile int j = i2 - b;
>        (void)j;
>      }
>
>      int
>      main (void)
>      {
>        pthread_t threads[NUM_THREADS];
>        int i;
>
>        for (i = 0; i < NUM_THREADS; i++)
>          pthread_create (&threads[i], NULL, worker, NULL);
>
>        do_steps ();
>
>        keep_running = 0;
>        for (i = 0; i < NUM_THREADS; i++)
>          pthread_join (threads[i], NULL);
>
>        return 0;
>      }
>
> ====================
> Build done with
> gcc -O0 -g -gdwarf -maix64 -DNUM_THREADS=20 -o bench_next_bin bench_next.c -lpthread
> =====================
> GDB batch script (bench_next.gdb):
>
>      set pagination off
>      set confirm off
>
>      break do_steps
>      run
>
>      next
>      next
>      next
>      next
>      next
>      next
>      next
>      next
>      next
>      next
>
>      quit
> ========================
> Then run,
> time gdb -batch -x bench_next.gdb ./bench_next_bin
> ---
>   gdb/aix-thread.c | 117 +++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 94 insertions(+), 23 deletions(-)
>
> diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
> index 5ac71d22237..5c43e1130e4 100644
> --- a/gdb/aix-thread.c
> +++ b/gdb/aix-thread.c
> @@ -173,6 +173,11 @@ static pthdb_callbacks_t pd_callbacks = {
>   /* Aix variable structure.  */
>   struct aix_thread_variables
>   {
> +  aix_thread_variables ()
> +    : pd_able (0), pd_active (0), pd_session (0), pd_brk_addr (0),
> +      arch64 (0), last_thread_count (-1)
> +  {}
> +
>     /* Whether the current application is debuggable by pthdb.  */
>     int pd_able;
>   
> @@ -192,6 +197,11 @@ struct aix_thread_variables
>   
>     /* Describes the number of thread exit events reported.  */
>     std::unordered_set<pthdb_pthread_t> exited_threads;
> +
> +  /* Last known libpthdebug thread count.  Used to skip sync_threadlists()
> +     when the thread population has not changed since the previous stop,
> +     which is the common case during stepping.  -1 means unknown/force sync.  */
> +  int last_thread_count;
>   };
>   
>   /* Key to our per-inferior data.  */
> @@ -733,6 +743,11 @@ state2str (pthdb_state_t state)
>       }
>   }
>   
> +/* Number of thread descriptors to fetch per getthrds() call.  The original
> +   code fetched one at a time, which costs one syscall per thread.  Fetching
> +   in batches reduces that to one syscall per GETTHRDS_BATCH threads.  */
> +#define GETTHRDS_BATCH 64
> +
>   /* Search through the list of all kernel threads for the thread
>      that has stopped on a SIGTRAP signal, and return its TID.
>      Return 0 if none found.  */
> @@ -740,20 +755,18 @@ state2str (pthdb_state_t state)
>   static pthdb_tid_t
>   get_signaled_thread (int pid)
>   {
> -  struct thrdsinfo64 thrinf;
> +  struct thrdsinfo64 thrinf[GETTHRDS_BATCH];
>     tid_t ktid = 0;
> +  int count, i;
>   
> -  while (1)
> +  while ((count = getthrds (pid, thrinf, sizeof (thrinf[0]),
> +			    &ktid, GETTHRDS_BATCH)) > 0)
>       {
> -      if (getthrds (pid, &thrinf,
> -		    sizeof (thrinf), &ktid, 1) != 1)
> -	break;
> -
>         /* We also need to keep in mind Trap and interrupt or any
>   	 signal that needs to be handled in pd_update ().  */
> -
> -      if (thrinf.ti_cursig)
> -	return thrinf.ti_tid;
> +      for (i = 0; i< count; i++) + if (thrinf[i].ti_cursig) + return thrinf[i].ti_tid; } 
> /* Didn't find any thread stopped on a SIGTRAP signal. */ @@ -861,13 
> +874,39 @@ sync_threadlists (pid_t pid) } } +/* Count the number of 
> live pthreads visible to libpthdebug. + Used before sync_threadlists() 
> to check whether the thread population + has changed since the last 
> stop. */ + +static int +count_pthdb_threads (struct 
> aix_thread_variables *data) +{ + pthdb_pthread_t pdtid; + int cmd, n = 
> 0; + + for (cmd = PTHDB_LIST_FIRST;; cmd = PTHDB_LIST_NEXT) + { + int 
> status = pthdb_pthread (data->pd_session, &pdtid, cmd);
> +      if (status != PTHDB_SUCCESS || pdtid == PTHDB_INVALID_PTHREAD)
> +	break;
> +      n++;
> +    }
> +  return n;
> +}
> +
>   /* Synchronize libpthdebug's state with the inferior and with GDB,
>      generate a composite process/thread <pid> for the current thread,
> -   Return the ptid of the event thread if one can be found, else
> -   return a pid-only ptid with PID.  */
> +   return the ptid of the event thread if one can be found, else
> +   return a pid-only ptid with PID.
> +
> +   STEP_STOP should be set when the stop is a single-step SIGTRAP.
> +   In that case, if the thread list was already synced at a previous
> +   stop, both pthdb_session_update() and sync_threadlists() are skipped.
> +   Those two calls dominate the cost of every stop in a multithreaded
> +   program, so skipping them during stepping gives a large speedup.  */
>   
>   static ptid_t
> -pd_update (pid_t pid)
> +pd_update (pid_t pid, bool step_stop = false)
>   {
>     int status;
>     ptid_t ptid;
> @@ -880,15 +919,36 @@ pd_update (pid_t pid)
>     if (!data->pd_active)
>       return ptid_t (pid);
>   
> -  status = pthdb_session_update (data->pd_session);
> -  if (status != PTHDB_SUCCESS)
> -    return ptid_t (pid);
> -
> -  /* Attempt to sync_threadlists () only when debugging object files
> -     and not core files since list of threads never change for core
> -     files.  */
> -  if (target_has_execution ())
> -    sync_threadlists (pid);
> +  /* Skip the session update and thread list sync when stepping.
> +     pthdb_session_update() reads through all pthread data structures in
> +     the inferior on every call, and sync_threadlists() follows it with
> +     O(N) kernel calls per thread.  During a next/step sequence the thread
> +     list does not change, so both calls can be omitted as long as we have
> +     done at least one full sync since activation (last_thread_count >= 0).
> +     Any other stop kind (breakpoints, real signals) always does a full
> +     sync because threads may have been created or destroyed.  */
> +  bool need_sync = !step_stop || (data->last_thread_count < 0);
> +
> +  if (need_sync)
> +    {
> +      status = pthdb_session_update (data->pd_session);
> +      if (status != PTHDB_SUCCESS)
> +	return ptid_t (pid);
> +
> +      /* Attempt to sync_threadlists() only when debugging object files
> +	 and not core files since the list of threads never changes for
> +	 core files.  Even for live inferiors, skip the sync when the
> +	 thread count matches the previous stop.  */
> +      if (target_has_execution ())
> +	{
> +	  int current_count = count_pthdb_threads (data);
> +	  if (current_count != data->last_thread_count)
> +	    {
> +	      sync_threadlists (pid);
> +	      data->last_thread_count = current_count;
> +	    }
> +	}
> +    }
>   
>     /* Define "current thread" as one that just received a trap signal.  */
>   
> @@ -921,7 +981,12 @@ pd_activate (pid_t pid)
>   			       PTHDB_FLAG_REGS, &pd_callbacks,
>   			       &data->pd_session);
>     if (status == PTHDB_SUCCESS)
> -    data->pd_active = 1;
> +    {
> +      data->pd_active = 1;
> +      /* Reset last_thread_count so that the very first stop after
> +	 activation always does a full sync_threadlists() pass.  */
> +      data->last_thread_count = -1;
> +    }
>   }
>   
>   /* AIX implementation of update_thread_list.  */
> @@ -1125,7 +1190,13 @@ aix_thread_target::wait (ptid_t ptid, struct target_waitstatus *status,
>   	pd_activate (ptid.pid ());
>       }
>   
> -  return pd_update (ptid.pid ());
> +  /* Tell pd_update() whether this is a single-step SIGTRAP so it can
> +     skip the expensive pthdb_session_update() and sync_threadlists()
> +     calls.  Breakpoints, real signals and other stop kinds always get
> +     the full sync.  */
> +  bool step_stop = (status->kind () == TARGET_WAITKIND_STOPPED
> +		    && status->sig () == GDB_SIGNAL_TRAP);
> +  return pd_update (ptid.pid (), step_stop);

Doesn't a breakpoint stop also arrive as TARGET_WAITKIND_STOPPED with GDB_SIGNAL_TRAP?
get_signaled_thread() already looks for the thread stopped on SIGTRAP to find the
current thread, and that has to work at breakpoint stops too, so I would expect
a breakpoint hit to look the same as a single-step here.

If so, breakpoint stops also get step_stop = true and skip the sync, which is the
opposite of what the commit message says:

"Any other stop kind (breakpoints, real signals) always does a full sync
because threads may have been created or destroyed."

The last_thread_count < 0 check only covers the first stop after pd_activate().
After that first sync, last_thread_count is >= 0 for the rest of the session
and need_sync becomes just !step_stop.  So from the second stop onwards
a breakpoint hit skips the sync, and any threads created since the previous
stop are missed.

The benchmark won't show this: all 20 threads are created before do_steps()
and there is only one breakpoint, so the thread list never changes after the
first stop.

resume() is already told whether GDB asked for a single step, and it already
has the aix_thread_variables pointer in hand.  Could you save the flag there
and use it here instead of inferring it from the signal?

/* aix_thread_target::resume (), after the existing
data = get_thread_data_helper_for_ptid (ptid); */
data->last_resume_step = step;

/* in wait () */
bool step_stop = (data->last_resume_step
&& status->kind () == TARGET_WAITKIND_STOPPED
&& status->sig () == GDB_SIGNAL_TRAP);

That would also handle "next" over a function call, where GDB puts a temporary
breakpoint at the return address and continues rather than single-stepping
through the callee.  resume() is called with step = 0 in that case, so the
stop would correctly get a full sync even though the user typed "next".

Regards,
Abhay

>   }
>   
>   /* Supply AIX altivec registers, both 64 and 32 bit.  */

[-- Attachment #2: Type: text/html, Size: 16017 bytes --]