[RFA] Improve performance with lots of shared libraries

[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

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

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

[Tom Tromey]

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

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

Gary> 2011-09-22  Gary Benson  <gbenson@redhat.com>
Gary> 	* infrun.c (stopped_at_solib_event_breakpoint): New function.
Gary> 	(stopped_at_solib_event_breakpoint_helper): Likewise.
Gary> 	(handle_inferior_event): Avoid calling skip_inline_frames
Gary> 	when at the solib event breakpoint.

It looks reasonable to me, but I would much prefer a review from Pedro.

Tom

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

[Pedro Alves]

Hi Gary,

On Tuesday 04 October 2011 11:25:18, Gary Benson wrote:

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

As I mentioned before, better would for this to be a property/function of
the breakpoint itself (and its locations) -- meaning "I am / am not an
inlined location".  There's no reason to limit this just to the solib
event breakpoint.  An obvious example without even that generalization
is the thread event breakpoint.

Even if you don't do that, please add this new function (that checks
whether we stopped at an address that can't be inlined) to breakpoint.c
instead, and call it `stopped_at_non_inline_function' or something
along those lines.  We can later rework its internals keeping
its interface.

> +{
> +  struct execution_control_state *ecs
> +    = (struct execution_control_state *) arg;
> +

You need to check that the breakpoint is enabled and installed.

> +  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))

I don't follow the prev_pc check.  What does that intend to do?

When working in the direction of matching from an event back to our tables
and symbolic info, you start from the address_space instead.  Get at the
address space of the current PC
using `get_regcache_aspace (get_thread_regcache (ecs->ptid))'.  But better
yet, since you'd end up rewritting bkpt_breakpoint_hit, it's better instead
to reuse bpstat_check_location (despite the name, it doesn't work with
a bpstat).  See bpstop_status->bpstat_check_location->bkpt_breakpoint_hit,
and factor out the necessary bits.

-- 
Pedro Alves

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

[Gary Benson]

Hi Pedro,

Pedro Alves wrote:
> On Tuesday 04 October 2011 11:25:18, Gary Benson wrote:
> >   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)
> 

> As I mentioned before, better would for this to be a
> property/function of the breakpoint itself (and its locations) --
> meaning "I am / am not an inlined location".  There's no reason to
> limit this just to the solib event breakpoint.  An obvious example
> without even that generalization is the thread event breakpoint.

Are you talking about a function like this:

  int non_inline_function (struct breakpoint *b)
  {
    return b->type == bp_shlib_event;
  }

or something more involved?

> Even if you don't do that, please add this new function (that checks
> whether we stopped at an address that can't be inlined) to
> breakpoint.c instead, and call it `stopped_at_non_inline_function'
> or something along those lines.  We can later rework its internals
> keeping its interface.

Ok.

> > +{
> > +  struct execution_control_state *ecs
> > +    = (struct execution_control_state *) arg;
> > +
> 
> You need to check that the breakpoint is enabled and installed.

Does it matter if it isn't?  My thinking was that we are trying to
state that there cannot be inlined functions at the stop address, and
the existence of the shlib event breakpoint there allows you to state
that whether it is enabled or not.

(Am I correct in thinking "installed" means "not-pending"?)

> > +  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))
> 
> I don't follow the prev_pc check.  What does that intend to do?

You end up in handle_inferior_event twice for every breakpoint, once
when you hit it and once when you've single-stepped over it.  The
prev_pc check catches the latter.

> When working in the direction of matching from an event back to our
> tables and symbolic info, you start from the address_space instead.
> Get at the address space of the current PC using
> `get_regcache_aspace (get_thread_regcache (ecs->ptid))'.  But better
> yet, since you'd end up rewritting bkpt_breakpoint_hit, it's better
> instead to reuse bpstat_check_location (despite the name, it doesn't
> work with a bpstat).  See
> bpstop_status->bpstat_check_location->bkpt_breakpoint_hit, and
> factor out the necessary bits.

Ok, I will look into this.

Thank you,
Gary

-- 
http://gbenson.net/

[RFA take 2] Improve performance with lots of shared libraries

[Gary Benson]

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

Hi Pedro,

Pedro Alves wrote:
> On Tuesday 04 October 2011 11:25:18, Gary Benson wrote:
> >   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.
> 
> As I mentioned before, better would for this to be a
> property/function of the breakpoint itself (and its locations) --
> meaning "I am / am not an inlined location".  There's no reason to
> limit this just to the solib event breakpoint.  An obvious example
> without even that generalization is the thread event breakpoint.
> 
> Even if you don't do that, please add this new function (that checks
> whether we stopped at an address that can't be inlined) to
> breakpoint.c instead, and call it `stopped_at_non_inline_function'
> or something along those lines.  We can later rework its internals
> keeping its interface.
...
> You need to check that the breakpoint is enabled and installed.
...
> When working in the direction of matching from an event back to our
> tables and symbolic info, you start from the address_space instead.
> Get at the address space of the current PC using
> `get_regcache_aspace (get_thread_regcache (ecs->ptid))'.  But better
> yet, since you'd end up rewritting bkpt_breakpoint_hit, it's better
> instead to reuse bpstat_check_location (despite the name, it doesn't
> work with a bpstat).  See
> bpstop_status->bpstat_check_location->bkpt_breakpoint_hit, and
> factor out the necessary bits.

Further to my mail yesterday with questions/explanations, attached is
a patch which does what I think you are asking.  Is this what you had
in mind?

Note that I haven't extended it to recognise anything other than the
shared library event breakpoint as a non-inline location.  If possible
I'd like to handle other cases in a separate patch as it would require
further discussion.

Thanks,
Gary

-- 
http://gbenson.net/

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

2011-10-11  Gary Benson  <gbenson@redhat.com>

	* breakpoint.h (stopped_at_non_inline_function): Declare.
	* breakpoint.c (is_non_inline_function,
	stopped_at_non_inline_function): New functions.
	* infrun.c (handle_inferior_event): Don't call skip_inline_frames
	if the stop is at a location where functions cannot be inlined.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 08ff69b..5782d3d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -13325,6 +13325,45 @@ iterate_over_breakpoints (int (*callback) (struct breakpoint *, void *),
   return NULL;
 }
 
+/* Zero if any of the breakpoint's locations could be a location where
+   functions have been inlined, nonzero otherwise.  */
+
+static int
+is_non_inline_function (struct breakpoint *b)
+{
+  /* The shared library event breakpoint is set on the address of a
+     non-inline function.  */
+  if (b->type == bp_shlib_event)
+    return 1;
+
+  return 0;
+}
+
+/* Nonzero if the specified PC cannot be a location where functions
+   have been inlined.  */
+
+int
+stopped_at_non_inline_function (struct address_space *aspace, CORE_ADDR pc)
+{
+  struct breakpoint *b;
+  struct bp_location *bl;
+
+  ALL_BREAKPOINTS (b)
+    {
+      if (!is_non_inline_function (b))
+	continue;
+
+      for (bl = b->loc; bl != NULL; bl = bl->next)
+	{
+	  if (!bl->shlib_disabled
+	      && bpstat_check_location (bl, aspace, pc))
+	    return 1;
+	}
+    }
+
+  return 0;
+}
+
 void
 initialize_breakpoint_ops (void)
 {
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 5e5d1b9..a4e5a4f 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1357,6 +1357,12 @@ extern void end_rbreak_breakpoints (void);
 extern struct breakpoint *iterate_over_breakpoints (int (*) (struct breakpoint *,
 							     void *), void *);
 
+/* Nonzero if the specified PC cannot be a location where functions
+   have been inlined.  */
+
+extern int stopped_at_non_inline_function (struct address_space *aspace,
+					   CORE_ADDR pc);
+			 
 extern int user_breakpoint_p (struct breakpoint *);
 
 #endif /* !defined (BREAKPOINT_H) */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index cc2e29b..11ba700 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4044,7 +4044,23 @@ handle_inferior_event (struct execution_control_state *ecs)
      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);
+    {
+      struct address_space *aspace = 
+	get_regcache_aspace (get_thread_regcache (ecs->ptid));
+
+      /* skip_inline_frames is expensive, so we avoid it if we can
+	 determine that the address is one where functions cannot have
+	 been inlined.  This improves performance with inferiors that
+	 load a lot of shared libraries, because the solib event
+	 breakpoint is defined as the address of a function (i.e. not
+	 inline).  Note that we have to check the previous PC as well
+	 as the current one to catch cases when we have just
+	 single-stepped over a breakpoint prior to reinstating it.  */
+      if (!stopped_at_non_inline_function (aspace, stop_pc)
+	  && !stopped_at_non_inline_function (aspace,
+					      ecs->event_thread->prev_pc))
+	skip_inline_frames (ecs->ptid);
+    }
 
   if (ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP
       && ecs->event_thread->control.trap_expected

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

[Pedro Alves]

On Tuesday 11 October 2011 16:53:01, Gary Benson wrote:
> Hi Pedro,
> 
> Pedro Alves wrote:
> > On Tuesday 04 October 2011 11:25:18, Gary Benson wrote:
> > >   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.
> > 
> > As I mentioned before, better would for this to be a
> > property/function of the breakpoint itself (and its locations) --
> > meaning "I am / am not an inlined location".  There's no reason to
> > limit this just to the solib event breakpoint.  An obvious example
> > without even that generalization is the thread event breakpoint.
> > 
> > Even if you don't do that, please add this new function (that checks
> > whether we stopped at an address that can't be inlined) to
> > breakpoint.c instead, and call it `stopped_at_non_inline_function'
> > or something along those lines.  We can later rework its internals
> > keeping its interface.
> ...
> > You need to check that the breakpoint is enabled and installed.
> ...
> > When working in the direction of matching from an event back to our
> > tables and symbolic info, you start from the address_space instead.
> > Get at the address space of the current PC using
> > `get_regcache_aspace (get_thread_regcache (ecs->ptid))'.  But better
> > yet, since you'd end up rewritting bkpt_breakpoint_hit, it's better
> > instead to reuse bpstat_check_location (despite the name, it doesn't
> > work with a bpstat).  See
> > bpstop_status->bpstat_check_location->bkpt_breakpoint_hit, and
> > factor out the necessary bits.
> 
> Further to my mail yesterday with questions/explanations, attached is
> a patch which does what I think you are asking.  Is this what you had
> in mind?

Eh, sorry, you were too fast.  :-)  Yeah, something like that.

> Note that I haven't extended it to recognise anything other than the
> shared library event breakpoint as a non-inline location.  If possible
> I'd like to handle other cases in a separate patch as it would require
> further discussion.

Yes, a good idea.

>    if (ecs->event_thread->control.step_range_end != 1)
> -    skip_inline_frames (ecs->ptid);
> +    {
> +      struct address_space *aspace = 
> +       get_regcache_aspace (get_thread_regcache (ecs->ptid));
> +
> +      /* skip_inline_frames is expensive, so we avoid it if we can
> +        determine that the address is one where functions cannot have
> +        been inlined.  This improves performance with inferiors that
> +        load a lot of shared libraries, because the solib event
> +        breakpoint is defined as the address of a function (i.e. not
> +        inline).  Note that we have to check the previous PC as well
> +        as the current one to catch cases when we have just
> +        single-stepped over a breakpoint prior to reinstating it.  */
> +      if (!stopped_at_non_inline_function (aspace, stop_pc)
> +         && !stopped_at_non_inline_function (aspace,
> +                                             ecs->event_thread->prev_pc))
> +       skip_inline_frames (ecs->ptid);
> +    }

Thanks for extending the comment.  The first check is quite definitive.
The second is heuristic.  There's nothing preventing the event breakpoint
function to contain a bit of inlined code, and the single-step ending up
there.  If the user had set a breakpoint on that inlined code, the missing
skip_inline_frames call would break things.  Fortunately, that's an extremely
unlikely scenario.  But please do adjust the comment to mention that we
assume the code the single-step takes us to after single-stepping is also
not inlined.

Also, let's not trust prev_pc unless we did finish a single-step:

      if (!stopped_at_non_inline_function (aspace, stop_pc)
          && !(ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP
               && ecs->event_thread->control.trap_expected
               && stopped_at_non_inline_function (aspace,
                                                  ecs->event_thread->prev_pc))

Please drop the "stopped" from the function's name.  The prev_pc case
clearly isn't about having stopped at prev_pc.  Something like
pc_at_non_inline_function would be better.  It is just concerned with
returning whether its passed in PC is known to point at a non-inlined
location, whether the thread is currently stopped there or not.

Okay with these changes.

To make this generic for all breakpoints/stops, what I have in mind would be:

 - at breakpoint creation or re-set time, check if the locations we've created
   point at inlined code, and set a flag in the breakpoint's locations.
   We know the location is inlined or not from the debug info.  Breakpoint
   creation is the slow path, so that's okay.

 - given that we need to single-step over those breakpoints, we also need
   to know whether the PC after stepping over those breakpoints points at
   inlined code.  I think we can still do that at breakpoint creation or
   re-set time.  We'd need to reuse the software single-step machinery to
   know where the single-step would take us, and record somewhere
   that those locations point to inline code or not.  We'd also check this
   list in stopped_at_non_inline_function.  The software single-step machinery
   would need some cleaning up to make this possible.  It's interface,
   gdbarch_software_single_step, isn't fit for this.  The gdbarch hook should return
   a list of locations where to put the breakpoint, instead of implementations
   planting the breakpoints themselves, which would be a nice cleanup for other
   things too.  We'd also need to implement this hook for x86.  It's not
   implemented currently because x86 can do hardware single-stepping.

Thanks.

-- 
Pedro Alves

[commit] Improve performance with lots of shared libraries

[Gary Benson]

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

Pedro Alves wrote:
> Thanks for extending the comment.  The first check is quite
> definitive.  The second is heuristic.  There's nothing preventing
> the event breakpoint function to contain a bit of inlined code, and
> the single-step ending up there.  If the user had set a breakpoint
> on that inlined code, the missing skip_inline_frames call would
> break things.  Fortunately, that's an extremely unlikely scenario.
> But please do adjust the comment to mention that we assume the code
> the single-step takes us to after single-stepping is also not
> inlined.
> 
> Also, let's not trust prev_pc unless we did finish a single-step:
> 
>       if (!stopped_at_non_inline_function (aspace, stop_pc)
>           && !(ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP
>                && ecs->event_thread->control.trap_expected
>                && stopped_at_non_inline_function (aspace,
>                                                   ecs->event_thread->prev_pc))
> 
> Please drop the "stopped" from the function's name.  The prev_pc
> case clearly isn't about having stopped at prev_pc.  Something like
> pc_at_non_inline_function would be better.  It is just concerned
> with returning whether its passed in PC is known to point at a
> non-inlined location, whether the thread is currently stopped there
> or not.
> 
> Okay with these changes.

Thanks, I made them and committed it (patch attached).

> To make this generic for all breakpoints/stops, what I have in mind
> would be:
> 
>  - at breakpoint creation or re-set time, check if the locations
>    we've created point at inlined code, and set a flag in the
>    breakpoint's locations.  We know the location is inlined or not
>    from the debug info.  Breakpoint creation is the slow path, so
>    that's okay.
> 
>  - given that we need to single-step over those breakpoints, we also
>    need to know whether the PC after stepping over those breakpoints
>    points at inlined code.  I think we can still do that at
>    breakpoint creation or re-set time.  We'd need to reuse the
>    software single-step machinery to know where the single-step
>    would take us, and record somewhere that those locations point to
>    inline code or not.  We'd also check this list in
>    stopped_at_non_inline_function.  The software single-step
>    machinery would need some cleaning up to make this possible.
>    It's interface, gdbarch_software_single_step, isn't fit for this.
>    The gdbarch hook should return a list of locations where to put
>    the breakpoint, instead of implementations planting the
>    breakpoints themselves, which would be a nice cleanup for other
>    things too.  We'd also need to implement this hook for x86.  It's
>    not implemented currently because x86 can do hardware
>    single-stepping.

Ah, nice!  Would it be appropriate to file a bug containing this
information?  So it doesn't get lost before I have a chance to work
on it?

Thanks,
Gary

-- 
http://gbenson.net/

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

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.13424
retrieving revision 1.13425
diff -u -r1.13424 -r1.13425
--- gdb/ChangeLog	12 Oct 2011 12:17:29 -0000	1.13424
+++ gdb/ChangeLog	12 Oct 2011 15:43:47 -0000	1.13425
@@ -1,3 +1,11 @@
+2011-10-12  Gary Benson  <gbenson@redhat.com>
+
+	* breakpoint.h (pc_at_non_inline_function): Declare.
+	* breakpoint.c (is_non_inline_function,
+	pc_at_non_inline_function): New functions.
+	* infrun.c (handle_inferior_event): Don't call skip_inline_frames
+	if the stop is at a location where functions cannot be inlined.
+
 2011-10-12  Pedro Alves  <pedro@codesourcery.com>
 
 	* linux-nat.c (stop_and_resume_callback): Don't re-resume LWPs if
Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.615
retrieving revision 1.616
diff -u -r1.615 -r1.616
--- gdb/breakpoint.c	2 Oct 2011 02:13:12 -0000	1.615
+++ gdb/breakpoint.c	12 Oct 2011 15:43:48 -0000	1.616
@@ -13325,6 +13325,45 @@
   return NULL;
 }
 
+/* Zero if any of the breakpoint's locations could be a location where
+   functions have been inlined, nonzero otherwise.  */
+
+static int
+is_non_inline_function (struct breakpoint *b)
+{
+  /* The shared library event breakpoint is set on the address of a
+     non-inline function.  */
+  if (b->type == bp_shlib_event)
+    return 1;
+
+  return 0;
+}
+
+/* Nonzero if the specified PC cannot be a location where functions
+   have been inlined.  */
+
+int
+pc_at_non_inline_function (struct address_space *aspace, CORE_ADDR pc)
+{
+  struct breakpoint *b;
+  struct bp_location *bl;
+
+  ALL_BREAKPOINTS (b)
+    {
+      if (!is_non_inline_function (b))
+	continue;
+
+      for (bl = b->loc; bl != NULL; bl = bl->next)
+	{
+	  if (!bl->shlib_disabled
+	      && bpstat_check_location (bl, aspace, pc))
+	    return 1;
+	}
+    }
+
+  return 0;
+}
+
 void
 initialize_breakpoint_ops (void)
 {
Index: gdb/breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.160
retrieving revision 1.161
diff -u -r1.160 -r1.161
--- gdb/breakpoint.h	26 Aug 2011 09:28:27 -0000	1.160
+++ gdb/breakpoint.h	12 Oct 2011 15:43:49 -0000	1.161
@@ -1357,6 +1357,12 @@
 extern struct breakpoint *iterate_over_breakpoints (int (*) (struct breakpoint *,
 							     void *), void *);
 
+/* Nonzero if the specified PC cannot be a location where functions
+   have been inlined.  */
+
+extern int pc_at_non_inline_function (struct address_space *aspace,
+				      CORE_ADDR pc);
+
 extern int user_breakpoint_p (struct breakpoint *);
 
 #endif /* !defined (BREAKPOINT_H) */
Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.508
retrieving revision 1.509
diff -u -r1.508 -r1.509
--- gdb/infrun.c	7 Oct 2011 12:06:46 -0000	1.508
+++ gdb/infrun.c	12 Oct 2011 15:43:49 -0000	1.509
@@ -4044,7 +4044,32 @@
      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);
+    {
+      struct address_space *aspace = 
+	get_regcache_aspace (get_thread_regcache (ecs->ptid));
+
+      /* skip_inline_frames is expensive, so we avoid it if we can
+	 determine that the address is one where functions cannot have
+	 been inlined.  This improves performance with inferiors that
+	 load a lot of shared libraries, because the solib event
+	 breakpoint is defined as the address of a function (i.e. not
+	 inline).  Note that we have to check the previous PC as well
+	 as the current one to catch cases when we have just
+	 single-stepped off a breakpoint prior to reinstating it.
+	 Note that we're assuming that the code we single-step to is
+	 not inline, but that's not definitive: there's nothing
+	 preventing the event breakpoint function from containing
+	 inlined code, and the single-step ending up there.  If the
+	 user had set a breakpoint on that inlined code, the missing
+	 skip_inline_frames call would break things.  Fortunately
+	 that's an extremely unlikely scenario.  */
+      if (!pc_at_non_inline_function (aspace, stop_pc)
+          && !(ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP
+               && ecs->event_thread->control.trap_expected
+               && pc_at_non_inline_function (aspace,
+                                             ecs->event_thread->prev_pc)))
+	skip_inline_frames (ecs->ptid);
+    }
 
   if (ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP
       && ecs->event_thread->control.trap_expected

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

[Pedro Alves]

On Wednesday 12 October 2011 16:59:18, Gary Benson wrote:
> Pedro Alves wrote:

> > To make this generic for all breakpoints/stops, what I have in mind
> > would be:
> > 
> >  - at breakpoint creation or re-set time, check if the locations
> >    we've created point at inlined code, and set a flag in the
> >    breakpoint's locations.  We know the location is inlined or not
> >    from the debug info.  Breakpoint creation is the slow path, so
> >    that's okay.
> > 
> >  - given that we need to single-step over those breakpoints, we also
> >    need to know whether the PC after stepping over those breakpoints
> >    points at inlined code.  I think we can still do that at
> >    breakpoint creation or re-set time.  We'd need to reuse the
> >    software single-step machinery to know where the single-step
> >    would take us, and record somewhere that those locations point to
> >    inline code or not.  We'd also check this list in
> >    stopped_at_non_inline_function.  The software single-step
> >    machinery would need some cleaning up to make this possible.
> >    It's interface, gdbarch_software_single_step, isn't fit for this.
> >    The gdbarch hook should return a list of locations where to put
> >    the breakpoint, instead of implementations planting the
> >    breakpoints themselves, which would be a nice cleanup for other
> >    things too.  We'd also need to implement this hook for x86.  It's
> >    not implemented currently because x86 can do hardware
> >    single-stepping.
> 
> Ah, nice!  Would it be appropriate to file a bug containing this
> information?  So it doesn't get lost before I have a chance to work
> on it?

Sure!  What I haven't thought about much is whether this
optimization would be indeed a general win.  :-)  It'd make a
difference if you tend to have planted breakpoints that 
don't cause a stop often (e.g., some python breakpoint),
and maybe it'd make a difference on software single-step
targets, and a tiny bit on handling step-resume
breakpoints on hardware step targets?  I don't have a clear
picture where time is being spent (other than roundtripping
to the target).  Thread event breakpoints sound like low
hang fruit though.

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