[RFA] Improve performance with lots of shared libraries

[Gary Benson <gbenson@redhat.com>]

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

Hi all,

Back in July I submitted a patch[1] that improved performance when
debugging inferiors that load lots of shared libraries.  It hinged on
avoiding calling find_pc_partial_function and skip_inline_frames in
handle_inferior_event when the event being handled was a stop at the
solib event breakpoint.

That patch was kind of hacky in that it relied on second-guessing some
of the logic that followed.  I started working on splitting the patch,
and committed a patch to make find_pc_partial_function lazy[2] in July.

Attached is a patch to make gdb avoid the call to skip_inline_frames.
With a small benchmark I put together (a little program that loads 1000
shared libraries) gdb ran the application in 36s with the patch
against 46s without.

This patch is superficially similar to the original patch, but much
simpler and different in scope.  The initial patch worked on the
premise that if the stop was specifially a stop for the solib event
breakpoint then the calls could be skipped (because I'd walked through
the code ahead and seen that their results were not used).  The result
was a fairly complex conditional that would have needed to be kept
updated if other parts of handle_inferior_event changed.

This patch works on the simpler premise that the solib event
breakpoint is by definition the address of a function, ie not inline,
so regardless of why the stop occurred the call to skip_inline_frames
can be omitted because there are no inline frames to skip.  The
heuristic is therefore simpler and very much less fragile.

I have tried a two of other approaches to this, but neither worked.
My preferred method was to make skip_inline_frames lazy, but this
caused problems because the skipping, when it occurred, invalidated
the frame cache.   handle_inferior_event has several places where
local "frame" variables are recreated because some call or another
has invalidated the frame cache and left the pointer dangling, and
making skip_inline_frames added many more.  It was looking like a
maintainence nightmaire long before I even got it to work.

The other approach I tried was to use bpstat_stop_status to
identify the solib event breakpoint, rather than writing my own
code to do it.  This looked really clean, but failed a huge number
of tests.  Apparently bpstat_stop_status has side effects!

How is this patch do you think?  Is it ok to commit?

Cheers,
Gary

[1] http://www.cygwin.com/ml/gdb-patches/2011-07/msg00026.html
[2] http://www.cygwin.com/ml/gdb-patches/2011-07/msg00460.html

-- 
http://gbenson.net/

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2438 bytes --]

2011-09-22  Gary Benson  <gbenson@redhat.com>

	* infrun.c (stopped_at_solib_event_breakpoint): New function.
	(stopped_at_solib_event_breakpoint_helper): Likewise.
	(handle_inferior_event): Avoid calling skip_inline_frames
	when at the solib event breakpoint.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 225034c..2e49470 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3102,6 +3102,39 @@ fill_in_stop_func (struct gdbarch *gdbarch,
     }
 }
 
+/* Helper for stopped_at_solib_event_breakpoint.  */
+
+static int
+stopped_at_solib_event_breakpoint_helper (struct breakpoint *b, void *arg)
+{
+  struct execution_control_state *ecs
+    = (struct execution_control_state *) arg;
+
+  if (b->type == bp_shlib_event)
+    {
+      CORE_ADDR prev_pc = ecs->event_thread->prev_pc;
+      struct bp_location *loc;
+
+      for (loc = b->loc; loc; loc = loc->next)
+	{
+	  if (loc->pspace == current_program_space
+	      && (loc->address == stop_pc || loc->address == prev_pc))
+	    return 1;
+	}
+    }
+
+  return 0;
+}
+
+/* Nonzero if we are stopped at the solib event breakpoint.  */
+
+static int
+stopped_at_solib_event_breakpoint (struct execution_control_state *ecs)
+{
+  return iterate_over_breakpoints (stopped_at_solib_event_breakpoint_helper,
+				   ecs) != NULL;
+}
+
 /* Given an execution control state that has been freshly filled in
    by an event from the inferior, figure out what it means and take
    appropriate action.  */
@@ -4010,9 +4043,14 @@ handle_inferior_event (struct execution_control_state *ecs)
   stopped_by_random_signal = 0;
 
   /* Hide inlined functions starting here, unless we just performed stepi or
-     nexti.  After stepi and nexti, always show the innermost frame (not any
-     inline function call sites).  */
-  if (ecs->event_thread->control.step_range_end != 1)
+     nexti, or we are at the solib event breakpoint.  After stepi and nexti,
+     always show the innermost frame (not any inline function call sites).
+     This call is expensive, and we avoid it if we are at the solib event
+     breakpoint which is defined as the address of a function (i.e., not
+     inline).  This improves performance with inferiors that load a lot of
+     shared libraries.  */
+  if (ecs->event_thread->control.step_range_end != 1
+      && !stopped_at_solib_event_breakpoint (ecs))
     skip_inline_frames (ecs->ptid);
 
   if (ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP