[patch 1/2] Speed up JIT support

[patch 1/2] Speed up JIT support

[Paul Pluzhnikov]

Greetings,

In earlier thread: http://sourceware.org/ml/gdb/2011-01/msg00009.html, Pedro
suggested that we should eliminate repeated symbol lookups by stashing
relevant info in per-objfile data.

This series of patches implements the idea.

The first patch merely reshuffles the code around to make it more convenient
to cache the results. The second patch is the "meat" of the change.

Timing results using:

  /usr/bin/time ./gdb -nx -q -ex 'run' -ex quit --args \
    testsuite/gdb.base/jit-main testsuite/gdb.base/jit-solib.so N

### before ###
   N   time
 300   1.04user 0.19system 0:01.24elapsed 99%CPU
 400   2.75user 0.16system 0:02.94elapsed 98%CPU
 500   5.70user 0.30system 0:06.04elapsed 99%CPU
 600  10.98user 0.28system 0:11.31elapsed 99%CPU
 700  18.56user 0.30system 0:18.96elapsed 99%CPU
 800  28.57user 0.46system 0:29.16elapsed 99%CPU

### after ###
   N   time
 600   0.48user 0.25system 0:00.75elapsed 96%CPU
1000   1.04user 0.40system 0:01.44elapsed 99%CPU
2000   3.17user 0.95system 0:04.23elapsed 97%CPU
3000   7.20user 1.44system 0:08.89elapsed 97%CPU
4000  13.67user 1.93system 0:15.97elapsed 97%CPU
5000  23.41user 2.46system 0:26.41elapsed 97%CPU

This is still showing non-linear growth, but the factors are much nicer ;-)

Next up: move jit_breakpoint_re_set into breakpoint.c and make it use
the same mechanism (lookup_minimal_symbol{,_text} are still major CPU
consumers, though find_pc_section moves to the top spot).

Thanks,
--
Paul Pluzhnikov


2010-02-03  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* breakpoint.c (create_overlay_event_breakpoint): Adjust.
	(create_longjmp_master_breakpoint): Adjust.
	(create_std_terminate_master_breakpoint): Adjust.
	(update_breakpoints_after_exec): Adjust.
	(breakpoint_re_set): Adjust.


Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.532
diff -u -p -r1.532 breakpoint.c
--- breakpoint.c	31 Jan 2011 21:37:01 -0000	1.532
+++ breakpoint.c	3 Feb 2011 21:37:16 -0000
@@ -2218,9 +2218,10 @@ create_internal_breakpoint (struct gdbar
 }
 
 static void
-create_overlay_event_breakpoint (char *func_name)
+create_overlay_event_breakpoint (void)
 {
   struct objfile *objfile;
+  const char *const func_name = "_ovly_debug_event";
 
   ALL_OBJFILES (objfile)
     {
@@ -2251,48 +2252,64 @@ create_overlay_event_breakpoint (char *f
 }
 
 static void
-create_longjmp_master_breakpoint (char *func_name)
+create_longjmp_master_breakpoint (void)
 {
   struct program_space *pspace;
-  struct objfile *objfile;
   struct cleanup *old_chain;
 
   old_chain = save_current_program_space ();
 
   ALL_PSPACES (pspace)
-  ALL_OBJFILES (objfile)
+  {
+    struct objfile *objfile;
+
+    set_current_program_space (pspace);
+
+    ALL_OBJFILES (objfile)
     {
-      struct breakpoint *b;
-      struct minimal_symbol *m;
+      const char *const longjmp_names[]
+	= { "longjmp", "_longjmp", "siglongjmp", "_siglongjmp" };
+      const int num_longjmp_names
+	= sizeof (longjmp_names) / sizeof (longjmp_names[0]);
+      int i;
+      struct gdbarch *gdbarch;
 
-      if (!gdbarch_get_longjmp_target_p (get_objfile_arch (objfile)))
+      gdbarch = get_objfile_arch (objfile);
+      if (!gdbarch_get_longjmp_target_p (gdbarch))
 	continue;
 
-      set_current_program_space (pspace);
+      for (i = 0; i < num_longjmp_names; i++)
+	{
+	  struct breakpoint *b;
+	  struct minimal_symbol *m;
+	  const char *func_name;
 
-      m = lookup_minimal_symbol_text (func_name, objfile);
-      if (m == NULL)
-        continue;
+	  func_name = longjmp_names[i];
+	  m = lookup_minimal_symbol_text (func_name, objfile);
+	  if (m == NULL)
+	    continue;
 
-      b = create_internal_breakpoint (get_objfile_arch (objfile),
-				      SYMBOL_VALUE_ADDRESS (m),
-                                      bp_longjmp_master);
-      b->addr_string = xstrdup (func_name);
-      b->enable_state = bp_disabled;
+	  b = create_internal_breakpoint (gdbarch,
+					  SYMBOL_VALUE_ADDRESS (m),
+					  bp_longjmp_master);
+	  b->addr_string = xstrdup (func_name);
+	  b->enable_state = bp_disabled;
+	}
     }
+  }
   update_global_location_list (1);
 
   do_cleanups (old_chain);
 }
 
-/* Create a master std::terminate breakpoint.  The actual function
-   looked for is named FUNC_NAME.  */
+/* Create a master std::terminate breakpoint.  */
 static void
-create_std_terminate_master_breakpoint (const char *func_name)
+create_std_terminate_master_breakpoint (void)
 {
   struct program_space *pspace;
   struct objfile *objfile;
   struct cleanup *old_chain;
+  const char *const func_name = "std::terminate()";
 
   old_chain = save_current_program_space ();
 
@@ -2462,12 +2479,9 @@ update_breakpoints_after_exec (void)
       }
   }
   /* FIXME what about longjmp breakpoints?  Re-create them here?  */
-  create_overlay_event_breakpoint ("_ovly_debug_event");
-  create_longjmp_master_breakpoint ("longjmp");
-  create_longjmp_master_breakpoint ("_longjmp");
-  create_longjmp_master_breakpoint ("siglongjmp");
-  create_longjmp_master_breakpoint ("_siglongjmp");
-  create_std_terminate_master_breakpoint ("std::terminate()");
+  create_overlay_event_breakpoint ();
+  create_longjmp_master_breakpoint ();
+  create_std_terminate_master_breakpoint ();
   create_exception_master_breakpoint ();
 }
 
@@ -10716,12 +10730,9 @@ breakpoint_re_set (void)
 
   do_cleanups (old_chain);
 
-  create_overlay_event_breakpoint ("_ovly_debug_event");
-  create_longjmp_master_breakpoint ("longjmp");
-  create_longjmp_master_breakpoint ("_longjmp");
-  create_longjmp_master_breakpoint ("siglongjmp");
-  create_longjmp_master_breakpoint ("_siglongjmp");
-  create_std_terminate_master_breakpoint ("std::terminate()");
+  create_overlay_event_breakpoint ();
+  create_longjmp_master_breakpoint ();
+  create_std_terminate_master_breakpoint ();
   create_exception_master_breakpoint ();
 }
 \f

[patch 2/2] Speed up JIT support

[Paul Pluzhnikov]

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

Greetings,

This is the second part of the JIT speedup patch (applies on top of the
first patch).

I vaguely remember (but can't find a reference) that there is no CORE_ADDR
that is guaranteed to always be invalid, and so my use of -1 for
INVALID_CORE_ADDR may be problematic.

I also forgot to mention: tested on Linux/x86_64, no regressions.

Thanks,
--
Paul Pluzhnikov


2010-02-03  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* breakpoint.c (longjmp_names): New variable.
	(struct breakpoint_objfile_data): New type.
	(breakpoint_objfile_key): New variable.
	(get_breakpoint_objfile_data): New function.
	(create_overlay_event_breakpoint): Adjust.
	(create_longjmp_master_breakpoint): Adjust.
	(create_std_terminate_master_breakpoint): Adjust.
	(create_exception_master_breakpoint): Adjust.
	(_initialize_breakpoint): Adjust.

[-- Attachment #2: gdb-jit-speedup-20110203a.txt --]
[-- Type: text/plain, Size: 8070 bytes --]

--- breakpoint.c.orig	2011-02-03 13:46:47.000000000 -0800
+++ breakpoint.c	2011-02-03 13:51:46.000000000 -0800
@@ -2217,6 +2217,46 @@ create_internal_breakpoint (struct gdbar
   return b;
 }
 
+static const char *const longjmp_names[] =
+  {
+    "longjmp", "_longjmp", "siglongjmp", "_siglongjmp"
+  };
+#define NUM_LONGJMP_NAMES \
+  (sizeof (longjmp_names) / sizeof (longjmp_names[0]))
+
+/* Per-objfile data private to breakpoint.c  */
+struct breakpoint_objfile_data
+{
+  CORE_ADDR overlay_bp_addr;
+  CORE_ADDR longjmp_bp_addr[NUM_LONGJMP_NAMES];
+  CORE_ADDR terminate_bp_addr;
+  CORE_ADDR exception_bp_addr;
+};
+
+static const struct objfile_data *breakpoint_objfile_key;
+
+/* Return per-objfile data needed by breakpoint.c.
+   Allocate the data if necessary.  */
+
+static struct breakpoint_objfile_data *
+get_breakpoint_objfile_data (struct objfile *objfile)
+{
+  struct breakpoint_objfile_data *bp_objfile_data;
+
+  bp_objfile_data = objfile_data (objfile, breakpoint_objfile_key);
+  if (bp_objfile_data == NULL)
+    {
+      bp_objfile_data = obstack_alloc (&objfile->objfile_obstack,
+				       sizeof (*bp_objfile_data));
+
+      memset (bp_objfile_data, 0, sizeof (*bp_objfile_data));
+      set_objfile_data (objfile, breakpoint_objfile_key, bp_objfile_data);
+    }
+  return bp_objfile_data;
+}
+
+#define INVALID_CORE_ADDR ((CORE_ADDR)-1)
+
 static void
 create_overlay_event_breakpoint (void)
 {
@@ -2226,14 +2266,29 @@ create_overlay_event_breakpoint (void)
   ALL_OBJFILES (objfile)
     {
       struct breakpoint *b;
-      struct minimal_symbol *m;
+      struct breakpoint_objfile_data *bp_objfile_data;
 
-      m = lookup_minimal_symbol_text (func_name, objfile);
-      if (m == NULL)
-        continue;
+      bp_objfile_data = get_breakpoint_objfile_data (objfile);
+
+      if (bp_objfile_data->overlay_bp_addr == INVALID_CORE_ADDR)
+	continue;
+
+      if (bp_objfile_data->overlay_bp_addr == 0)
+	{
+	  struct minimal_symbol *m;
+
+	  m = lookup_minimal_symbol_text (func_name, objfile);
+	  if (m == NULL)
+	    {
+	      /* Avoid future lookups in this objfile.  */
+	      bp_objfile_data->overlay_bp_addr = INVALID_CORE_ADDR;
+	      continue;
+	    }
+	  bp_objfile_data->overlay_bp_addr = SYMBOL_VALUE_ADDRESS (m);
+	}
 
       b = create_internal_breakpoint (get_objfile_arch (objfile),
-				      SYMBOL_VALUE_ADDRESS (m),
+				      bp_objfile_data->overlay_bp_addr,
                                       bp_overlay_event);
       b->addr_string = xstrdup (func_name);
 
@@ -2267,30 +2322,41 @@ create_longjmp_master_breakpoint (void)
 
     ALL_OBJFILES (objfile)
     {
-      const char *const longjmp_names[]
-	= { "longjmp", "_longjmp", "siglongjmp", "_siglongjmp" };
-      const int num_longjmp_names
-	= sizeof (longjmp_names) / sizeof (longjmp_names[0]);
       int i;
       struct gdbarch *gdbarch;
+      struct breakpoint_objfile_data *bp_objfile_data;
 
       gdbarch = get_objfile_arch (objfile);
       if (!gdbarch_get_longjmp_target_p (gdbarch))
 	continue;
 
-      for (i = 0; i < num_longjmp_names; i++)
+      bp_objfile_data = get_breakpoint_objfile_data (objfile);
+
+      for (i = 0; i < NUM_LONGJMP_NAMES; i++)
 	{
 	  struct breakpoint *b;
-	  struct minimal_symbol *m;
 	  const char *func_name;
 
-	  func_name = longjmp_names[i];
-	  m = lookup_minimal_symbol_text (func_name, objfile);
-	  if (m == NULL)
+	  if (bp_objfile_data->longjmp_bp_addr[i] == INVALID_CORE_ADDR)
 	    continue;
 
+	  func_name = longjmp_names[i];
+	  if (bp_objfile_data->longjmp_bp_addr[i] == 0)
+	    {
+	      struct minimal_symbol *m;
+
+	      m = lookup_minimal_symbol_text (func_name, objfile);
+	      if (m == NULL)
+		{
+		  /* Prevent future lookups in this objfile.  */
+		  bp_objfile_data->longjmp_bp_addr[i] = INVALID_CORE_ADDR;
+		  continue;
+		}
+	      bp_objfile_data->longjmp_bp_addr[i] = SYMBOL_VALUE_ADDRESS (m);
+	    }
+
 	  b = create_internal_breakpoint (gdbarch,
-					  SYMBOL_VALUE_ADDRESS (m),
+					  bp_objfile_data->longjmp_bp_addr[i],
 					  bp_longjmp_master);
 	  b->addr_string = xstrdup (func_name);
 	  b->enable_state = bp_disabled;
@@ -2307,31 +2373,50 @@ static void
 create_std_terminate_master_breakpoint (void)
 {
   struct program_space *pspace;
-  struct objfile *objfile;
   struct cleanup *old_chain;
   const char *const func_name = "std::terminate()";
 
   old_chain = save_current_program_space ();
 
   ALL_PSPACES (pspace)
+  {
+    struct objfile *objfile;
+
+    set_current_program_space (pspace);
+
     ALL_OBJFILES (objfile)
     {
       struct breakpoint *b;
-      struct minimal_symbol *m;
+      struct breakpoint_objfile_data *bp_objfile_data;
 
-      set_current_program_space (pspace);
+      bp_objfile_data = get_breakpoint_objfile_data (objfile);
 
-      m = lookup_minimal_symbol (func_name, NULL, objfile);
-      if (m == NULL || (MSYMBOL_TYPE (m) != mst_text
-			&& MSYMBOL_TYPE (m) != mst_file_text))
-        continue;
+      if (bp_objfile_data->terminate_bp_addr == INVALID_CORE_ADDR)
+	continue;
+
+      if (bp_objfile_data->terminate_bp_addr == 0)
+	{
+	  struct minimal_symbol *m;
+
+	  m = lookup_minimal_symbol (func_name, NULL, objfile);
+	  if (m == NULL || (MSYMBOL_TYPE (m) != mst_text
+			    && MSYMBOL_TYPE (m) != mst_file_text))
+	    {
+	      /* Prevent future lookups in this objfile.  */
+	      bp_objfile_data->terminate_bp_addr = INVALID_CORE_ADDR;
+	      continue;
+	    }
+	  bp_objfile_data->terminate_bp_addr = SYMBOL_VALUE_ADDRESS (m);
+	}
 
       b = create_internal_breakpoint (get_objfile_arch (objfile),
-				      SYMBOL_VALUE_ADDRESS (m),
+				      bp_objfile_data->terminate_bp_addr,
                                       bp_std_terminate_master);
       b->addr_string = xstrdup (func_name);
       b->enable_state = bp_disabled;
     }
+  }
+
   update_global_location_list (1);
 
   do_cleanups (old_chain);
@@ -2343,24 +2428,45 @@ void
 create_exception_master_breakpoint (void)
 {
   struct objfile *objfile;
+  const char *const func_name = "_Unwind_DebugHook";
 
   ALL_OBJFILES (objfile)
     {
-      struct minimal_symbol *debug_hook;
+      struct breakpoint *b;
+      struct gdbarch *gdbarch;
+      struct breakpoint_objfile_data *bp_objfile_data;
 
-      debug_hook = lookup_minimal_symbol ("_Unwind_DebugHook", NULL, objfile);
-      if (debug_hook != NULL)
+      bp_objfile_data = get_breakpoint_objfile_data (objfile);
+
+      if (bp_objfile_data->exception_bp_addr == INVALID_CORE_ADDR)
+	continue;
+
+      gdbarch = get_objfile_arch (objfile);
+
+      if (bp_objfile_data->exception_bp_addr == 0)
 	{
-	  struct breakpoint *b;
-	  CORE_ADDR addr = SYMBOL_VALUE_ADDRESS (debug_hook);
-	  struct gdbarch *gdbarch = get_objfile_arch (objfile);
+	  struct minimal_symbol *debug_hook;
+	  CORE_ADDR addr;
 
-	  addr = gdbarch_convert_from_func_ptr_addr (gdbarch, addr,
-						     &current_target);
-	  b = create_internal_breakpoint (gdbarch, addr, bp_exception_master);
-	  b->addr_string = xstrdup ("_Unwind_DebugHook");
-	  b->enable_state = bp_disabled;
+	  debug_hook = lookup_minimal_symbol (func_name, NULL, objfile);
+	  if (debug_hook == NULL)
+	    {
+	      bp_objfile_data->exception_bp_addr = INVALID_CORE_ADDR;
+	      continue;
+	    }
+
+	  addr = SYMBOL_VALUE_ADDRESS (debug_hook);
+
+	  bp_objfile_data->exception_bp_addr
+	    = gdbarch_convert_from_func_ptr_addr (gdbarch, addr,
+						  &current_target);
 	}
+
+      b = create_internal_breakpoint (gdbarch,
+				      bp_objfile_data->exception_bp_addr,
+				      bp_exception_master);
+      b->addr_string = xstrdup (func_name);
+      b->enable_state = bp_disabled;
     }
 
   update_global_location_list (1);
@@ -12071,6 +12177,8 @@ _initialize_breakpoint (void)
   observer_attach_inferior_exit (clear_syscall_counts);
   observer_attach_memory_changed (invalidate_bp_value_on_memory_change);
 
+  breakpoint_objfile_key = register_objfile_data ();
+
   breakpoint_chain = 0;
   /* Don't bother to call set_breakpoint_count.  $bpnum isn't useful
      before a breakpoint is set.  */

Re: [patch 1/2] Speed up JIT support

[Pedro Alves]

On Friday 04 February 2011 21:46:14, Paul Pluzhnikov wrote:

> 2010-02-03  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	* breakpoint.c (create_overlay_event_breakpoint): Adjust.
> 	(create_longjmp_master_breakpoint): Adjust.
> 	(create_std_terminate_master_breakpoint): Adjust.
> 	(update_breakpoints_after_exec): Adjust.
> 	(breakpoint_re_set): Adjust.

Thanks, looks okay to me.  But please put a little
more info in the changelog entry mentioning what you changed
than just saying strictly "Adjust" everywhere.

-- 
Pedro Alves

Re: [patch 2/2] Speed up JIT support

[Pedro Alves]

On Friday 04 February 2011 21:58:06, Paul Pluzhnikov wrote:

> This is the second part of the JIT speedup patch (applies on top of the
> first patch).
> 
> I vaguely remember (but can't find a reference) that there is no CORE_ADDR
> that is guaranteed to always be invalid, and so my use of -1 for
> INVALID_CORE_ADDR may be problematic.

Yeah... In this case, I don't think it's problematic, but
it's certainly bad practice to assume -1 or 0 are special.
The "right" way to do this is to use an auxiliary variable
recording for example 'looked up' | 'looked up and
not found' | 'looked up and valid'.  Different states, but
see for example frame.h:struct frame_info->stack_addr|stack_addr_p.

Please don't be shy in documenting each field of
new structures.

> 2010-02-03  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	* breakpoint.c (longjmp_names): New variable.
> 	(struct breakpoint_objfile_data): New type.
> 	(breakpoint_objfile_key): New variable.
> 	(get_breakpoint_objfile_data): New function.
> 	(create_overlay_event_breakpoint): Adjust.
> 	(create_longjmp_master_breakpoint): Adjust.
> 	(create_std_terminate_master_breakpoint): Adjust.
> 	(create_exception_master_breakpoint): Adjust.
> 	(_initialize_breakpoint): Adjust.

Please expands those "Adjust"s a bit...  Examples:

...
	(create_exception_master_breakpoint): Check per-objfile cache
   for symbols first.
	(_initialize_breakpoint): Register per-objfile data key.
...

Otherwise looks okay to me.

-- 
Pedro Alves

Re: [patch 1/2] Speed up JIT support

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> This is still showing non-linear growth, but the factors are much
Paul> nicer ;-)

Thanks for doing this.

Paul> +      const int num_longjmp_names
Paul> +	= sizeof (longjmp_names) / sizeof (longjmp_names[0]);

We have an ARRAY_SIZE macro for this (available in libiberty.h, included
everywhere via defs.h).  If you already committed, don't bother changing
it though.

Tom

Re: [patch 2/2] Speed up JIT support

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> 2010-02-03  Paul Pluzhnikov  <ppluzhnikov@google.com>
Paul> 	* breakpoint.c (longjmp_names): New variable.
Paul> 	(struct breakpoint_objfile_data): New type.
Paul> 	(breakpoint_objfile_key): New variable.
Paul> 	(get_breakpoint_objfile_data): New function.
Paul> 	(create_overlay_event_breakpoint): Adjust.
Paul> 	(create_longjmp_master_breakpoint): Adjust.
Paul> 	(create_std_terminate_master_breakpoint): Adjust.
Paul> 	(create_exception_master_breakpoint): Adjust.
Paul> 	(_initialize_breakpoint): Adjust.

From what I can tell, the cache is never invalidated.  But, if
objfile_relocate was called, then I think the cache ought to be
invalidated, or the addresses relocated.

Maybe the cache could point to the symbols themselves.  I think that
would fix the problem and be both safe and efficient.

Paul>  create_exception_master_breakpoint (void)
Paul>  {
Paul>    struct objfile *objfile;
Paul> +  const char *const func_name = "_Unwind_DebugHook";
 
Paul>    ALL_OBJFILES (objfile)
Paul>      {

I don't really know why the other functions loop over ALL_PSPACES but
this one does not.  It is my fault, seeing as I wrote this... but I
still don't understand.

Tom

Re: [patch 2/2] Speed up JIT support

[Paul Pluzhnikov]

On Fri, Feb 4, 2011 at 12:17 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:
>
> From what I can tell, the cache is never invalidated.  But, if
> objfile_relocate was called, then I think the cache ought to be
> invalidated, or the addresses relocated.

Right.

> Maybe the cache could point to the symbols themselves.  I think that
> would fix the problem and be both safe and efficient.

Ah, that also nicely takes care of the problematic use of -1 as invalid
core address -- we *know* that 0 and -1 are *not* valid symbol values.

Two birds with one stone :-)

> Paul>  create_exception_master_breakpoint (void)
> Paul>  {
> Paul>    struct objfile *objfile;
> Paul> +  const char *const func_name = "_Unwind_DebugHook";
>
> Paul>    ALL_OBJFILES (objfile)
> Paul>      {
>
> I don't really know why the other functions loop over ALL_PSPACES but
> this one does not.  It is my fault, seeing as I wrote this... but I
> still don't understand.

I didn't understand that either :-(

Thanks,
-- 
Paul Pluzhnikov

Re: [patch 2/2] Speed up JIT support

[Paul Pluzhnikov]

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

On Fri, Feb 4, 2011 at 12:32 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

>> Maybe the cache could point to the symbols themselves.  I think that
>> would fix the problem and be both safe and efficient.
>
> Ah, that also nicely takes care of the problematic use of -1 as invalid
> core address -- we *know* that 0 and -1 are *not* valid symbol values.

Here is the updated patch (applies on top of the [1/2] cosmetic patch).
Re-tested on linux/x86_64, no regressions.

Thanks,
-- 
Paul Pluzhnikov


2010-02-04  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* breakpoint.c (longjmp_names): New variable.
	(struct breakpoint_objfile_data): New type.
	(breakpoint_objfile_key): New variable.
	(msym_not_found): New variable.
	(msym_not_found_p): New predicate.
	(get_breakpoint_objfile_data): New function.
	(create_overlay_event_breakpoint): Check per-objfile cache for
	symbols first.
	(create_longjmp_master_breakpoint): Likewise.
	(create_std_terminate_master_breakpoint): Likewise.
	(create_exception_master_breakpoint): Likewise.
	(_initialize_breakpoint): Register per-objfile data key.

[-- Attachment #2: gdb-jit-speedup-20110204a.txt --]
[-- Type: text/plain, Size: 8708 bytes --]

--- breakpoint.c.orig	2011-02-04 14:11:34.000000000 -0800
+++ breakpoint.c	2011-02-04 16:11:12.000000000 -0800
@@ -2217,6 +2217,59 @@ create_internal_breakpoint (struct gdbar
   return b;
 }
 
+static const char *const longjmp_names[] =
+  {
+    "longjmp", "_longjmp", "siglongjmp", "_siglongjmp"
+  };
+#define NUM_LONGJMP_NAMES ARRAY_SIZE(longjmp_names)
+
+/* Per-objfile data private to breakpoint.c  */
+struct breakpoint_objfile_data
+{
+  /* minimal symbol for "_ovly_debug_event" (if any)  */
+  struct minimal_symbol *overlay_msym;
+
+  /* minimal symbol(s) for "longjmp", "siglongjmp", etc. (if any)  */
+  struct minimal_symbol *longjmp_msym[NUM_LONGJMP_NAMES];
+
+  /* minimal symbol for "std::terminate()" (if any)  */
+  struct minimal_symbol *terminate_msym;
+
+  /* minimal symbol for "_Unwind_DebugHook" (if any)  */
+  struct minimal_symbol *exception_msym;
+};
+
+static const struct objfile_data *breakpoint_objfile_key;
+
+/* minimal symbol not found sentinel.  */
+static struct minimal_symbol msym_not_found;
+
+static int
+msym_not_found_p (const struct minimal_symbol *msym)
+{
+  return msym == &msym_not_found;
+}
+
+/* Return per-objfile data needed by breakpoint.c.
+   Allocate the data if necessary.  */
+
+static struct breakpoint_objfile_data *
+get_breakpoint_objfile_data (struct objfile *objfile)
+{
+  struct breakpoint_objfile_data *bp_objfile_data;
+
+  bp_objfile_data = objfile_data (objfile, breakpoint_objfile_key);
+  if (bp_objfile_data == NULL)
+    {
+      bp_objfile_data = obstack_alloc (&objfile->objfile_obstack,
+				       sizeof (*bp_objfile_data));
+
+      memset (bp_objfile_data, 0, sizeof (*bp_objfile_data));
+      set_objfile_data (objfile, breakpoint_objfile_key, bp_objfile_data);
+    }
+  return bp_objfile_data;
+}
+
 static void
 create_overlay_event_breakpoint (void)
 {
@@ -2226,14 +2279,30 @@ create_overlay_event_breakpoint (void)
   ALL_OBJFILES (objfile)
     {
       struct breakpoint *b;
-      struct minimal_symbol *m;
+      struct breakpoint_objfile_data *bp_objfile_data;
+      CORE_ADDR addr;
 
-      m = lookup_minimal_symbol_text (func_name, objfile);
-      if (m == NULL)
-        continue;
+      bp_objfile_data = get_breakpoint_objfile_data (objfile);
+
+      if (msym_not_found_p (bp_objfile_data->overlay_msym))
+	continue;
+
+      if (bp_objfile_data->overlay_msym == NULL)
+	{
+	  struct minimal_symbol *m;
+
+	  m = lookup_minimal_symbol_text (func_name, objfile);
+	  if (m == NULL)
+	    {
+	      /* Avoid future lookups in this objfile.  */
+	      bp_objfile_data->overlay_msym = &msym_not_found;
+	      continue;
+	    }
+	  bp_objfile_data->overlay_msym = m;
+	}
 
-      b = create_internal_breakpoint (get_objfile_arch (objfile),
-				      SYMBOL_VALUE_ADDRESS (m),
+      addr = SYMBOL_VALUE_ADDRESS (bp_objfile_data->overlay_msym);
+      b = create_internal_breakpoint (get_objfile_arch (objfile), addr,
                                       bp_overlay_event);
       b->addr_string = xstrdup (func_name);
 
@@ -2267,31 +2336,42 @@ create_longjmp_master_breakpoint (void)
 
     ALL_OBJFILES (objfile)
     {
-      const char *const longjmp_names[]
-	= { "longjmp", "_longjmp", "siglongjmp", "_siglongjmp" };
-      const int num_longjmp_names
-	= sizeof (longjmp_names) / sizeof (longjmp_names[0]);
       int i;
       struct gdbarch *gdbarch;
+      struct breakpoint_objfile_data *bp_objfile_data;
 
       gdbarch = get_objfile_arch (objfile);
       if (!gdbarch_get_longjmp_target_p (gdbarch))
 	continue;
 
-      for (i = 0; i < num_longjmp_names; i++)
+      bp_objfile_data = get_breakpoint_objfile_data (objfile);
+
+      for (i = 0; i < NUM_LONGJMP_NAMES; i++)
 	{
 	  struct breakpoint *b;
-	  struct minimal_symbol *m;
 	  const char *func_name;
+	  CORE_ADDR addr;
 
-	  func_name = longjmp_names[i];
-	  m = lookup_minimal_symbol_text (func_name, objfile);
-	  if (m == NULL)
+	  if (msym_not_found_p (bp_objfile_data->longjmp_msym[i]))
 	    continue;
 
-	  b = create_internal_breakpoint (gdbarch,
-					  SYMBOL_VALUE_ADDRESS (m),
-					  bp_longjmp_master);
+	  func_name = longjmp_names[i];
+	  if (bp_objfile_data->longjmp_msym[i] == NULL)
+	    {
+	      struct minimal_symbol *m;
+
+	      m = lookup_minimal_symbol_text (func_name, objfile);
+	      if (m == NULL)
+		{
+		  /* Prevent future lookups in this objfile.  */
+		  bp_objfile_data->longjmp_msym[i] = &msym_not_found;
+		  continue;
+		}
+	      bp_objfile_data->longjmp_msym[i] = m;
+	    }
+
+	  addr = SYMBOL_VALUE_ADDRESS (bp_objfile_data->longjmp_msym[i]);
+	  b = create_internal_breakpoint (gdbarch, addr, bp_longjmp_master);
 	  b->addr_string = xstrdup (func_name);
 	  b->enable_state = bp_disabled;
 	}
@@ -2307,31 +2387,51 @@ static void
 create_std_terminate_master_breakpoint (void)
 {
   struct program_space *pspace;
-  struct objfile *objfile;
   struct cleanup *old_chain;
   const char *const func_name = "std::terminate()";
 
   old_chain = save_current_program_space ();
 
   ALL_PSPACES (pspace)
+  {
+    struct objfile *objfile;
+    CORE_ADDR addr;
+
+    set_current_program_space (pspace);
+
     ALL_OBJFILES (objfile)
     {
       struct breakpoint *b;
-      struct minimal_symbol *m;
+      struct breakpoint_objfile_data *bp_objfile_data;
 
-      set_current_program_space (pspace);
+      bp_objfile_data = get_breakpoint_objfile_data (objfile);
 
-      m = lookup_minimal_symbol (func_name, NULL, objfile);
-      if (m == NULL || (MSYMBOL_TYPE (m) != mst_text
-			&& MSYMBOL_TYPE (m) != mst_file_text))
-        continue;
+      if (msym_not_found_p (bp_objfile_data->terminate_msym))
+	continue;
+
+      if (bp_objfile_data->terminate_msym == NULL)
+	{
+	  struct minimal_symbol *m;
+
+	  m = lookup_minimal_symbol (func_name, NULL, objfile);
+	  if (m == NULL || (MSYMBOL_TYPE (m) != mst_text
+			    && MSYMBOL_TYPE (m) != mst_file_text))
+	    {
+	      /* Prevent future lookups in this objfile.  */
+	      bp_objfile_data->terminate_msym = &msym_not_found;
+	      continue;
+	    }
+	  bp_objfile_data->terminate_msym = m;
+	}
 
-      b = create_internal_breakpoint (get_objfile_arch (objfile),
-				      SYMBOL_VALUE_ADDRESS (m),
+      addr = SYMBOL_VALUE_ADDRESS (bp_objfile_data->terminate_msym);
+      b = create_internal_breakpoint (get_objfile_arch (objfile), addr,
                                       bp_std_terminate_master);
       b->addr_string = xstrdup (func_name);
       b->enable_state = bp_disabled;
     }
+  }
+
   update_global_location_list (1);
 
   do_cleanups (old_chain);
@@ -2343,24 +2443,42 @@ void
 create_exception_master_breakpoint (void)
 {
   struct objfile *objfile;
+  const char *const func_name = "_Unwind_DebugHook";
 
   ALL_OBJFILES (objfile)
     {
-      struct minimal_symbol *debug_hook;
+      struct breakpoint *b;
+      struct gdbarch *gdbarch;
+      struct breakpoint_objfile_data *bp_objfile_data;
+      CORE_ADDR addr;
+
+      bp_objfile_data = get_breakpoint_objfile_data (objfile);
+
+      if (msym_not_found_p (bp_objfile_data->exception_msym))
+	continue;
+
+      gdbarch = get_objfile_arch (objfile);
 
-      debug_hook = lookup_minimal_symbol ("_Unwind_DebugHook", NULL, objfile);
-      if (debug_hook != NULL)
+      if (bp_objfile_data->exception_msym == NULL)
 	{
-	  struct breakpoint *b;
-	  CORE_ADDR addr = SYMBOL_VALUE_ADDRESS (debug_hook);
-	  struct gdbarch *gdbarch = get_objfile_arch (objfile);
+	  struct minimal_symbol *debug_hook;
 
-	  addr = gdbarch_convert_from_func_ptr_addr (gdbarch, addr,
-						     &current_target);
-	  b = create_internal_breakpoint (gdbarch, addr, bp_exception_master);
-	  b->addr_string = xstrdup ("_Unwind_DebugHook");
-	  b->enable_state = bp_disabled;
+	  debug_hook = lookup_minimal_symbol (func_name, NULL, objfile);
+	  if (debug_hook == NULL)
+	    {
+	      bp_objfile_data->exception_msym = &msym_not_found;
+	      continue;
+	    }
+
+	  bp_objfile_data->exception_msym = debug_hook;
 	}
+
+      addr = SYMBOL_VALUE_ADDRESS (bp_objfile_data->exception_msym);
+      addr = gdbarch_convert_from_func_ptr_addr (gdbarch, addr,
+						 &current_target);
+      b = create_internal_breakpoint (gdbarch, addr, bp_exception_master);
+      b->addr_string = xstrdup (func_name);
+      b->enable_state = bp_disabled;
     }
 
   update_global_location_list (1);
@@ -12071,6 +12189,8 @@ _initialize_breakpoint (void)
   observer_attach_inferior_exit (clear_syscall_counts);
   observer_attach_memory_changed (invalidate_bp_value_on_memory_change);
 
+  breakpoint_objfile_key = register_objfile_data ();
+
   breakpoint_chain = 0;
   /* Don't bother to call set_breakpoint_count.  $bpnum isn't useful
      before a breakpoint is set.  */

Re: [patch 2/2] Speed up JIT support

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> Here is the updated patch (applies on top of the [1/2] cosmetic patch).
Paul> Re-tested on linux/x86_64, no regressions.

Thanks for doing this.

Just some nits, nothing serious.

Paul> +/* Per-objfile data private to breakpoint.c  */

Period at the end of the comment.

Paul> +  /* minimal symbol for "_ovly_debug_event" (if any)  */
Paul> +  struct minimal_symbol *overlay_msym;

Comments should start with an upper case character and end with a
period.  There are a few instances of this.

Paul> +
Paul> +static int
Paul> +msym_not_found_p (const struct minimal_symbol *msym)

This function needs an introductory comment.

This is ok with these things fixed.

Tom

Re: [patch 2/2] Speed up JIT support

[Paul Pluzhnikov]

On Mon, Feb 7, 2011 at 7:31 AM, Tom Tromey <tromey@redhat.com> wrote:

> This is ok with these things fixed.

Thanks. Fixed, retested (no regressions on Linux/x86_64) and committed.

-- 
Paul Pluzhnikov