Bug#17766582 PERFORMANCE SCHEMA OVERHEAD IN PFS_LOCK  

This fix is a general cleanup for code involving atomic operations in the
performance schema, to reduce overhead and improve code clarity.

Changes implemented:

1)

Removed 'volatile' in the code.
The C 'volatile' keyword has nothing to do with atomic operations,
and is confusing.

This is a code cleanup.

2)

Added missing PFS_cacheline_uint32 to atomic counters,
to enforce no false sharing happens.

This is a performance improvement.

3)

Modified optimistic locks, for clarity.

Pattern before:
  pfs_lock lock;
  m_lock.begin_optimistic_lock(&lock);
  m_lock.end_optimistic_lock(&lock);

Pattern after:
  pfs_optimistic_state lock;
  m_lock.begin_optimistic_lock(&lock);
  m_lock.end_optimistic_lock(&lock);

The new type, pfs_optimistic_state, better reflects that a state information
is used in a begin / end section.
This provides better typing, for type safety.

Adjusted all the code accordingly.

4)

Modified pfs_lock allocation, for performances.

Pattern before:
  m_lock.is_free();
  m_lock.free_to_dirty();
  m_lock.dirty_to_allocated();
  total: 0+1+2 = 3 atomic operations

Note that free_to_dirty() could fail even for free locks,
because the CAS can use an old version number,
making the code attempt again another record.

Pattern after:
  pfs_dirty_state dirty_state;
  m_lock.free_to_dirty(& dirty_state);
  m_lock.dirty_to_allocated(& dirty_state);
  total: 2+1 = 3 atomic operations.

Now the code is garanteed to detect free records,
because free_to_dirty() does an atomic load then a CAS.

Adjusted all the code accordingly.

5)

Modified pfs_lock deallocation, for performances.

Pattern before:
  m_lock.allocated_to_free();
  Total 2 atomic operations.

Pattern after:
  m_lock.allocated_to_free();
  Total 1 atomic operation.

6)

Modified record updates, for performances.

Pattern before:
  m_lock.allocated_to_dirty();
  m_lock.dirty_to_allocated();
  Total 4 atomic operations.

Pattern after:
  pfs_dirty_state dirty_state;
  m_lock.allocated_to_dirty(& dirty_state);
  m_lock.dirty_to_allocated(& dirty_state);
  Total 2 atomic operations.

Adjusted all the code accordingly.

7)

Modified record peek, for reliability.

Pattern before:
  m_lock.is_populated()
  used a dirty read, causing spurious missing records

Pattern after:
  m_lock.is_populated()
  uses an atomic load, for correctness.

This change is expected to resolve spurious test failures,
where some records in performance schema tables are sometime missing,
os some statistics (when computed on the fly) are sometime under evaluated.

Author: marcalff

storage/perfschema/pfs.cc

@@ -2169,6 +2169,7 @@ pfs_get_thread_v1(void)
 */
 void pfs_set_thread_user_v1(const char *user, int user_len)
 {
+  pfs_dirty_state dirty_state;
   PFS_thread *pfs= my_pthread_get_THR_PFS();
 
   DBUG_ASSERT((user != NULL) || (user_len == 0));
@@ -2180,7 +2181,7 @@ void pfs_set_thread_user_v1(const char *user, int user_len)
 
   aggregate_thread(pfs, pfs->m_account, pfs->m_user, pfs->m_host);
 
-  pfs->m_session_lock.allocated_to_dirty();
+  pfs->m_session_lock.allocated_to_dirty(& dirty_state);
 
   clear_thread_account(pfs);
 
@@ -2213,7 +2214,7 @@ void pfs_set_thread_user_v1(const char *user, int user_len)
 
   pfs->set_enabled(enabled);
 
-  pfs->m_session_lock.dirty_to_allocated();
+  pfs->m_session_lock.dirty_to_allocated(& dirty_state);
 }
 
 /**
@@ -2223,6 +2224,7 @@ void pfs_set_thread_user_v1(const char *user, int user_len)
 void pfs_set_thread_account_v1(const char *user, int user_len,
                                const char *host, int host_len)
 {
+  pfs_dirty_state dirty_state;
   PFS_thread *pfs= my_pthread_get_THR_PFS();
 
   DBUG_ASSERT((user != NULL) || (user_len == 0));
@@ -2235,7 +2237,7 @@ void pfs_set_thread_account_v1(const char *user, int user_len,
   if (unlikely(pfs == NULL))
     return;
 
-  pfs->m_session_lock.allocated_to_dirty();
+  pfs->m_session_lock.allocated_to_dirty(& dirty_state);
 
   clear_thread_account(pfs);
 
@@ -2271,7 +2273,7 @@ void pfs_set_thread_account_v1(const char *user, int user_len,
   }
   pfs->set_enabled(enabled);
 
-  pfs->m_session_lock.dirty_to_allocated();
+  pfs->m_session_lock.dirty_to_allocated(& dirty_state);
 }
 
 /**
@@ -2288,11 +2290,12 @@ void pfs_set_thread_db_v1(const char* db, int db_len)
 
   if (likely(pfs != NULL))
   {
-    pfs->m_stmt_lock.allocated_to_dirty();
+    pfs_dirty_state dirty_state;
+    pfs->m_stmt_lock.allocated_to_dirty(& dirty_state);
     if (db_len > 0)
       memcpy(pfs->m_dbname, db, db_len);
     pfs->m_dbname_length= db_len;
-    pfs->m_stmt_lock.dirty_to_allocated();
+    pfs->m_stmt_lock.dirty_to_allocated(& dirty_state);
   }
 }
 
@@ -2342,6 +2345,7 @@ void pfs_set_thread_state_v1(const char* state)
 */
 void pfs_set_thread_info_v1(const char* info, uint info_len)
 {
+  pfs_dirty_state dirty_state;
   PFS_thread *pfs= my_pthread_get_THR_PFS();
 
   DBUG_ASSERT((info != NULL) || (info_len == 0));
@@ -2353,16 +2357,16 @@ void pfs_set_thread_info_v1(const char* info, uint info_len)
       if (info_len > sizeof(pfs->m_processlist_info))
         info_len= sizeof(pfs->m_processlist_info);
 
-      pfs->m_stmt_lock.allocated_to_dirty();
+      pfs->m_stmt_lock.allocated_to_dirty(& dirty_state);
       memcpy(pfs->m_processlist_info, info, info_len);
       pfs->m_processlist_info_length= info_len;
-      pfs->m_stmt_lock.dirty_to_allocated();
+      pfs->m_stmt_lock.dirty_to_allocated(& dirty_state);
     }
     else
     {
-      pfs->m_stmt_lock.allocated_to_dirty();
+      pfs->m_stmt_lock.allocated_to_dirty(& dirty_state);
       pfs->m_processlist_info_length= 0;
-      pfs->m_stmt_lock.dirty_to_allocated();
+      pfs->m_stmt_lock.dirty_to_allocated(& dirty_state);
     }
   }
 }
@@ -5889,16 +5893,17 @@ int pfs_set_thread_connect_attrs_v1(const char *buffer, uint length,
 
   if (likely(thd != NULL) && session_connect_attrs_size_per_thread > 0)
   {
+    pfs_dirty_state dirty_state;
     const CHARSET_INFO *cs = static_cast<const CHARSET_INFO *> (from_cs);
 
     /* copy from the input buffer as much as we can fit */
     uint copy_size= (uint)(length < session_connect_attrs_size_per_thread ?
                            length : session_connect_attrs_size_per_thread);
-    thd->m_session_lock.allocated_to_dirty();
+    thd->m_session_lock.allocated_to_dirty(& dirty_state);
     memcpy(thd->m_session_connect_attrs, buffer, copy_size);
     thd->m_session_connect_attrs_length= copy_size;
     thd->m_session_connect_attrs_cs_number= cs->number;
-    thd->m_session_lock.dirty_to_allocated();
+    thd->m_session_lock.dirty_to_allocated(& dirty_state);
 
     if (copy_size == length)
       return 0;

storage/perfschema/pfs_account.cc

@@ -260,6 +260,7 @@ find_or_create_account(PFS_thread *thread,
   const uint retry_max= 3;
   uint index;
   uint attempts= 0;
+  pfs_dirty_state dirty_state;
 
 search:
   entry= reinterpret_cast<PFS_account**>
@@ -286,72 +287,69 @@ find_or_create_account(PFS_thread *thread,
     index= PFS_atomic::add_u32(& monotonic.m_u32, 1) % account_max;
     pfs= account_array + index;
 
-    if (pfs->m_lock.is_free())
+    if (pfs->m_lock.free_to_dirty(& dirty_state))
     {
-      if (pfs->m_lock.free_to_dirty())
+      pfs->m_key= key;
+      if (username_length > 0)
+        pfs->m_username= &pfs->m_key.m_hash_key[0];
+      else
+        pfs->m_username= NULL;
+      pfs->m_username_length= username_length;
+
+      if (hostname_length > 0)
+        pfs->m_hostname= &pfs->m_key.m_hash_key[username_length + 1];
+      else
+        pfs->m_hostname= NULL;
+      pfs->m_hostname_length= hostname_length;
+
+      pfs->m_user= find_or_create_user(thread, username, username_length);
+      pfs->m_host= find_or_create_host(thread, hostname, hostname_length);
+
+      pfs->init_refcount();
+      pfs->reset_stats();
+      pfs->m_disconnected_count= 0;
+
+      if (username_length > 0 && hostname_length > 0)
       {
-        pfs->m_key= key;
-        if (username_length > 0)
-          pfs->m_username= &pfs->m_key.m_hash_key[0];
-        else
-          pfs->m_username= NULL;
-        pfs->m_username_length= username_length;
-
-        if (hostname_length > 0)
-          pfs->m_hostname= &pfs->m_key.m_hash_key[username_length + 1];
-        else
-          pfs->m_hostname= NULL;
-        pfs->m_hostname_length= hostname_length;
-
-        pfs->m_user= find_or_create_user(thread, username, username_length);
-        pfs->m_host= find_or_create_host(thread, hostname, hostname_length);
-
-        pfs->init_refcount();
-        pfs->reset_stats();
-        pfs->m_disconnected_count= 0;
-
-        if (username_length > 0 && hostname_length > 0)
-        {
-          lookup_setup_actor(thread, username, username_length, hostname, hostname_length,
-                             & pfs->m_enabled);
-        }
-        else
-          pfs->m_enabled= true;
+        lookup_setup_actor(thread, username, username_length, hostname, hostname_length,
+                           & pfs->m_enabled);
+      }
+      else
+        pfs->m_enabled= true;
 
-        int res;
-        pfs->m_lock.dirty_to_allocated();
-        res= lf_hash_insert(&account_hash, pins, &pfs);
-        if (likely(res == 0))
-        {
-          return pfs;
-        }
+      int res;
+      pfs->m_lock.dirty_to_allocated(& dirty_state);
+      res= lf_hash_insert(&account_hash, pins, &pfs);
+      if (likely(res == 0))
+      {
+        return pfs;
+      }
 
-        if (pfs->m_user)
-        {
-          pfs->m_user->release();
-          pfs->m_user= NULL;
-        }
-        if (pfs->m_host)
-        {
-          pfs->m_host->release();
-          pfs->m_host= NULL;
-        }
+      if (pfs->m_user)
+      {
+        pfs->m_user->release();
+        pfs->m_user= NULL;
+      }
+      if (pfs->m_host)
+      {
+        pfs->m_host->release();
+        pfs->m_host= NULL;
+      }
 
-        pfs->m_lock.allocated_to_free();
+      pfs->m_lock.allocated_to_free();
 
-        if (res > 0)
+      if (res > 0)
+      {
+        if (++retry_count > retry_max)
         {
-          if (++retry_count > retry_max)
-          {
-            account_lost++;
-            return NULL;
-          }
-          goto search;
+          account_lost++;
+          return NULL;
         }
-
-        account_lost++;
-        return NULL;
+        goto search;
       }
+
+      account_lost++;
+      return NULL;
     }
   }