Hi Aditya, On 09/09/26 11:13, Aditya Vidyadhar Kamath wrote: > From: Aditya Vidyadhar Kamath > > 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 > #include > #include > #include > > #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 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 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. */