Re: performance of multithreading gets gradually worse under gdb

Re: performance of multithreading gets gradually worse under gdb

[Pedro Alves]

[Adding gdb-patches@]

On Friday 04 February 2011 15:25:54, Tom Tromey wrote:
> Pedro> Yeah.  And only clear current_regcache_ptid if it was deleted in the
> Pedro> first place; and only reinit the frame cache if we deleted the
> Pedro> regcache of inferior_ptid ?  Like the patch below.
> 
> Yeah, this looks reasonable to me.

Okay, thanks, I've applied it.

> One somewhat distressing thing is that my patch did not cause any
> regressions.

Indeed.

-- 
Pedro Alves

Re: performance of multithreading gets gradually worse under gdb

[Pedro Alves]

On Friday 04 February 2011 15:55:51, Pedro Alves wrote:
> [Adding gdb-patches@]

...

> Okay, thanks, I've applied it.

"it" being...

-- 
Pedro Alves

2011-02-04  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* regcache.c (registers_changed_ptid): Don't explictly always
	clear `current_regcache'.  Only clear current_thread_ptid and
	current_thread_arch when PTID matches.  Only reinit the frame
	cache if PTID matches the current inferior_ptid.  Move alloca(0)
	call to ...
	(registers_changed): ... here.

---
 gdb/regcache.c |   28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

Index: src/gdb/regcache.c
===================================================================
--- src.orig/gdb/regcache.c	2011-02-01 15:27:37.000000000 +0000
+++ src/gdb/regcache.c	2011-02-04 11:56:49.273328004 +0000
@@ -530,6 +530,7 @@ void
 registers_changed_ptid (ptid_t ptid)
 {
   struct regcache_list *list, **list_link;
+  int wildcard = ptid_equal (ptid, minus_one_ptid);
 
   list = current_regcache;
   list_link = &current_regcache;
@@ -550,13 +551,24 @@ registers_changed_ptid (ptid_t ptid)
       list = *list_link;
     }
 
-  current_regcache = NULL;
+  if (wildcard || ptid_equal (ptid, current_thread_ptid))
+    {
+      current_thread_ptid = null_ptid;
+      current_thread_arch = NULL;
+    }
 
-  current_thread_ptid = null_ptid;
-  current_thread_arch = NULL;
+  if (wildcard || ptid_equal (ptid, inferior_ptid))
+    {
+      /* We just deleted the regcache of the current thread.  Need to
+	 forget about any frames we have cached, too.  */
+      reinit_frame_cache ();
+    }
+}
 
-  /* Need to forget about any frames we have cached, too.  */
-  reinit_frame_cache ();
+void
+registers_changed (void)
+{
+  registers_changed_ptid (minus_one_ptid);
 
   /* Force cleanup of any alloca areas if using C alloca instead of
      a builtin alloca.  This particular call is used to clean up
@@ -567,12 +579,6 @@ registers_changed_ptid (ptid_t ptid)
 }
 
 void
-registers_changed (void)
-{
-  registers_changed_ptid (minus_one_ptid);
-}
-
-void
 regcache_raw_read (struct regcache *regcache, int regnum, gdb_byte *buf)
 {
   gdb_assert (regcache != NULL && buf != NULL);

[patch] Multi-architecture debugging regression (Re: performance of multithreading gets gradually worse under gdb)

[Ulrich Weigand]

Pedro Alves wrote:

> 2011-02-04  Pedro Alves  <pedro@codesourcery.com>
> 
> 	gdb/
> 	* regcache.c (registers_changed_ptid): Don't explictly always
> 	clear `current_regcache'.  Only clear current_thread_ptid and
> 	current_thread_arch when PTID matches.  Only reinit the frame
> 	cache if PTID matches the current inferior_ptid.  Move alloca(0)
> 	call to ...
> 	(registers_changed): ... here.

It seems this patch broke multi-architecture debugging on Cell/B.E. since
the cached thread architecture is sometimes not invalidated when it should
be.  This happens when resume is called with a PTID that refers to all
threads of a process.

The problem is that your patch specifically handles PTIDs that refer to
one particular thread, and also handles a wildcard PTID that refers to
all threads of all processes.  But it does not handle PTIDs that refer
to all threads of one single process.

I think the code should simply use ptid_match here (as is already done
elsewhere, even in this same function).  This fixes the problem on
Cell/B.E. for me ...

Tested on powerpc-linux (Cell/B.E.).
Does this look OK to you?

Bye,
Ulrich


ChangeLog:

	* regcache.c (registers_changed_ptid): Invalidate thread architecture
	and frame caches if PTID refers to all threads of a process.


Index: gdb/regcache.c
===================================================================
RCS file: /cvs/src/src/gdb/regcache.c,v
retrieving revision 1.194
diff -u -p -r1.194 regcache.c
--- gdb/regcache.c	11 Oct 2011 18:35:25 -0000	1.194
+++ gdb/regcache.c	24 Oct 2011 11:02:46 -0000
@@ -548,7 +548,6 @@ void
 registers_changed_ptid (ptid_t ptid)
 {
   struct regcache_list *list, **list_link;
-  int wildcard = ptid_equal (ptid, minus_one_ptid);
 
   list = current_regcache;
   list_link = &current_regcache;
@@ -569,13 +568,13 @@ registers_changed_ptid (ptid_t ptid)
       list = *list_link;
     }
 
-  if (wildcard || ptid_equal (ptid, current_thread_ptid))
+  if (ptid_match (current_thread_ptid, ptid))
     {
       current_thread_ptid = null_ptid;
       current_thread_arch = NULL;
     }
 
-  if (wildcard || ptid_equal (ptid, inferior_ptid))
+  if (ptid_match (inferior_ptid, ptid))
     {
       /* We just deleted the regcache of the current thread.  Need to
 	 forget about any frames we have cached, too.  */

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] Multi-architecture debugging regression (Re: performance of multithreading gets gradually worse under gdb)

[Pedro Alves]

On Wednesday 26 October 2011 18:14:11, Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
> > 2011-02-04  Pedro Alves  <pedro@codesourcery.com>
> > 
> > 	gdb/
> > 	* regcache.c (registers_changed_ptid): Don't explictly always
> > 	clear `current_regcache'.  Only clear current_thread_ptid and
> > 	current_thread_arch when PTID matches.  Only reinit the frame
> > 	cache if PTID matches the current inferior_ptid.  Move alloca(0)
> > 	call to ...
> > 	(registers_changed): ... here.
> 
> It seems this patch broke multi-architecture debugging on Cell/B.E. since
> the cached thread architecture is sometimes not invalidated when it should
> be.  This happens when resume is called with a PTID that refers to all
> threads of a process.
> 
> The problem is that your patch specifically handles PTIDs that refer to
> one particular thread, and also handles a wildcard PTID that refers to
> all threads of all processes.  But it does not handle PTIDs that refer
> to all threads of one single process.
> 
> I think the code should simply use ptid_match here (as is already done
> elsewhere, even in this same function).  This fixes the problem on
> Cell/B.E. for me ...
> 
> Tested on powerpc-linux (Cell/B.E.).
> Does this look OK to you?

Yes, thanks.

-- 
Pedro Alves

Re: [patch] Multi-architecture debugging regression (Re: performance of multithreading gets gradually worse under gdb)

[Ulrich Weigand]

Pedro Alves wrote:
> On Wednesday 26 October 2011 18:14:11, Ulrich Weigand wrote:
> > I think the code should simply use ptid_match here (as is already done
> > elsewhere, even in this same function).  This fixes the problem on
> > Cell/B.E. for me ...
> > 
> > Tested on powerpc-linux (Cell/B.E.).
> > Does this look OK to you?
> 
> Yes, thanks.

OK, checked in.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: performance of multithreading gets gradually worse under gdb

[Tom Tromey]

Tom> Yeah, this looks reasonable to me.

Pedro> Okay, thanks, I've applied it.

Thanks

Markus -- any way you could try gdb CVS HEAD to see if it fixes your
problem?  If it doesn't, then there are more bugs to find...

Tom

Re: performance of multithreading gets gradually worse under gdb

[Markus Alber]

 On Fri, 04 Feb 2011 10:01:57 -0700, Tom Tromey wrote:

> Markus -- any way you could try gdb CVS HEAD to see if it fixes your
> problem?  If it doesn't, then there are more bugs to find...
>
> Tom

 Will try on Monday, thanks.

Re: performance of multithreading gets gradually worse under gdb

[Markus Alber]

 On Fri, 04 Feb 2011 10:01:57 -0700, Tom Tromey wrote:
> Tom> Yeah, this looks reasonable to me.
>
> Pedro> Okay, thanks, I've applied it.
>
> Thanks
>
> Markus -- any way you could try gdb CVS HEAD to see if it fixes your
> problem?  If it doesn't, then there are more bugs to find...
>
> Tom

 Hi, I did as told and it does indeed make the situation a lot better.
 The application runs to the end (for the first time!) and it does not
 slow down.

 Thanks for that!

 The application is a lot slower though when run in gdb, than 
 stand-alone.
 I think the reason is this. During each iteration, it branches into the
 worker threads and then comes back to some part of the computation 
 which
 is performed only by one CPU. With 12 cores, the time per iteration is
 essentially spent in there. It is here that gdb competes with the 
 application,
 because both seem to be running on the same core. The average load 
 drops from
 66% to about 20% when run in gdb.

 Just for your information/in case you are interested.

 I did an oprofile as well, and here is what it says:

 samples  %        image name               app name                 
 symbol name
 1113     19.3464  libthread_db-1.0.so      libthread_db-1.0.so      
 _td_locate_field
 1054     18.3209  libthread_db-1.0.so      libthread_db-1.0.so      
 td_thr_get_info
 847      14.7228  libthread_db-1.0.so      libthread_db-1.0.so      
 td_ta_event_getmsg
 804      13.9753  libthread_db-1.0.so      libthread_db-1.0.so      
 _td_fetch_value_local
 568       9.8731  libthread_db-1.0.so      libthread_db-1.0.so      
 td_ta_map_lwp2thr
 519       9.0214  libthread_db-1.0.so      libthread_db-1.0.so      
 _td_fetch_value
 484       8.4130  libthread_db-1.0.so      libthread_db-1.0.so      
 __td_ta_lookup_th_unique
 169       2.9376  libthread_db-1.0.so      libthread_db-1.0.so      
 _td_store_value
 167       2.9028  libthread_db-1.0.so      libthread_db-1.0.so      
 td_thr_event_enable
 3         0.0521  iconv                    iconv                    
 /usr/bin/iconv
 3         0.0521  libstreamanalyzer.so.0.7.2 libstreamanalyzer.so.0.7.2 
 /usr/lib64/libstreamanalyzer.so.0.7.2
 2         0.0348  [vdso] (tgid:2539 
 range:0x7fffe81ff000-0x7fffe8200000) auditd                   [vdso] 
 (tgid:2539 range:0x7fffe81
 200000)
 2         0.0348  libpcre.so.0.0.1         libpcre.so.0.0.1         
 /lib64/libpcre.so.0.0.1
 2         0.0348  ophelp                   ophelp                   
 /usr/bin/ophelp
 1         0.0174  [vdso] (tgid:7680 
 range:0x7fff267c6000-0x7fff267c7000) kdeinit4                 [vdso] 
 (tgid:7680 range:0x7fff267
 7c7000)
 1         0.0174  [vdso] (tgid:8945 
 range:0x7fffbbbff000-0x7fffbbc00000) gconfd-2                 [vdso] 
 (tgid:8945 range:0x7fffbbb
 c00000)
 1         0.0174  auditd                   auditd                   
 /sbin/auditd
 1         0.0174  basename                 basename                 
 /bin/basename
 1         0.0174  kded_networkstatus.so    kded_networkstatus.so    
 /usr/lib64/kde4/kded_networkstatus.so
 1         0.0174  libXau.so.6.0.0          libXau.so.6.0.0          
 /usr/lib64/libXau.so.6.0.0
 1         0.0174  libXi.so.6.1.0           libXi.so.6.1.0           
 /usr/lib64/libXi.so.6.1.0
 1         0.0174  libkdeinit4_kglobalaccel.so 
 libkdeinit4_kglobalaccel.so /usr/lib64/libkdeinit4_kglobalaccel.so
 1         0.0174  libstreams.so.0.7.2      libstreams.so.0.7.2      
 /usr/lib64/libstreams.so.0.7.2
 1         0.0174  libthread_db-1.0.so      libthread_db-1.0.so      
 __do_global_ctors_aux
 1         0.0174  pickup                   pickup                   
 /usr/lib/postfix/pickup
 1         0.0174  plasma_applet_icon.so    plasma_applet_icon.so    
 /usr/lib64/kde4/plasma_applet_icon.so
 1         0.0174  qmgr                     qmgr                     
 /usr/lib/postfix/qmgr
 1         0.0174  rpcbind                  rpcbind                  
 /sbin/rpcbind
 1         0.0174  sleep                    sleep                    
 /bin/sleep
 1         0.0174  touch                    touch                    
 /bin/touch

 If you want to have it in more detail, please tell me what options 
 you'd like to have engaged.

 regards,

 Markus