[RFA 5/4 take 2] Improved linker-debugger interface

[RFA 5/4 take 2] Improved linker-debugger interface

[Gary Benson]

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

Hi all,

I did some profiling and realised that the probes patch I just mailed
had reintroduced calls to update_section_map, a slow function I did
some work to avoid calling last year:

  http://www.cygwin.com/ml/gdb-patches/2011-07/msg00460.html
  http://www.cygwin.com/ml/gdb-patches/2011-10/msg00361.html

Attached is a patch to avoid calling update_section_map from the
probes interface.  Updated timings are as follows:

  no of solibs    100     250     500     1000    2000    5000
  ------------------------------------------------------------
  old interface     1       3       9       35     141     942
  new interface     0       0       1        4      14      89
  (times in seconds)

So, with this patch GDB is not three but ten times faster.

Thanks,
Gary

-- 
http://gbenson.net/

[-- Attachment #2: rtld-stap-7.patch --]
[-- Type: text/plain, Size: 4603 bytes --]

2012-07-19  Gary Benson  <gbenson@redhat.com>

	* objfiles.h (inhibit_section_map_updates): New function
	declaration.
	(resume_section_map_updates): Likewise.
	* objfiles.c (objfile_pspace_info): New field "inhibit_updates".
	(find_pc_section): Do not update the section map if
	inhibit_updates is set.
	(inhibit_section_map_updates): New function.
	(resume_section_map_updates): Likewise.
	* solib-svr4.c (svr4_handle_solib_event): Inhibit section map
	updates for calls to evaluate_probe_argument.

diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 0df5798..c7db1f2 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -522,6 +522,19 @@ extern void set_objfile_data (struct objfile *objfile,
 extern void *objfile_data (struct objfile *objfile,
 			   const struct objfile_data *data);
 
+/* In normal use, the section map will be rebuilt by FIND_PC_SECTION
+   if objfiles have been added, removed or relocated since it was last
+   called.  Calling INHIBIT_SECTION_MAP_UPDATES will inhibit this
+   behavior until RESUME_SECTION_MAP_UPDATES is called.  If you call
+   INHIBIT_SECTION_MAP_UPDATES you must ensure that every call to
+   FIND_PC_SECTION in the inhibited region relates to a section that
+   is already in the section map and has not since been removed or
+   relocated.  */
+extern void inhibit_section_map_updates (void);
+
+/* Resume automatically rebuilding the section map as required.  */
+extern void resume_section_map_updates (void);
+
 extern void default_iterate_over_objfiles_in_search_order
   (struct gdbarch *gdbarch,
    iterate_over_objfiles_in_search_order_cb_ftype *cb,
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 5ff0eb2..327b5d6 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -70,6 +70,9 @@ struct objfile_pspace_info
   int objfiles_changed_p;
   struct obj_section **sections;
   int num_sections;
+
+  /* Nonzero if section map updates should be inhibited.  */
+  int inhibit_updates;
 };
 
 /* Per-program-space data key.  */
@@ -1289,7 +1292,7 @@ find_pc_section (CORE_ADDR pc)
     return s;
 
   pspace_info = get_objfile_pspace_data (current_program_space);
-  if (pspace_info->objfiles_changed_p != 0)
+  if (pspace_info->objfiles_changed_p && !pspace_info->inhibit_updates)
     {
       update_section_map (current_program_space,
 			  &pspace_info->sections,
@@ -1457,6 +1460,23 @@ objfiles_changed (void)
   get_objfile_pspace_data (current_program_space)->objfiles_changed_p = 1;
 }
 
+/* See comments in objfiles.h.  */
+
+void
+inhibit_section_map_updates (void)
+{
+  get_objfile_pspace_data (current_program_space)->inhibit_updates = 1;
+
+}
+
+/* See comments in objfiles.h.  */
+
+void
+resume_section_map_updates (void)
+{
+  get_objfile_pspace_data (current_program_space)->inhibit_updates = 0;
+}
+
 /* The default implementation for the "iterate_over_objfiles_in_search_order"
    gdbarch method.  It is equivalent to use the ALL_OBJFILES macro,
    searching the objfiles in the order they are stored internally,
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 17395f7..d3c388b 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1820,6 +1820,18 @@ svr4_handle_solib_event (bpstat bs)
   if (action == NAMESPACE_NO_ACTION)
     return;
 
+  /* EVALUATE_PROBE_ARGUMENT looks up symbols in the dynamic linker
+     using FIND_PC_SECTION.  FIND_PC_SECTION is accelerated by a cache
+     called the section map.  The section map is invalidated every
+     time a shared library is loaded or unloaded, and if the inferior
+     is generating a lot of shared library events then the section map
+     will be updated every time SVR4_HANDLE_SOLIB_EVENT is called.
+     We called FIND_PC_SECTION in SVR4_CREATE_SOLIB_EVENT_BREAKPOINTS,
+     so we can guarantee that the dynamic linker's sections are in the
+     section map.  We can therefore inhibit section map updates across
+     these calls to EVALUATE_PROBE_ARGUMENT and save a lot of time.  */
+  inhibit_section_map_updates ();
+
   val = evaluate_probe_argument (pi->probe, 0);
   if (val == NULL)
     goto error;
@@ -1851,6 +1863,8 @@ svr4_handle_solib_event (bpstat bs)
 	action = NAMESPACE_RELOAD;
     }
 
+  resume_section_map_updates ();
+
   if (action == NAMESPACE_UPDATE_OR_RELOAD)
     {
       if (namespace_update_incremental (info, lmid, lm, is_initial_ns))
@@ -1873,6 +1887,7 @@ svr4_handle_solib_event (bpstat bs)
   warning (_("Probes-based dynamic linker interface failed.\n"
 	     "Reverting to original interface.\n"));
 
+  resume_section_map_updates ();
   free_namespace_table (info);
   free_probes (info);
   info->using_probes = 0;

Re: [RFA 5/4 take 2] Improved linker-debugger interface

[dje]

Gary Benson writes:
 > Hi all,
 > 
 > I did some profiling and realised that the probes patch I just mailed
 > had reintroduced calls to update_section_map, a slow function I did
 > some work to avoid calling last year:
 > 
 >   http://www.cygwin.com/ml/gdb-patches/2011-07/msg00460.html
 >   http://www.cygwin.com/ml/gdb-patches/2011-10/msg00361.html
 > 
 > Attached is a patch to avoid calling update_section_map from the
 > probes interface.  Updated timings are as follows:
 > 
 >   no of solibs    100     250     500     1000    2000    5000
 >   ------------------------------------------------------------
 >   old interface     1       3       9       35     141     942
 >   new interface     0       0       1        4      14      89
 >   (times in seconds)
 > 
 > So, with this patch GDB is not three but ten times faster.

Cool.

Not that you have to write it, but if you've got the beginnings of something
already :-), gdb should have a performance testsuite.

It needn't run it with "make check" since it could quite expectedly
increase the time to run the normal testsuite beyond a reasonable threshold.
But we should start collecting testcases to exercise gdb's performance,
and one nice way to do this is to have testcase generators.
I wouldn't want to manually write 5000 shared libs :-), but I do
want to track gdb's performance of handling that much over time,
and it's rather straightforward to write a program that will generate
a specified number of shared libs.
[And similarly for other performance issues we want to track.]

The result of such a testsuite needn't be PASS/FAIL, per se.
As a start it just needs to report numbers.
[Comparing absolute performance depends on the machine used to run the test,
etc. etc. etc.  I think these are solvable problems, and I think this is
something we need to do.]

Thoughts?

Re: [RFA 5/4 take 2] Improved linker-debugger interface

[Gary Benson]

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

dje@google.com wrote:
> Not that you have to write it, but if you've got the beginnings of
> something already :-), gdb should have a performance testsuite.

FWIW, this is what I was using.  Not much of a start, I know!

Cheers,
Gary

-- 
http://gbenson.net/

[-- Attachment #2: test.c --]
[-- Type: text/plain, Size: 681 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <dlfcn.h>

int
main(int argc, char *argv[])
{
  time_t start;
  int count, i;

  if (argc != 2)
    {
      fprintf (stderr, "usage: %s COUNT\n", argv[0]);
      exit (EXIT_FAILURE);
    }

  count = atoi (argv[1]);
  start = time (NULL);

  for (i = 1; i <= count; i++)
    {
      char path[32];
      void *handle;

      sprintf (path, "./lib%04d.so", i);
      handle = dlopen (path, RTLD_LAZY);
      if (handle == NULL)
	{
	  fprintf (stderr, "%s: dlopen failed\n", path);
	  exit (EXIT_FAILURE);
	}
    }

  printf ("%d libraries loaded in %d seconds.\n", count, time (NULL) - start);
}

[-- Attachment #3: build.sh --]
[-- Type: application/x-sh, Size: 168 bytes --]

Re: [RFA 5/4 take 2] Improved linker-debugger interface

[André Pönitz]

On Thu, Jul 19, 2012 at 04:19:13PM +0100, Gary Benson wrote:
> Hi all,
> 
> I did some profiling and realised that the probes patch I just mailed
> had reintroduced calls to update_section_map, a slow function I did
> some work to avoid calling last year:
> 
>   http://www.cygwin.com/ml/gdb-patches/2011-07/msg00460.html
>   http://www.cygwin.com/ml/gdb-patches/2011-10/msg00361.html
> 
> Attached is a patch to avoid calling update_section_map from the
> probes interface.  Updated timings are as follows:
> 
>   no of solibs    100     250     500     1000    2000    5000
>   ------------------------------------------------------------
>   old interface     1       3       9       35     141     942
>   new interface     0       0       1        4      14      89
>   (times in seconds)
> 
> So, with this patch GDB is not three but ten times faster.

Nice.

Andre'
 
PS: It pretty much looks like there is still some quadratic
behaviour somewhere...

Re: [RFA 5/4 take 2] Improved linker-debugger interface

[Gary Benson]

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

André Pönitz wrote:
> On Thu, Jul 19, 2012 at 04:19:13PM +0100, Gary Benson wrote:
> > Attached is a patch to avoid calling update_section_map from the
> > probes interface.  Updated timings are as follows:
> > 
> >   no of solibs    100     250     500     1000    2000    5000
> >   ------------------------------------------------------------
> >   old interface     1       3       9       35     141     942
> >   new interface     0       0       1        4      14      89
> >   (times in seconds)
> > 
> > So, with this patch GDB is not three but ten times faster.
> 
> It pretty much looks like there is still some quadratic behaviour
> somewhere...

Yes.  It's difficult to avoid given the current architecture.
target_so_ops->current_sos() is expected to return a freshly
allocated list of libraries to update_solib_list(), and the
latter is expected to free them.  This makes sense for the
standard interface, which reads everything from the inferior
into a list and returns it.  The probes interface already has
everything read by the time target_so_ops->current_sos() is
called, but it can't just return its own copy because the
rest of GDB will start freeing bits of it, so it needs to
copy it.  Every time a library is loaded, a new copy of the
list is made, but every time a library is loaded the list is
one element bigger.  You can see all the memcpy calls in the
profile I attached.

Cheers,
Gary

-- 
http://gbenson.net/

[-- Attachment #2: profile --]
[-- Type: text/plain, Size: 1334 bytes --]

samples  %        image name     symbol name
-------------------------------------------------------------------------
347937   11.4258  no-vmlinux     /no-vmlinux
317639   10.4309  libc-2.13.so   __memcpy_sse2
231494    7.6020  gdb            lookup_minimal_symbol_text
230405    7.5662  gdb            objfile_data
206993    6.7974  libc-2.13.so   __GI_memset
169853    5.5778  gdb            svr4_same
154056    5.0590  gdb            lookup_minimal_symbol
105589    3.4674  libc-2.13.so   _int_free
99916     3.2811  gdb            solib_add
93557     3.0723  libc-2.13.so   _int_malloc
78125     2.5655  gdb            namespace_update_incremental
74170     2.4357  gdb            create_longjmp_master_breakpoint
70165     2.3041  libc-2.13.so   __strcmp_sse2
62896     2.0654  gdb            lookup_minimal_symbol_and_objfile
55876     1.8349  gdb            create_overlay_event_breakpoint
51030     1.6758  gdb            get_objfile_arch
46512     1.5274  gdb            create_exception_master_breakpoint
45851     1.5057  gdb            create_std_terminate_master_breakpoint
40766     1.3387  ld-2.16.90.so  _dl_name_match_p
40233     1.3212  gdb            allocate_objfile
34723     1.1403  ld-2.16.90.so  _dl_map_object_from_fd
33676     1.1059  libc-2.13.so   freehook
31009     1.0183  ld-2.16.90.so  _dl_map_object

Re: [RFA 5/4 take 2] Improved linker-debugger interface

[Tom Tromey]

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

>> It pretty much looks like there is still some quadratic behaviour
>> somewhere...

Gary> Yes.  It's difficult to avoid given the current architecture.

This sounds like a good follow-up patch :-)

If it is a reasonable amount of effort, and you aren't totally sick of
this area, I guess you could just do it.  Otherwise a bug report to note
the difficulties would be nice.

Tom

Re: [RFA 5/4 take 2] Improved linker-debugger interface

[Tom Tromey]

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

Gary> So, with this patch GDB is not three but ten times faster.

Excellent work.

Gary> +/* See comments in objfiles.h.  */
Gary> +
Gary> +void
Gary> +inhibit_section_map_updates (void)
Gary> +{
Gary> +  get_objfile_pspace_data (current_program_space)->inhibit_updates = 1;
Gary> +
Gary> +}

Spurious blank line before the "}".

Gary> @@ -1820,6 +1820,18 @@ svr4_handle_solib_event (bpstat bs)
[...]
Gary> +  inhibit_section_map_updates ();
[...]
Gary> +  resume_section_map_updates ();

If there is any possibility of throwing an exception between these
calls, then there should be a cleanup instead.

I didn't dig up the patch supplying svr4_handle_solib_event to see for
myself.

If there is no possibility, then this patch is ok.

Tom

Re: [RFA 5/4 take 2] Improved linker-debugger interface

[Gary Benson]

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

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> Gary> +/* See comments in objfiles.h.  */
> Gary> +
> Gary> +void
> Gary> +inhibit_section_map_updates (void)
> Gary> +{
> Gary> +  get_objfile_pspace_data (current_program_space)->inhibit_updates = 1;
> Gary> +
> Gary> +}
> 
> Spurious blank line before the "}".
> 
> Gary> @@ -1820,6 +1820,18 @@ svr4_handle_solib_event (bpstat bs)
> [...]
> Gary> +  inhibit_section_map_updates ();
> [...]
> Gary> +  resume_section_map_updates ();
> 
> If there is any possibility of throwing an exception between these
> calls, then there should be a cleanup instead.
[snip]
> If there is no possibility, then this patch is ok.

I think it is possible, so I've changed it.

Attached is an updated patch with both these fixes.

Thanks,
Gary

-- 
http://gbenson.net/

[-- Attachment #2: rtld-stap-7.patch --]
[-- Type: text/plain, Size: 5113 bytes --]

2012-07-30  Gary Benson  <gbenson@redhat.com>

	* objfiles.h (inhibit_section_map_updates): New function
	declaration.
	(resume_section_map_updates): Likewise.
	(resume_section_map_updates_cleanup): Likewise.
	* objfiles.c (objfile_pspace_info): New field "inhibit_updates".
	(find_pc_section): Do not update the section map if
	inhibit_updates is set.
	(inhibit_section_map_updates): New function.
	(resume_section_map_updates): Likewise.
	(resume_section_map_updates_cleanup): Likewise.
	* solib-svr4.c (svr4_handle_solib_event): Inhibit section map
	updates for calls to evaluate_probe_argument.

diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 0df5798..c7db1f2 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -522,6 +522,22 @@
 extern void *objfile_data (struct objfile *objfile,
 			   const struct objfile_data *data);
 
+/* In normal use, the section map will be rebuilt by FIND_PC_SECTION
+   if objfiles have been added, removed or relocated since it was last
+   called.  Calling INHIBIT_SECTION_MAP_UPDATES will inhibit this
+   behavior until RESUME_SECTION_MAP_UPDATES is called.  If you call
+   INHIBIT_SECTION_MAP_UPDATES you must ensure that every call to
+   FIND_PC_SECTION in the inhibited region relates to a section that
+   is already in the section map and has not since been removed or
+   relocated.  */
+extern void inhibit_section_map_updates (void);
+
+/* Resume automatically rebuilding the section map as required.  */
+extern void resume_section_map_updates (void);
+
+/* Version of the above suitable for use as a cleanup.  */
+extern void resume_section_map_updates_cleanup (void *arg);
+
 extern void default_iterate_over_objfiles_in_search_order
   (struct gdbarch *gdbarch,
    iterate_over_objfiles_in_search_order_cb_ftype *cb,
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 5ff0eb2..327b5d6 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -70,6 +70,9 @@ struct objfile_pspace_info
   int objfiles_changed_p;
   struct obj_section **sections;
   int num_sections;
+
+  /* Nonzero if section map updates should be inhibited.  */
+  int inhibit_updates;
 };
 
 /* Per-program-space data key.  */
@@ -1289,7 +1292,7 @@ find_pc_section (CORE_ADDR pc)
     return s;
 
   pspace_info = get_objfile_pspace_data (current_program_space);
-  if (pspace_info->objfiles_changed_p != 0)
+  if (pspace_info->objfiles_changed_p && !pspace_info->inhibit_updates)
     {
       update_section_map (current_program_space,
 			  &pspace_info->sections,
@@ -1457,6 +1460,30 @@
   get_objfile_pspace_data (current_program_space)->objfiles_changed_p = 1;
 }
 
+/* See comments in objfiles.h.  */
+
+void
+inhibit_section_map_updates (void)
+{
+  get_objfile_pspace_data (current_program_space)->inhibit_updates = 1;
+}
+
+/* See comments in objfiles.h.  */
+
+void
+resume_section_map_updates (void)
+{
+  get_objfile_pspace_data (current_program_space)->inhibit_updates = 0;
+}
+
+/* See comments in objfiles.h.  */
+
+void
+resume_section_map_updates_cleanup (void *arg)
+{
+  resume_section_map_updates ();
+}
+
 /* The default implementation for the "iterate_over_objfiles_in_search_order"
    gdbarch method.  It is equivalent to use the ALL_OBJFILES macro,
    searching the objfiles in the order they are stored internally,
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 17395f7..d3c388b 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1796,6 +1796,7 @@
   struct svr4_info *info = get_svr4_info ();
   struct probe_and_info buf, *pi;
   enum probe_action action;
+  struct cleanup *cleanups = NULL;
   struct value *val;
   LONGEST lmid;
   CORE_ADDR debug_base, lm = 0;
@@ -1820,6 +1821,19 @@
   if (action == NAMESPACE_NO_ACTION)
     return;
 
+  /* EVALUATE_PROBE_ARGUMENT looks up symbols in the dynamic linker
+     using FIND_PC_SECTION.  FIND_PC_SECTION is accelerated by a cache
+     called the section map.  The section map is invalidated every
+     time a shared library is loaded or unloaded, and if the inferior
+     is generating a lot of shared library events then the section map
+     will be updated every time SVR4_HANDLE_SOLIB_EVENT is called.
+     We called FIND_PC_SECTION in SVR4_CREATE_SOLIB_EVENT_BREAKPOINTS,
+     so we can guarantee that the dynamic linker's sections are in the
+     section map.  We can therefore inhibit section map updates across
+     these calls to EVALUATE_PROBE_ARGUMENT and save a lot of time.  */
+  inhibit_section_map_updates ();
+  cleanups = make_cleanup (resume_section_map_updates_cleanup, NULL);
+
   val = evaluate_probe_argument (pi->probe, 0);
   if (val == NULL)
     goto error;
@@ -1851,6 +1865,9 @@
 	action = NAMESPACE_RELOAD;
     }
 
+  do_cleanups (cleanups);
+  cleanups = NULL;
+
   if (action == NAMESPACE_UPDATE_OR_RELOAD)
     {
       if (namespace_update_incremental (info, lmid, lm, is_initial_ns))
@@ -1873,6 +1890,8 @@
   warning (_("Probes-based dynamic linker interface failed.\n"
 	     "Reverting to original interface.\n"));
 
+  if (cleanups != NULL)
+    do_cleanups (cleanups);
   free_namespace_table (info);
   free_probes (info);
   info->using_probes = 0;

Re: [RFA 5/4 take 2] Improved linker-debugger interface

[Tom Tromey]

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

Gary> 2012-07-30  Gary Benson  <gbenson@redhat.com>
Gary> 	* objfiles.h (inhibit_section_map_updates): New function
Gary> 	declaration.
Gary> 	(resume_section_map_updates): Likewise.
Gary> 	(resume_section_map_updates_cleanup): Likewise.
Gary> 	* objfiles.c (objfile_pspace_info): New field "inhibit_updates".
Gary> 	(find_pc_section): Do not update the section map if
Gary> 	inhibit_updates is set.
Gary> 	(inhibit_section_map_updates): New function.
Gary> 	(resume_section_map_updates): Likewise.
Gary> 	(resume_section_map_updates_cleanup): Likewise.
Gary> 	* solib-svr4.c (svr4_handle_solib_event): Inhibit section map
Gary> 	updates for calls to evaluate_probe_argument.

Ok.

Tom