List:Commits« Previous MessageNext Message »
From:Marc Alff Date:January 28 2011 1:41pm
Subject:bzr commit into mysql-trunk branch (marc.alff:3567) Bug#59740
View as plain text  
#At file:///Users/malff/BZR_TREE/mysql-trunk-bug59740/ based on revid:marc.alff@stripped

 3567 Marc Alff	2011-01-28
      BUG#59740 Performance schema test failures on freebsd
      
      Investigation of bug 59740 shows that selects on
      table performance_schema.events_waits_summary_by_thread_by_event_name
      sometime to not return the expected rows.
      
      While the root cause of this is not identified yet,
      the method table_ews_by_thread_by_event_name::rnd_next()
      is suspected to be the origin of the failure.
      
      The code of this method can benefit from simplifications already
      identified with other tables, see for example
      table_setup_instruments::rnd_next().
      
      This fix implements several improvements for the table scan
      of table performance_schema.events_waits_summary_by_thread_by_event_name.
      In particular:
      - code duplication is reduced
      - code complexity is reduced
      - the number of calls to thread_array[i].m_lock.is_populated()
        is reduced by a factor 200+ (the number of instruments)
        This is implemented by changing the iteration on threads from
        the innermost index (index 3) to the outermost index (index 1).
      
      While is it unclear if calling atomic operations too frequently
      can cause end_optimistic_lock() to fail in ::make_row(),
      which could be a possible root cause for the bug found,
      reducing the number of calls to atomic operations will make the code
      more robust and scalable, so implementing this cleanup is required.
      
      This fix also improves the ortho_iter test, for coverage,
      since it also checks table events_waits_summary_by_thread_by_event_name.

    modified:
      mysql-test/suite/perfschema/r/ortho_iter.result
      mysql-test/suite/perfschema/t/ortho_iter.test
      storage/perfschema/table_ews_by_thread_by_event_name.cc
      storage/perfschema/table_ews_by_thread_by_event_name.h
      storage/perfschema/table_ews_global_by_event_name.h
=== modified file 'mysql-test/suite/perfschema/r/ortho_iter.result'
--- a/mysql-test/suite/perfschema/r/ortho_iter.result	2011-01-26 09:34:10 +0000
+++ b/mysql-test/suite/perfschema/r/ortho_iter.result	2011-01-28 13:40:57 +0000
@@ -154,6 +154,15 @@ status
 Checking table events_waits_summary_global_by_event_name ...
 Warnings:
 Warning	12000	Done
+call check_instrument("wait/synch/");
+instr_name	is_wait	is_stage	is_statement
+wait/synch/	1	0	0
+status
+Checking table events_waits_summary_by_thread_by_event_name ...
+status
+Checking table events_waits_summary_global_by_event_name ...
+Warnings:
+Warning	12000	Done
 call check_instrument("wait/io/file/");
 instr_name	is_wait	is_stage	is_statement
 wait/io/file/	1	0	0
@@ -181,6 +190,15 @@ status
 Checking table events_waits_summary_global_by_event_name ...
 Warnings:
 Warning	12000	Done
+call check_instrument("wait/io/");
+instr_name	is_wait	is_stage	is_statement
+wait/io/	1	0	0
+status
+Checking table events_waits_summary_by_thread_by_event_name ...
+status
+Checking table events_waits_summary_global_by_event_name ...
+Warnings:
+Warning	12000	Done
 call check_instrument("wait/lock/table/");
 instr_name	is_wait	is_stage	is_statement
 wait/lock/table/	1	0	0
@@ -190,6 +208,24 @@ status
 Checking table events_waits_summary_global_by_event_name ...
 Warnings:
 Warning	12000	Done
+call check_instrument("wait/lock/");
+instr_name	is_wait	is_stage	is_statement
+wait/lock/	1	0	0
+status
+Checking table events_waits_summary_by_thread_by_event_name ...
+status
+Checking table events_waits_summary_global_by_event_name ...
+Warnings:
+Warning	12000	Done
+call check_instrument("wait/");
+instr_name	is_wait	is_stage	is_statement
+wait/	1	0	0
+status
+Checking table events_waits_summary_by_thread_by_event_name ...
+status
+Checking table events_waits_summary_global_by_event_name ...
+Warnings:
+Warning	12000	Done
 call check_instrument("stage/");
 instr_name	is_wait	is_stage	is_statement
 stage/	0	1	0
@@ -217,4 +253,13 @@ status
 Checking table events_waits_summary_global_by_event_name ...
 Warnings:
 Warning	12000	Done
+call check_instrument("statement/");
+instr_name	is_wait	is_stage	is_statement
+statement/	0	0	1
+status
+Checking table events_waits_summary_by_thread_by_event_name ...
+status
+Checking table events_waits_summary_global_by_event_name ...
+Warnings:
+Warning	12000	Done
 drop procedure check_instrument;

=== modified file 'mysql-test/suite/perfschema/t/ortho_iter.test'
--- a/mysql-test/suite/perfschema/t/ortho_iter.test	2011-01-26 09:34:10 +0000
+++ b/mysql-test/suite/perfschema/t/ortho_iter.test	2011-01-28 13:40:57 +0000
@@ -142,13 +142,18 @@ show status like "performance_schema%";
 call check_instrument("wait/synch/mutex/");
 call check_instrument("wait/synch/rwlock/");
 call check_instrument("wait/synch/cond/");
+call check_instrument("wait/synch/");
 call check_instrument("wait/io/file/");
 call check_instrument("wait/io/net/");
 call check_instrument("wait/io/table/");
+call check_instrument("wait/io/");
 call check_instrument("wait/lock/table/");
+call check_instrument("wait/lock/");
+call check_instrument("wait/");
 call check_instrument("stage/");
 call check_instrument("statement/com/");
 call check_instrument("statement/sql/");
+call check_instrument("statement/");
 
 drop procedure check_instrument;
 

=== modified file 'storage/perfschema/table_ews_by_thread_by_event_name.cc'
--- a/storage/perfschema/table_ews_by_thread_by_event_name.cc	2011-01-07 16:20:19 +0000
+++ b/storage/perfschema/table_ews_by_thread_by_event_name.cc	2011-01-28 13:40:57 +0000
@@ -115,120 +115,54 @@ void table_ews_by_thread_by_event_name::
 int table_ews_by_thread_by_event_name::rnd_next(void)
 {
   PFS_thread *thread;
-  PFS_mutex_class *mutex_class;
-  PFS_rwlock_class *rwlock_class;
-  PFS_cond_class *cond_class;
-  PFS_file_class *file_class;
-  PFS_instr_class *table_class;
+  PFS_instr_class *instr_class;
 
   for (m_pos.set_at(&m_next_pos);
-       m_pos.has_more_view();
-       m_pos.next_view())
+       m_pos.has_more_thread();
+       m_pos.next_thread())
   {
-    switch (m_pos.m_index_1)
-    {
-    case pos_ews_by_thread_by_event_name::VIEW_MUTEX:
-      do
-      {
-        mutex_class= find_mutex_class(m_pos.m_index_2);
-        if (mutex_class)
-        {
-          for ( ; m_pos.has_more_thread(); m_pos.next_thread())
-          {
-            thread= &thread_array[m_pos.m_index_3];
-            if (thread->m_lock.is_populated())
-            {
-              make_row(thread, mutex_class);
-              m_next_pos.set_after(&m_pos);
-              return 0;
-            }
-          }
-          m_pos.next_instrument();
-        }
-      } while (mutex_class != NULL);
-      break;
-    case pos_ews_by_thread_by_event_name::VIEW_RWLOCK:
-      do
-      {
-        rwlock_class= find_rwlock_class(m_pos.m_index_2);
-        if (rwlock_class)
-        {
-          for ( ; m_pos.has_more_thread(); m_pos.next_thread())
-          {
-            thread= &thread_array[m_pos.m_index_3];
-            if (thread->m_lock.is_populated())
-            {
-              make_row(thread, rwlock_class);
-              m_next_pos.set_after(&m_pos);
-              return 0;
-            }
-          }
-          m_pos.next_instrument();
-        }
-      } while (rwlock_class != NULL);
-      break;
-    case pos_ews_by_thread_by_event_name::VIEW_COND:
-      do
-      {
-        cond_class= find_cond_class(m_pos.m_index_2);
-        if (cond_class)
-        {
-          for ( ; m_pos.has_more_thread(); m_pos.next_thread())
-          {
-            thread= &thread_array[m_pos.m_index_3];
-            if (thread->m_lock.is_populated())
-            {
-              make_row(thread, cond_class);
-              m_next_pos.set_after(&m_pos);
-              return 0;
-            }
-          }
-          m_pos.next_instrument();
-        }
-      } while (cond_class != NULL);
-      break;
-    case pos_ews_by_thread_by_event_name::VIEW_FILE:
-      do
+    thread= &thread_array[m_pos.m_index_1];
+
+    /*
+      Important note: the thread scan is the outer loop (index 1),
+      to minimize the number of calls to atomic operations.
+    */
+    if (thread->m_lock.is_populated())
+    {
+      for ( ;
+           m_pos.has_more_view();
+           m_pos.next_view())
       {
-        file_class= find_file_class(m_pos.m_index_2);
-        if (file_class)
+        switch (m_pos.m_index_2)
         {
-          for ( ; m_pos.has_more_thread(); m_pos.next_thread())
-          {
-            thread= &thread_array[m_pos.m_index_3];
-            if (thread->m_lock.is_populated())
-            {
-              make_row(thread, file_class);
-              m_next_pos.set_after(&m_pos);
-              return 0;
-            }
-          }
-          m_pos.next_instrument();
+        case pos_ews_by_thread_by_event_name::VIEW_MUTEX:
+          instr_class= find_mutex_class(m_pos.m_index_3);
+          break;
+        case pos_ews_by_thread_by_event_name::VIEW_RWLOCK:
+          instr_class= find_rwlock_class(m_pos.m_index_3);
+          break;
+        case pos_ews_by_thread_by_event_name::VIEW_COND:
+          instr_class= find_cond_class(m_pos.m_index_3);
+          break;
+        case pos_ews_by_thread_by_event_name::VIEW_FILE:
+          instr_class= find_file_class(m_pos.m_index_3);
+          break;
+        case pos_ews_by_thread_by_event_name::VIEW_TABLE:
+          instr_class= find_table_class(m_pos.m_index_3);
+          break;
+        default:
+          DBUG_ASSERT(false);
+          instr_class= NULL;
+          break;
         }
-      } while (file_class != NULL);
-      break;
-    case pos_ews_by_thread_by_event_name::VIEW_TABLE:
-      do
-      {
-        table_class= find_table_class(m_pos.m_index_2);
-        if (table_class)
+
+        if (instr_class != NULL)
         {
-          for ( ; m_pos.has_more_thread(); m_pos.next_thread())
-          {
-            thread= &thread_array[m_pos.m_index_3];
-            if (thread->m_lock.is_populated())
-            {
-              make_row(thread, table_class);
-              m_next_pos.set_after(&m_pos);
-              return 0;
-            }
-          }
-          m_pos.next_instrument();
+          make_row(thread, instr_class);
+          m_next_pos.set_after(&m_pos);
+          return 0;
         }
-      } while (table_class != NULL);
-      break;
-    default:
-      break;
+      }
     }
   }
 
@@ -239,63 +173,42 @@ int
 table_ews_by_thread_by_event_name::rnd_pos(const void *pos)
 {
   PFS_thread *thread;
-  PFS_mutex_class *mutex_class;
-  PFS_rwlock_class *rwlock_class;
-  PFS_cond_class *cond_class;
-  PFS_file_class *file_class;
-  PFS_instr_class *table_class;
+  PFS_instr_class *instr_class;
 
   set_position(pos);
-  DBUG_ASSERT(m_pos.m_index_3 < thread_max);
+  DBUG_ASSERT(m_pos.m_index_1 < thread_max);
 
-  thread= &thread_array[m_pos.m_index_3];
+  thread= &thread_array[m_pos.m_index_1];
   if (! thread->m_lock.is_populated())
     return HA_ERR_RECORD_DELETED;
 
-  switch (m_pos.m_index_1)
+  switch (m_pos.m_index_2)
   {
   case pos_ews_by_thread_by_event_name::VIEW_MUTEX:
-    mutex_class= find_mutex_class(m_pos.m_index_2);
-    if (mutex_class)
-    {
-      make_row(thread, mutex_class);
-      return 0;
-    }
+    instr_class= find_mutex_class(m_pos.m_index_3);
     break;
   case pos_ews_by_thread_by_event_name::VIEW_RWLOCK:
-    rwlock_class= find_rwlock_class(m_pos.m_index_2);
-    if (rwlock_class)
-    {
-      make_row(thread, rwlock_class);
-      return 0;
-    }
+    instr_class= find_rwlock_class(m_pos.m_index_3);
     break;
   case pos_ews_by_thread_by_event_name::VIEW_COND:
-    cond_class= find_cond_class(m_pos.m_index_2);
-    if (cond_class)
-    {
-      make_row(thread, cond_class);
-      return 0;
-    }
+    instr_class= find_cond_class(m_pos.m_index_3);
     break;
   case pos_ews_by_thread_by_event_name::VIEW_FILE:
-    file_class= find_file_class(m_pos.m_index_2);
-    if (file_class)
-    {
-      make_row(thread, file_class);
-      return 0;
-    }
+    instr_class= find_file_class(m_pos.m_index_3);
     break;
   case pos_ews_by_thread_by_event_name::VIEW_TABLE:
-    table_class= find_table_class(m_pos.m_index_2);
-    if (table_class)
-    {
-      make_row(thread, table_class);
-      return 0;
-    }
+    instr_class= find_table_class(m_pos.m_index_3);
     break;
+  default:
+    DBUG_ASSERT(false);
+    instr_class= NULL;
   }
 
+  if (instr_class)
+  {
+    make_row(thread, instr_class);
+    return 0;
+  }
   return HA_ERR_RECORD_DELETED;
 }
 

=== modified file 'storage/perfschema/table_ews_by_thread_by_event_name.h'
--- a/storage/perfschema/table_ews_by_thread_by_event_name.h	2011-01-07 16:20:19 +0000
+++ b/storage/perfschema/table_ews_by_thread_by_event_name.h	2011-01-28 13:40:57 +0000
@@ -49,46 +49,41 @@ struct row_ews_by_thread_by_event_name
 /**
   Position of a cursor on
   PERFORMANCE_SCHEMA.EVENTS_WAITS_SUMMARY_BY_THREAD_BY_EVENT_NAME.
-  Index 1 on instrument view
-  Index 2 on instrument class (1 based)
-  Index 3 on thread (0 based)
+  Index 1 on thread (0 based)
+  Index 2 on instrument view
+  Index 3 on instrument class (1 based)
 */
 struct pos_ews_by_thread_by_event_name
 : public PFS_triple_index, public PFS_instrument_view_constants
 {
   pos_ews_by_thread_by_event_name()
-    : PFS_triple_index(FIRST_VIEW, 1, 0)
+    : PFS_triple_index(0, FIRST_VIEW, 1)
   {}
 
   inline void reset(void)
   {
-    m_index_1= FIRST_VIEW;
-    m_index_2= 1;
-    m_index_3= 0;
+    m_index_1= 0;
+    m_index_2= FIRST_VIEW;
+    m_index_3= 1;
   }
 
-  inline bool has_more_view(void)
-  { return (m_index_1 <= LAST_VIEW); }
-
   inline bool has_more_thread(void)
-  { return (m_index_3 < thread_max); }
+  { return (m_index_1 < thread_max); }
 
-  inline void next_view(void)
+  inline void next_thread(void)
   {
     m_index_1++;
-    m_index_2= 1;
-    m_index_3= 0;
+    m_index_2= FIRST_VIEW;
+    m_index_3= 1;
   }
 
-  inline void next_instrument(void)
-  {
-    m_index_2++;
-    m_index_3= 0;
-  }
+  inline bool has_more_view(void)
+  { return (m_index_2 <= LAST_VIEW); }
 
-  inline void next_thread(void)
+  inline void next_view(void)
   {
-    m_index_3++;
+    m_index_2++;
+    m_index_3= 1;
   }
 };
 

=== modified file 'storage/perfschema/table_ews_global_by_event_name.h'
--- a/storage/perfschema/table_ews_global_by_event_name.h	2011-01-07 16:20:19 +0000
+++ b/storage/perfschema/table_ews_global_by_event_name.h	2011-01-28 13:40:57 +0000
@@ -71,11 +71,6 @@ struct pos_ews_global_by_event_name
     m_index_1++;
     m_index_2= 1;
   }
-
-  inline void next_instrument(void)
-  {
-    m_index_2++;
-  }
 };
 
 /** Table PERFORMANCE_SCHEMA.EVENTS_WAITS_SUMMARY_GLOBAL_BY_EVENT_NAME. */


Attachment: [text/bzr-bundle] bzr/marc.alff@oracle.com-20110128134057-i3px1zc1e6t8dgat.bundle
Thread
bzr commit into mysql-trunk branch (marc.alff:3567) Bug#59740Marc Alff28 Jan