[RFA] Improve performance with lots of shared libraries

[RFA] Improve performance with lots of shared libraries

[Gary Benson]

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

Hi all,

While working on a new linker-debugger interface I took some time out
to do a bit of profiling to see exactly where gdb is spending its time
with inferiors that load a lot of shared libraries, and it turned out
that the top 30% of the profile was update_section_map and the things
it calls.

update_section_map is called in exactly one place, by find_pc_section,
which calls update_section_map if the list of loaded object files has
changed.

There are two calls in handle_inferior_event that (indirectly) call
find_pc_section: find_pc_partial_function, and skip_inline_frames.
The first of these to be called will end up calling update_section_map
every time the solib event breakpoint is hit, because the list of
loaded objects has been changed by the last time the breakpoint was
hit.

I walked through handle_inferior_event and it turns out that when
stop_on_solib_events is unset both the call to
find_pc_partial_function and the call to skip_inline_frames can be
omitted, the first because its result is never used, and the second
because the solib event breakpoint is defined to be the address of
a function--ie not inlined.

This patch alters handle_inferior_event to check whether a stop is due
to the solib event breakpoint, and omit the two calls if it is.  I'm
not 100% convinced there aren't odd corner cases I've missed (the
surrounding code is pretty dense!) but it passes the tests, and 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 47s without, a 23% improvement.

I'd really appreciate feedback from people who know this part of gdb
well, as well as feedback from those users who are using gdb on
many-solibs applications as to whether this patch helps.

Cheers,
Gary

-- 
http://gbenson.net/

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

2011-07-01  Gary Benson  <gbenson@redhat.com>

	* infrun.c (solib_event_breakpoint_helper_arg): New structure.
	(at_solib_event_breakpoint_helper): New function.
	(at_solib_event_breakpoint): Likewise.
	(handle_inferior_event): Avoid calling find_pc_partial_function
	and skip_inline_frames when stopping at the solib event breakpoint.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index a656cbf..6e7a062 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3080,6 +3080,56 @@ handle_syscall_event (struct execution_control_state *ecs)
   return 1;
 }
 
+/* Argument for at_solib_event_breakpoint_helper.  */
+
+struct solib_event_breakpoint_helper_arg
+{
+  CORE_ADDR prev_pc;
+  int shlib_bp_count;
+  int other_bp_count;
+};
+
+/* Helper for at_solib_event_breakpoint.  */
+
+static int
+at_solib_event_breakpoint_helper (struct breakpoint *b, void *argp)
+{
+  struct solib_event_breakpoint_helper_arg *arg
+    = (struct solib_event_breakpoint_helper_arg *) argp; 
+  struct bp_location *loc;
+
+  for (loc = b->loc; loc; loc = loc->next)
+    {
+      if (loc->pspace == current_program_space
+	  && (loc->address == stop_pc || loc->address == arg->prev_pc))
+	{
+	  if (b->type == bp_shlib_event)
+	    arg->shlib_bp_count++;
+	  else
+	    {
+	      arg->other_bp_count++;
+	      return 1; /* quick exit */
+	    }
+	}
+    }
+
+  return 0; /* carry on looking */
+}
+
+/* Nonzero if the location stopoed at is the shlib event breakpoint.  */
+
+static int
+at_solib_event_breakpoint (struct execution_control_state *ecs)
+{
+  struct solib_event_breakpoint_helper_arg arg;
+  arg.prev_pc = ecs->event_thread->prev_pc;
+  arg.shlib_bp_count = arg.other_bp_count = 0;
+
+  iterate_over_breakpoints (at_solib_event_breakpoint_helper, &arg);
+
+  return arg.shlib_bp_count && !arg.other_bp_count;
+}
+
 /* 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.  */
@@ -3928,12 +3978,6 @@ handle_inferior_event (struct execution_control_state *ecs)
   ecs->stop_func_start = 0;
   ecs->stop_func_end = 0;
   ecs->stop_func_name = 0;
-  /* Don't care about return value; stop_func_start and stop_func_name
-     will both be 0 if it doesn't work.  */
-  find_pc_partial_function (stop_pc, &ecs->stop_func_name,
-			    &ecs->stop_func_start, &ecs->stop_func_end);
-  ecs->stop_func_start
-    += gdbarch_deprecated_function_start_offset (gdbarch);
   ecs->event_thread->stepping_over_breakpoint = 0;
   bpstat_clear (&ecs->event_thread->control.stop_bpstat);
   ecs->event_thread->control.stop_step = 0;
@@ -3941,11 +3985,30 @@ handle_inferior_event (struct execution_control_state *ecs)
   ecs->random_signal = 0;
   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)
-    skip_inline_frames (ecs->ptid);
+  /* If we have stopped at the solib event breakpoint and
+     stop_on_solib_events is not set then we can avoid calling
+     anything that calls find_pc_section.  This saves a lot
+     of time when the inferior loads a lot of shared libraries,
+     because otherwise the section map gets regenerated every
+     time we stop.  */
+  if (stop_on_solib_events
+      || ecs->event_thread->suspend.stop_signal != TARGET_SIGNAL_TRAP
+      || stop_after_trap
+      || !at_solib_event_breakpoint (ecs))
+    {
+      /* Don't care about return value; stop_func_start and stop_func_name
+	 will both be 0 if it doesn't work.  */
+      find_pc_partial_function (stop_pc, &ecs->stop_func_name,
+				&ecs->stop_func_start, &ecs->stop_func_end);
+      ecs->stop_func_start
+	+= gdbarch_deprecated_function_start_offset (gdbarch);
+
+      /* 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)
+	skip_inline_frames (ecs->ptid);
+    }
 
   if (ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP
       && ecs->event_thread->control.trap_expected

Re: [RFA] Improve performance with lots of shared libraries

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> While working on a new linker-debugger interface I took some time out
Gary> to do a bit of profiling to see exactly where gdb is spending its time
Gary> with inferiors that load a lot of shared libraries, and it turned out
Gary> that the top 30% of the profile was update_section_map and the things
Gary> it calls.

Nice.

I think this will solve a problem I had:

    http://sourceware.org/ml/gdb-patches/2011-03/msg00606.html

I haven't committed that yet, but I will, sooner or later.

Gary> I'd really appreciate feedback from people who know this part of gdb
Gary> well, as well as feedback from those users who are using gdb on
Gary> many-solibs applications as to whether this patch helps.

I think Pedro is the person to review it.  The idea seems sound to me,
but as you note, this code is very dense.

I have a couple nits.

Gary> +/* Nonzero if the location stopoed at is the shlib event breakpoint.  */

Typo, "stopped".

Gary> +static int
Gary> +at_solib_event_breakpoint (struct execution_control_state *ecs)
Gary> +{
Gary> +  struct solib_event_breakpoint_helper_arg arg;
Gary> +  arg.prev_pc = ecs->event_thread->prev_pc;

Blank line between declarations and code.

Tom

Re: [RFA] Improve performance with lots of shared libraries

[Gary Benson]

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> 
> Gary> While working on a new linker-debugger interface I took some
> Gary> time out to do a bit of profiling to see exactly where gdb is
> Gary> spending its time with inferiors that load a lot of shared
> Gary> libraries, and it turned out that the top 30% of the profile
> Gary> was update_section_map and the things it calls.
> 
> Nice.
> 
> I think this will solve a problem I had:
> 
>     http://sourceware.org/ml/gdb-patches/2011-03/msg00606.html

Oh, I think it will.  Nice!

> I have a couple nits.
> 
> Gary> +/* Nonzero if the location stopoed at is the shlib event breakpoint.  */
> 
> Typo, "stopped".
> 
> Gary> +static int
> Gary> +at_solib_event_breakpoint (struct execution_control_state *ecs)
> Gary> +{
> Gary> +  struct solib_event_breakpoint_helper_arg arg;
> Gary> +  arg.prev_pc = ecs->event_thread->prev_pc;
> 
> Blank line between declarations and code.

Cool, fixed.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [RFA] Improve performance with lots of shared libraries

[Joel Brobecker]

> 2011-07-01  Gary Benson  <gbenson@redhat.com>
> 
> 	* infrun.c (solib_event_breakpoint_helper_arg): New structure.
> 	(at_solib_event_breakpoint_helper): New function.
> 	(at_solib_event_breakpoint): Likewise.
> 	(handle_inferior_event): Avoid calling find_pc_partial_function
> 	and skip_inline_frames when stopping at the solib event breakpoint.

I just have a few general comments, that are minor, and yet important
(IMO).

> +/* Argument for at_solib_event_breakpoint_helper.  */
> +
> +struct solib_event_breakpoint_helper_arg
> +{
> +  CORE_ADDR prev_pc;
> +  int shlib_bp_count;
> +  int other_bp_count;

I think the meaning of each field should be documented. A good
example you could look at, for instance, is the declaration of
type struct frame_id in frame.h.

> +/* Helper for at_solib_event_breakpoint.  */

I think it's important to also say WHAT the function does.
'Helper' doesn't help in that respect (no pun intended).

> +static int
> +at_solib_event_breakpoint (struct execution_control_state *ecs)
> +{
> +  struct solib_event_breakpoint_helper_arg arg;
> +  arg.prev_pc = ecs->event_thread->prev_pc;
> +  arg.shlib_bp_count = arg.other_bp_count = 0;

Empty line between variable declarations and code...
(sorry, one of the innumerable coding style rules in the GDB project).

-- 
Joel

Re: [RFA] Improve performance with lots of shared libraries

[Gary Benson]

Joel Brobecker wrote:
> Gary Benson wrote:
> > +/* Argument for at_solib_event_breakpoint_helper.  */
> > +
> > +struct solib_event_breakpoint_helper_arg
> > +{
> > +  CORE_ADDR prev_pc;
> > +  int shlib_bp_count;
> > +  int other_bp_count;
> 
> I think the meaning of each field should be documented. A good
> example you could look at, for instance, is the declaration of
> type struct frame_id in frame.h.
> 
> > +/* Helper for at_solib_event_breakpoint.  */
> 
> I think it's important to also say WHAT the function does.
> 'Helper' doesn't help in that respect (no pun intended).
> 
> > +static int
> > +at_solib_event_breakpoint (struct execution_control_state *ecs)
> > +{
> > +  struct solib_event_breakpoint_helper_arg arg;
> > +  arg.prev_pc = ecs->event_thread->prev_pc;
> > +  arg.shlib_bp_count = arg.other_bp_count = 0;
> 
> Empty line between variable declarations and code...
> (sorry, one of the innumerable coding style rules in the GDB project).

Thanks Joel, I have updated my tree with the extra documentation and
the extra line.

Cheers,
Gary

-- 
http://gbenson.net/

Re: [RFA] Improve performance with lots of shared libraries

[Pedro Alves]

On Friday 01 July 2011 17:51:09, Gary Benson wrote:
> Hi all,
> 
> While working on a new linker-debugger interface I took some time out
> to do a bit of profiling to see exactly where gdb is spending its time
> with inferiors that load a lot of shared libraries, and it turned out
> that the top 30% of the profile was update_section_map and the things
> it calls.
> 
> update_section_map is called in exactly one place, by find_pc_section,
> which calls update_section_map if the list of loaded object files has
> changed.
> 
> There are two calls in handle_inferior_event that (indirectly) call
> find_pc_section: find_pc_partial_function, and skip_inline_frames.
> The first of these to be called will end up calling update_section_map
> every time the solib event breakpoint is hit, because the list of
> loaded objects has been changed by the last time the breakpoint was
> hit.
> 
> I walked through handle_inferior_event and it turns out that when
> stop_on_solib_events is unset both the call to
> find_pc_partial_function and the call to skip_inline_frames can be
> omitted, the first because its result is never used, and the second
> because the solib event breakpoint is defined to be the address of
> a function--ie not inlined.

I'd rather a split in two different chunks/concepts, as per
your description of the problem:

1 - find_pc_partial_function is expensive, and as such we should
    only call it when necessary.  Can we somehow only do this:

> /* Don't care about return value; stop_func_start and stop_func_name
>     will both be 0 if it doesn't work.  */
>  find_pc_partial_function (stop_pc, &ecs->stop_func_name,
>			    &ecs->stop_func_start, &ecs->stop_func_end);
>  ecs->stop_func_start
>    += gdbarch_deprecated_function_start_offset (gdbarch);

   bit when necessary, rather than listing some special
   cases when it is _not_ necessary?  That is, make that bit
   a bit more lazy.  E.g, it looks like stops for
   BPSTAT_WHAT_STOP* never need that info.  (beware of places
   that pass the ecs down as argument to some function that
   ends up using those fields).

2 - A way to identify that we're stopped at a place defined to
    be the address of a function, i.e., not inlined.  This is sort
    of what you already have.  Can we move the skip_inline_frames
    call until after bpstat_stop_status is called, so we can
    look if the bpstat contains a bp_shlib_event instead?

> 
> This patch alters handle_inferior_event to check whether a stop is due
> to the solib event breakpoint, and omit the two calls if it is.  I'm
> not 100% convinced there aren't odd corner cases I've missed (the
> surrounding code is pretty dense!) but it passes the tests, and 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 47s without, a 23% improvement.
> 
> I'd really appreciate feedback from people who know this part of gdb
> well, as well as feedback from those users who are using gdb on
> many-solibs applications as to whether this patch helps.
> 
> Cheers,
> Gary
> 
> 

-- 
Pedro Alves

Re: [RFA] Improve performance with lots of shared libraries

[Pedro Alves]

On Friday 01 July 2011 18:45:01, Pedro Alves wrote:
> I'd rather a split in two different chunks/concepts, as per
> your description of the problem:
> 
> 1 - find_pc_partial_function is expensive, and as such we should
>     only call it when necessary.  Can we somehow only do this:
> 
> > /* Don't care about return value; stop_func_start and stop_func_name
> >     will both be 0 if it doesn't work.  */
> >  find_pc_partial_function (stop_pc, &ecs->stop_func_name,
> >                           &ecs->stop_func_start, &ecs->stop_func_end);
> >  ecs->stop_func_start
> >    += gdbarch_deprecated_function_start_offset (gdbarch);
> 
>    bit when necessary, rather than listing some special
>    cases when it is not necessary?  That is, make that bit
>    a bit more lazy.  E.g, it looks like stops for
>    BPSTAT_WHAT_STOP* never need that info.  (beware of places
>    that pass the ecs down as argument to some function that
>    ends up using those fields).

Reading back, I may have not been clear --- I'm not asking
to come up with conditions to whitelist these call under,
but rather to move that bit to a new function, like:

static void
stop_func_info (struct execution_control_state *ecs)
{
  /* Don't care about return value; stop_func_start and stop_func_name
     will both be 0 if it doesn't work.  */
     find_pc_partial_function (stop_pc, &ecs->stop_func_name,
                               &ecs->stop_func_start, &ecs->stop_func_end);
     ecs->stop_func_start
      += gdbarch_deprecated_function_start_offset (gdbarch);
}

and then call that function a bit further down, e.g.,
in the BPSTAT_WHAT_STEP_RESUME case; where it reads:

/* When stepping backward, stop at beginning of line range
   (unless it's the function entry point, in which case
	keep going back to the call point).  */

where it reads "Check for subroutine calls.", etc.

-- 
Pedro Alves

[RFA] Improve performance with lots of shared libraries

[Gary Benson]

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

Hi all,

When inferiors load a lot of shared libraries gdb spends a lot of
time processing shared library breakpoint events.  These have a lot
of associated bookkeeping that is not easily avoided.

This patch modifies gdb to disable the shared library breakpoint
when it is not required, specifically:

 * when stop-on-solib-events is off,
 * when there are no pending breakpoints,
 * and when libthread_db is loaded

I have a simple test which dlopens 1000 shared libraries.  On my
machine it runs instantly outside of gdb, but takes roughly a minute
when run within gdb.  With this patch, it runs instantly in gdb too.

The idea for this was not mine, it was Jan Kratochvil's, and he
very definitely deserves the credit for it.  I wrote the code though,
so the bugs are all my fault :)

There are two things I'm not sure about with it as it stands.  One is
to do with program spaces: I noticed that breakpoints have a program
space, but breakpoint locations also have a program space.  Is the way
I have used these correct?

The other issue I have is the way it detects whether libthread_db is
loaded.  This should work fine on platforms that use a libthread_db,
but some platforms will have this optimization disabled.  Nothing will
get worse, but some platforms will not get better when they could.
Have I gone about this the wrong way?

I would definitely appreciate feedback from those of you who are using
gdb on applications with many shared libraries, to know if you are
getting any improvement.

Cheers,
Gary

-- 
http://gbenson.net/

[-- Attachment #2: solib-break-disabler-1.patch --]
[-- Type: text/plain, Size: 8490 bytes --]

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

	* Makefile.in (SFILES): Add solib-bp-disable.c.
	(HFILES_NO_SRCDIR): Add solib-bp-disable.h.
	(COMMON_OBS): Add solib-bp-disable.o.
	* infrun.c: Include solib-bp-disable.h.
	(set_stop_on_solib_events): New function.
	(_initialize_infrun): Add set_stop_on_solib_events.
	* solib-bp-disable.c: New file.
	* solib-bp-disable.h: Likewise.

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 826d339..9e947d4 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -720,7 +720,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
 	regcache.c reggroups.c remote.c remote-fileio.c reverse.c \
 	sentinel-frame.c \
 	serial.c ser-base.c ser-unix.c \
-	solib.c solib-target.c source.c \
+	solib.c solib-bp-disable.c solib-target.c source.c \
 	stabsread.c stack.c std-regs.c symfile.c symfile-mem.c symmisc.c \
 	symtab.c \
 	target.c target-descriptions.c target-memory.c \
@@ -821,7 +821,7 @@ solib-darwin.h solib-ia64-hpux.h solib-spu.h windows-nat.h xcoffread.h \
 gnulib/extra/arg-nonnull.h gnulib/extra/c++defs.h gnulib/extra/warn-on-use.h \
 gnulib/stddef.in.h inline-frame.h \
 common/common-utils.h common/xml-utils.h common/buffer.h common/ptid.h \
-common/linux-osdata.h
+common/linux-osdata.h solib-bp-disable.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
@@ -902,7 +902,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	reggroups.o regset.o \
 	trad-frame.o \
 	tramp-frame.o \
-	solib.o solib-target.o \
+	solib.o solib-bp-disable.o solib-target.o \
 	prologue-value.o memory-map.o memrange.o \
 	xml-support.o xml-syscall.o xml-utils.o \
 	target-descriptions.o target-memory.o xml-tdesc.o xml-builtin.o \
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 2b4f6db..fd57f49 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -56,6 +56,7 @@
 #include "tracepoint.h"
 #include "continuations.h"
 #include "interps.h"
+#include "solib-bp-disable.h"
 
 /* Prototypes for local functions */
 
@@ -320,6 +321,13 @@ static struct symbol *step_start_function;
 /* Nonzero if we want to give control to the user when we're notified
    of shared library events by the dynamic linker.  */
 int stop_on_solib_events;
+
+static void
+set_stop_on_solib_events (char *args, int from_tty, struct cmd_list_element *c)
+{
+  update_solib_breakpoints ();
+} 
+
 static void
 show_stop_on_solib_events (struct ui_file *file, int from_tty,
 			   struct cmd_list_element *c, const char *value)
@@ -7096,7 +7104,7 @@ Show stopping for shared library events."), _("\
 If nonzero, gdb will give control to the user when the dynamic linker\n\
 notifies gdb of shared library events.  The most common event of interest\n\
 to the user would be loading/unloading of a new library."),
-			    NULL,
+			    set_stop_on_solib_events,
 			    show_stop_on_solib_events,
 			    &setlist, &showlist);
 
diff --git a/gdb/solib-bp-disable.c b/gdb/solib-bp-disable.c
new file mode 100644
index 0000000..c243a73
--- /dev/null
+++ b/gdb/solib-bp-disable.c
@@ -0,0 +1,153 @@
+/* Disable the shared library event breakpoint when unnecessary.
+
+   Copyright (C) 2011 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+
+#include "breakpoint.h"
+#include "inferior.h"
+#include "observer.h"
+#include "solib-bp-disable.h"
+#include "solist.h"
+#include "target.h"
+
+/* Nonzero if multi-threaded inferior support is present.  */
+
+static int
+multi_thread_support_availabie (void)
+{
+  struct target_ops *t;
+
+  for (t = current_target.beneath; t != NULL; t = t->beneath)
+    if (t->to_find_new_threads != NULL)
+      return 1;
+
+  return 0;
+}
+
+/* Nonzero if this breakpoint is pending in the current program space.  */
+
+static int
+breakpoint_is_pending (struct breakpoint *b, void *arg)
+{
+  struct bp_location *loc;
+
+  if (b->pspace != current_program_space)
+    return 0;
+
+  if (b->loc == NULL)
+    return 1;
+
+  for (loc = b->loc; loc; loc = loc->next)
+    if (loc->shlib_disabled && loc->pspace == current_program_space)
+      return 1;
+
+  return 0;
+}
+
+/* Nonzero if pending breakpoints exist in the current program space.  */
+
+static int
+pending_breakpoints_exist (void)
+{
+  return iterate_over_breakpoints (breakpoint_is_pending, NULL) != NULL;
+}
+
+/* Enable or disable a single solib event breakpoint as appropriate.  */
+
+static int
+update_solib_breakpoint (struct breakpoint *b, void *arg)
+{
+  int enable = *(int *) arg;
+  struct bp_location *loc;
+
+  if (b->pspace != current_program_space)
+    return 0;
+
+  if (b->type != bp_shlib_event)
+    return 0;
+  
+  for (loc = b->loc; loc; loc = loc->next)
+    if (loc->pspace == current_program_space)
+      {
+	if (enable && b->enable_state == bp_disabled)
+	  b->enable_state = bp_enabled;
+	else if (!enable && b->enable_state == bp_enabled)
+	  b->enable_state = bp_disabled;
+      }
+
+  return 0;
+}
+
+/* Enable or disable solib event breakpoints as appropriate.  */
+
+void
+update_solib_breakpoints ()
+{
+  int enable = stop_on_solib_events
+    || !multi_thread_support_availabie ()
+    || pending_breakpoints_exist ();
+
+  iterate_over_breakpoints (update_solib_breakpoint, &enable);
+}
+
+/* Wrappers to match the observer function pointer prototypes.  */
+
+static void
+breakpoint_event (struct breakpoint *b)
+{
+  update_solib_breakpoints ();
+}
+
+static void
+inferior_event (struct target_ops *ops, int from_tty)
+{
+  update_solib_breakpoints ();
+}
+
+static void
+solib_event (struct so_list *s)
+{
+  update_solib_breakpoints ();
+}
+
+/* Provide a prototype to silence -Wmissing-prototypes.  */
+extern initialize_file_ftype _initialize_solib_bp_disable;
+
+void
+_initialize_solib_bp_disable (void)
+{
+  /* Observe breakpoint operations so we can enable the shared
+     library event breakpoint if there are breakpoints pending
+     on shared library loads.  */
+  observer_attach_breakpoint_created (breakpoint_event);
+  observer_attach_breakpoint_modified (breakpoint_event);
+  observer_attach_breakpoint_deleted (breakpoint_event);
+
+  /* We also need to watch for inferior creation, because the
+     creation of the shared library event breakpoint does not
+     cause a breakpoint_created notification so inferior_created
+     is the next best thing.  */
+  observer_attach_inferior_created (inferior_event);
+
+  /* Observe shared libraries being loaded and unloaded so we
+     can disable the shared library event breakpoint once a
+     thread debugging library has been loaded.  */
+  observer_attach_solib_loaded (solib_event);
+  observer_attach_solib_unloaded (solib_event);
+}
diff --git a/gdb/solib-bp-disable.h b/gdb/solib-bp-disable.h
new file mode 100644
index 0000000..da883ab
--- /dev/null
+++ b/gdb/solib-bp-disable.h
@@ -0,0 +1,27 @@
+/* Disable the shared library event breakpoint when unnecessary.
+
+   Copyright (C) 2011 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef SOLIB_BP_DISABLE_H
+#define SOLIB_BP_DISABLE_H
+
+/* Enable or disable solib event breakpoints as appropriate.  */
+
+extern void update_solib_breakpoints (void);
+
+#endif /* SOLIB_BP_DISABLE_H */

Re: [RFA] Improve performance with lots of shared libraries

[Daniel Jacobowitz]

Thanks for working on this.  I've recently run into a lot of problems
with tests that link to an excessive number of shared libraries, so
I'm glad to see someone working on it.  I have a concern with one
part, though:

On Fri, Sep 9, 2011 at 8:31 AM, Gary Benson <gbenson@redhat.com> wrote:
>  * when there are no pending breakpoints,

If you "break foo", it might put that breakpoint in more than one
shared library.  If you load a new library with an implementation of
foo, we should stop on that one too.  How can we make that work
without processing the library events?

-- 
Thanks,
Daniel

Re: [RFA] Improve performance with lots of shared libraries

[Jan Kratochvil]

On Fri, 09 Sep 2011 16:25:30 +0200, Daniel Jacobowitz wrote:
> On Fri, Sep 9, 2011 at 8:31 AM, Gary Benson <gbenson@redhat.com> wrote:
> >  * when there are no pending breakpoints,
> 
> If you "break foo", it might put that breakpoint in more than one
> shared library.  If you load a new library with an implementation of
> foo, we should stop on that one too.  How can we make that work
> without processing the library events?

This feature was planned being aware of this problem.

It does not work currently, GDB just puts the breakpoint on a random first
place found.

The Tom's multi-location breakpoints patch (archer-tromey-ambiguous-linespec)
is being coded with this behavior so that by default gdb syntax it is defined
the first resolution is final.
	http://sourceware.org/ml/gdb-patches/2011-07/msg00740.html

I would guess this patch should be checked-in only after Tom's.  Or make it
opt-in configurable.  Or just say the already broken behavior is now broken in
a different way.


Thanks,
Jan

Re: [RFA] Improve performance with lots of shared libraries

[Pedro Alves]

On Friday 09 September 2011 15:35:21, Jan Kratochvil wrote:
> On Fri, 09 Sep 2011 16:25:30 +0200, Daniel Jacobowitz wrote:
> > On Fri, Sep 9, 2011 at 8:31 AM, Gary Benson <gbenson@redhat.com> wrote:
> > >  * when there are no pending breakpoints,
> > 
> > If you "break foo", it might put that breakpoint in more than one
> > shared library.  If you load a new library with an implementation of
> > foo, we should stop on that one too.  How can we make that work
> > without processing the library events?
> 
> This feature was planned being aware of this problem.
> 
> It does not work currently, GDB just puts the breakpoint on a random first
> place found.
> 
> The Tom's multi-location breakpoints patch (archer-tromey-ambiguous-linespec)
> is being coded with this behavior so that by default gdb syntax it is defined
> the first resolution is final.
> 	http://sourceware.org/ml/gdb-patches/2011-07/msg00740.html

Not according to <http://sourceware.org/ml/gdb-patches/2011-08/msg00032.html>

> 
> I would guess this patch should be checked-in only after Tom's.  Or make it
> opt-in configurable.  Or just say the already broken behavior is now broken in
> a different way.
> 
> 
> Thanks,
> Jan
> 

-- 
Pedro Alves

Re: [RFA] Improve performance with lots of shared libraries

[Jan Kratochvil]

On Fri, 09 Sep 2011 16:53:22 +0200, Pedro Alves wrote:
> Not according to <http://sourceware.org/ml/gdb-patches/2011-08/msg00032.html>

Oops, I read it but ...

On Fri, 09 Sep 2011 17:09:31 +0200, Pedro Alves wrote:
> ... and I don't think that's entirely true.  Yes, GDB will stop
> trying to resolve the spec to a symbol, but at every event gdb will
> still re-set the breakpoint locations.  If the new library has
> the _same_ file:lineno compiled in (e.g., we put a breakpoint
> on a c++ template in a header, or an inline function that is
> also used by new library), then we should be getting new locations in
> the new shared library, today.

I would not state it so optimistically, in my experience the watchpoints to
inlined functions (PR 10738) and templates (file:line will break just at the
one instance kind, not all of them) do not work much.  But I agree the design
of extensions should not block fixes of these issues.

Maybe the conditionals for "breakpoint can extended into new objfiles" could
be extended so that most common most simple practical use cases do not get
affected.

For example if the breakpoint is function-name kind (not [file:]line), exinst
placement has single location, it is not inlined instance and it is not
template instance?  Any other ideas for restrictions?


Thanks,
Jan

Re: [RFA] Improve performance with lots of shared libraries

[Pedro Alves]

On Friday 09 September 2011 20:32:40, Jan Kratochvil wrote:
> On Fri, 09 Sep 2011 16:53:22 +0200, Pedro Alves wrote:
> > Not according to <http://sourceware.org/ml/gdb-patches/2011-08/msg00032.html>
> 
> Oops, I read it but ...
> 
> On Fri, 09 Sep 2011 17:09:31 +0200, Pedro Alves wrote:
> > ... and I don't think that's entirely true.  Yes, GDB will stop
> > trying to resolve the spec to a symbol, but at every event gdb will
> > still re-set the breakpoint locations.  If the new library has
> > the _same_ file:lineno compiled in (e.g., we put a breakpoint
> > on a c++ template in a header, or an inline function that is
> > also used by new library), then we should be getting new locations in
> > the new shared library, today.
> 
> I would not state it so optimistically, in my experience the watchpoints to
> inlined functions (PR 10738) and templates (file:line will break just at the
> one instance kind, not all of them) do not work much.  But I agree the design
> of extensions should not block fixes of these issues.
> 
> Maybe the conditionals for "breakpoint can extended into new objfiles" could
> be extended so that most common most simple practical use cases do not get
> affected.
> 
> For example if the breakpoint is function-name kind (not [file:]line), exinst
> placement has single location, it is not inlined instance and it is not
> template instance?

Not sure that's a good restriction. :-(  Inline functions can have an out-of-line
copy too.  If you "break my_inline_function", and end up with only a breakpoint
at the out-of-line copy (there were no inline expansion in the program yet), and then
later the program loads a DSO that does have inline expansions of the same function, 
you'd want your breakpoint to now gain locations for those inline expansions.
I think Tom's branch is supposed to make this case work that way.

> Any other ideas for restrictions?

Only one, when Tom's work is done, make breakpoints have a "final" property (meaning,
"stop adding locations for this breakpoint") for user breakpoints, and automatically
mark some internal breakpoints we know can't have more than one location with that
flag.  Breakpoints on addresses ("b *FOO") would have the "final" property implicitly
as well.  If all the breakpoints in the table are "final", then we don't need to
track shared library loads.

-- 
Pedro Alves

Re: [RFA] Improve performance with lots of shared libraries

[Jan Kratochvil]

On Mon, 12 Sep 2011 14:17:39 +0200, Pedro Alves wrote:
> Only one, when Tom's work is done, make breakpoints have a "final" property (meaning,
> "stop adding locations for this breakpoint") for user breakpoints, and automatically
> mark some internal breakpoints we know can't have more than one location with that
> flag.  Breakpoints on addresses ("b *FOO") would have the "final" property implicitly
> as well.  If all the breakpoints in the table are "final", then we don't need to
> track shared library loads.

BTW I agree with this design.


Thanks,
Jan

Re: [RFA] Improve performance with lots of shared libraries

[Gary Benson]

Jan Kratochvil wrote:
> On Mon, 12 Sep 2011 14:17:39 +0200, Pedro Alves wrote:
> > Only one, when Tom's work is done, make breakpoints have a "final"
> > property (meaning, "stop adding locations for this breakpoint")
> > for user breakpoints, and automatically mark some internal
> > breakpoints we know can't have more than one location with that
> > flag.  Breakpoints on addresses ("b *FOO") would have the "final"
> > property implicitly as well.  If all the breakpoints in the table
> > are "final", then we don't need to track shared library loads.
> 
> BTW I agree with this design.

Me too.

I will leave this patch on ice until Tom's work is done, or at least
until Tom is back and can comment.

Thank you all for your comments and ideas.

Cheers,
Gary

-- 
http://gbenson.net/

Re: [RFA] Improve performance with lots of shared libraries

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Pedro> Only one, when Tom's work is done, make breakpoints have a "final"
Pedro> property (meaning, "stop adding locations for this breakpoint")
Pedro> for user breakpoints, and automatically mark some internal
Pedro> breakpoints we know can't have more than one location with that
Pedro> flag.  Breakpoints on addresses ("b *FOO") would have the "final"
Pedro> property implicitly as well.  If all the breakpoints in the table
Pedro> are "final", then we don't need to track shared library loads.

Jan> BTW I agree with this design.

Gary> Me too.

Gary> I will leave this patch on ice until Tom's work is done, or at least
Gary> until Tom is back and can comment.

I haven't read the patch yet, just this subthread.

I also agree with this approach; in fact I think it is what we all
agreed to in the long thread determining the new semantics.

Tom

Re: [RFA] Improve performance with lots of shared libraries

[Pedro Alves]

On Friday 09 September 2011 15:35:21, Jan Kratochvil wrote:
> On Fri, 09 Sep 2011 16:25:30 +0200, Daniel Jacobowitz wrote:
> > On Fri, Sep 9, 2011 at 8:31 AM, Gary Benson <gbenson@redhat.com> wrote:
> > >  * when there are no pending breakpoints,
> > 
> > If you "break foo", it might put that breakpoint in more than one
> > shared library.  If you load a new library with an implementation of
> > foo, we should stop on that one too.  How can we make that work
> > without processing the library events?
> 
> This feature was planned being aware of this problem.
> 
> It does not work currently, GDB just puts the breakpoint on a random first
> place found.

... and I don't think that's entirely true.  Yes, GDB will stop
trying to resolve the spec to a symbol, but at every event gdb will
still re-set the breakpoint locations.  If the new library has
the _same_ file:lineno compiled in (e.g., we put a breakpoint
on a c++ template in a header, or an inline function that is
also used by new library), then we should be getting new locations in
the new shared library, today.

-- 
Pedro Alves

Re: [RFA] Improve performance with lots of shared libraries

[Jan Kratochvil]

On Fri, 09 Sep 2011 14:31:56 +0200, Gary Benson wrote:
> There are two things I'm not sure about with it as it stands.  One is
> to do with program spaces: I noticed that breakpoints have a program
> space, but breakpoint locations also have a program space.  Is the way
> I have used these correct?

Tom is working on removing the program space from the breakpoint itself.  Or
at least Tom was discussing its removal.


> --- /dev/null
> +++ b/gdb/solib-bp-disable.c
[...]
> +/* Nonzero if multi-threaded inferior support is present.  */
> +
> +static int
> +multi_thread_support_availabie (void)

This should be in target.c (and probably renamed).


[...]
> +/* Enable or disable a single solib event breakpoint as appropriate.  */
> +
> +static int
> +update_solib_breakpoint (struct breakpoint *b, void *arg)
> +{
> +  int enable = *(int *) arg;
> +  struct bp_location *loc;
> +
> +  if (b->pspace != current_program_space)
> +    return 0;
> +
> +  if (b->type != bp_shlib_event)
> +    return 0;
> +  
> +  for (loc = b->loc; loc; loc = loc->next)
> +    if (loc->pspace == current_program_space)
> +      {
> +	if (enable && b->enable_state == bp_disabled)
> +	  b->enable_state = bp_enabled;
> +	else if (!enable && b->enable_state == bp_enabled)
> +	  b->enable_state = bp_disabled;
> +      }

After modifying any ENABLE_STATE you have to always call
update_global_location_list.  For some events it is already done, it should be
probably per-event (sometimes still duplicating the call but that is probably
OK).


> +
> +  return 0;
> +}
> +
> +/* Enable or disable solib event breakpoints as appropriate.  */
> +
> +void
> +update_solib_breakpoints ()
> +{
> +  int enable = stop_on_solib_events
> +    || !multi_thread_support_availabie ()
> +    || pending_breakpoints_exist ();

This formatting is not compliant with Emacs-driven GNU Coding Style, it should
be AFAIK:
  int enable = (stop_on_solib_events || !multi_thread_support_availabie ()
		|| pending_breakpoints_exist ());

[...]
> +void
> +_initialize_solib_bp_disable (void)
> +{
> +  /* Observe breakpoint operations so we can enable the shared
> +     library event breakpoint if there are breakpoints pending
> +     on shared library loads.  */
> +  observer_attach_breakpoint_created (breakpoint_event);
> +  observer_attach_breakpoint_modified (breakpoint_event);
> +  observer_attach_breakpoint_deleted (breakpoint_event);
> +
> +  /* We also need to watch for inferior creation, because the
> +     creation of the shared library event breakpoint does not
> +     cause a breakpoint_created notification so inferior_created
> +     is the next best thing.  */

Isn't more bug the missing notification?


> +  observer_attach_inferior_created (inferior_event);
> +
> +  /* Observe shared libraries being loaded and unloaded so we
> +     can disable the shared library event breakpoint once a
> +     thread debugging library has been loaded.  */
> +  observer_attach_solib_loaded (solib_event);
> +  observer_attach_solib_unloaded (solib_event);
> +}


There is open the problem of ambiguous breakpoints but otherwise it looks
great to me, thanks.


Jan

Re: [RFA] Improve performance with lots of shared libraries

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> Tom is working on removing the program space from the breakpoint
Jan> itself.  Or at least Tom was discussing its removal.

Yeah, I think we should remove the pspace and gdbarch fields from struct
breakpoint, because these don't make sense in a multi-location,
multi-inferior world.  I have not looked yet to see how hard this is.

>> +static int
>> +multi_thread_support_availabie (void)

Jan> This should be in target.c (and probably renamed).

Yeah, there's a typo in the name :-)

Tom

[RFA] Improve performance with lots of shared libraries

[Gary Benson]

[-- 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