Merge lp:~akopytov/percona-server/bug1128066 into lp:percona-server/5.5

[Alexey Kopytov]

  Bug #1128066: Suboptimal userstat code

  Make sure no additional work is done when userstat is disabled.

http://jenkins.percona.com/view/PS%205.5/job/percona-server-5.5-param/681/

[Stewart Smith (stewart)]

I'm not sure about the unlikely() annotation on the if(opt_userstat) as if somebody is running with userstats enabled, it's likely() rather than unlikely(). I wonder if this is something we should just leave up to normal branch prediction?

review: Needs Information

[Alexey Kopytov (akopytov)]

Yes, when opt_userstat enabled the unlikely() case becomes likely(). However, some overhead from enabling userstat is expected, and is probably much larger than branch mispredictions overhead. In other words, we are optimizing for the "common" case here, with the common case being userstat disabled.

[Stewart Smith (stewart)]

review: Approve

=== modified file 'Percona-Server/sql/handler.cc'
--- Percona-Server/sql/handler.cc	2013-01-22 16:32:44 +0000
+++ Percona-Server/sql/handler.cc	2013-02-21 13:05:37 +0000
@@ -2348,8 +2348,13 @@
       dup_ref=ref+ALIGN_SIZE(ref_length);
     cached_table_flags= table_flags();
   }
-  rows_read= rows_changed= 0;
-  memset(index_rows_read, 0, sizeof(index_rows_read));
+
+  if (unlikely(opt_userstat))
+  {
+    rows_read= rows_changed= 0;
+    memset(index_rows_read, 0, sizeof(index_rows_read));
+  }
+
   DBUG_RETURN(error);
 }
 
@@ -3861,12 +3866,6 @@
 // Updates the global table stats with the TABLE this handler represents.
 void handler::update_global_table_stats()
 {
-  if (!opt_userstat)
-  {
-    rows_read= rows_changed= 0;
-    return;
-  }
-
   if (!rows_read && !rows_changed)
     return;  // Nothing to update.
   // table_cache_key is db_name + '\0' + table_name + '\0'.
@@ -3923,15 +3922,6 @@
   if (!table->s || !table->s->table_cache_key.str || !table->s->table_name.str)
     return;
 
-  if (!opt_userstat)
-  {
-    for (uint x= 0; x < table->s->keys; ++x)
-    {
-      index_rows_read[x]= 0;
-    }
-    return;
-  }
-
   for (uint x = 0; x < table->s->keys; ++x)
   {
     if (index_rows_read[x])

=== modified file 'Percona-Server/sql/sql_base.cc'
--- Percona-Server/sql/sql_base.cc	2013-01-09 23:45:25 +0000
+++ Percona-Server/sql/sql_base.cc	2013-02-21 13:05:37 +0000
@@ -1593,11 +1593,13 @@
   table->mdl_ticket= NULL;
 
   mysql_mutex_lock(&thd->LOCK_thd_data);
-  if(table->file)
+
+  if(unlikely(opt_userstat && table->file))
   {
     table->file->update_global_table_stats();
     table->file->update_global_index_stats();
   }
+
   *table_ptr=table->next;
   mysql_mutex_unlock(&thd->LOCK_thd_data);
 
@@ -2236,8 +2238,12 @@
   DBUG_PRINT("tmptable", ("closing table: '%s'.'%s'",
                           table->s->db.str, table->s->table_name.str));
 
-  table->file->update_global_table_stats();
-  table->file->update_global_index_stats();
+  if (unlikely(opt_userstat))
+  {
+    table->file->update_global_table_stats();
+    table->file->update_global_index_stats();
+  }
+
   free_io_cache(table);
   closefrm(table, 0);
   if (delete_table)

=== modified file 'Percona-Server/sql/sql_class.cc'
--- Percona-Server/sql/sql_class.cc	2013-01-28 11:27:47 +0000
+++ Percona-Server/sql/sql_class.cc	2013-02-21 13:05:37 +0000
@@ -1344,8 +1344,6 @@
 // Updates 'diff' stats of a THD.
 void THD::update_stats(bool ran_command)
 {
-  if (opt_userstat)
-  {
   diff_total_busy_time+=            busy_time;
   diff_total_cpu_time+=             cpu_time;
   diff_total_bytes_received+=       bytes_received;
@@ -1390,7 +1388,7 @@
   /* reset counters to zero to avoid double-counting since values
      are already store in diff_total_*.
   */
-  }
+
   busy_time=            0;
   cpu_time=             0;
   bytes_received=       0;

=== modified file 'Percona-Server/sql/sql_connect.cc'
--- Percona-Server/sql/sql_connect.cc	2013-01-30 09:55:26 +0000
+++ Percona-Server/sql/sql_connect.cc	2013-02-21 13:05:37 +0000
@@ -562,9 +562,6 @@
   const char* client_string= get_client_host(thd);
   int return_value=          0;
 
-  if (!opt_userstat)
-    return return_value;
-
   if (acl_is_utility_user(thd->security_ctx->user, thd->security_ctx->host,
       thd->security_ctx->ip))
     return return_value;
@@ -1263,8 +1260,9 @@
   my_net_set_write_timeout(net, thd->variables.net_write_timeout);
 
   thd->reset_stats();
+
   // Updates global user connection stats.
-  if (increment_connection_count(thd, true))
+  if (opt_userstat && increment_connection_count(thd, true))
     DBUG_RETURN(1);
 
   DBUG_RETURN(0);
@@ -1493,8 +1491,13 @@
    
 end_thread:
     close_connection(thd);
-    thd->update_stats(false);
-    update_global_user_stats(thd, create_user, time(NULL));
+
+    if (unlikely(opt_userstat))
+    {
+      thd->update_stats(false);
+      update_global_user_stats(thd, create_user, time(NULL));
+    }
+
     if (MYSQL_CALLBACK_ELSE(thd->scheduler, end_thread, (thd, 1), 0))
       return;                                 // Probably no-threads
 

=== modified file 'Percona-Server/sql/sql_parse.cc'
--- Percona-Server/sql/sql_parse.cc	2013-02-18 21:08:57 +0000
+++ Percona-Server/sql/sql_parse.cc	2013-02-21 13:05:37 +0000
@@ -5920,9 +5920,13 @@
     else
       thd->cpu_time = 0;
   }
+
   // Updates THD stats and the global user stats.
-  thd->update_stats(true);
-  update_global_user_stats(thd, true, time(NULL));
+  if (unlikely(opt_userstat))
+  {
+    thd->update_stats(true);
+    update_global_user_stats(thd, true, time(NULL));
+  }
 
   DBUG_VOID_RETURN;
 }

=== modified file 'Percona-Server/sql/sql_prepare.cc'
--- Percona-Server/sql/sql_prepare.cc	2013-01-17 22:50:22 +0000
+++ Percona-Server/sql/sql_prepare.cc	2013-02-21 13:05:37 +0000
@@ -2195,7 +2195,7 @@
   double start_cpu_nsecs= 0;
   double end_cpu_nsecs=   0;
 
-  if (opt_userstat)
+  if (unlikely(opt_userstat))
   {
 #ifdef HAVE_CLOCK_GETTIME
     /* get start cputime */
@@ -2237,7 +2237,7 @@
 
   /* check_prepared_statemnt sends the metadata packet in case of success */
 end:
-  if (opt_userstat)
+  if (unlikely(opt_userstat))
   {
     // Gets the end time.
     if (!(end_time_error= gettimeofday(&end_time, NULL)))
@@ -2279,9 +2279,13 @@
     else
       thd->cpu_time = 0;
   }
+
   // Updates THD stats and the global user stats.
-  thd->update_stats(true);
-  update_global_user_stats(thd, true, time(NULL));
+  if (unlikely(opt_userstat))
+  {
+    thd->update_stats(true);
+    update_global_user_stats(thd, true, time(NULL));
+  }
 
   DBUG_VOID_RETURN;
 }
@@ -2730,9 +2734,13 @@
     else
       thd->cpu_time = 0;
   }
+
   // Updates THD stats and the global user stats.
-  thd->update_stats(true);
-  update_global_user_stats(thd, true, time(NULL));
+  if (unlikely(opt_userstat))
+  {
+    thd->update_stats(true);
+    update_global_user_stats(thd, true, time(NULL));
+  }
 
   DBUG_VOID_RETURN;
 }
@@ -2905,9 +2913,13 @@
     } else
       thd->cpu_time= 0;
   }
+
   // Updates THD stats and the global user stats.
-  thd->update_stats(true);
-  update_global_user_stats(thd, true, time(NULL));
+  if (unlikely(opt_userstat))
+  {
+    thd->update_stats(true);
+    update_global_user_stats(thd, true, time(NULL));
+  }
 
   DBUG_VOID_RETURN;
 }
@@ -3031,9 +3043,13 @@
     else
       thd->cpu_time= 0;
   }
+
   // Updates THD stats and the global user stats.
-  thd->update_stats(true);
-  update_global_user_stats(thd, true, time(NULL));
+  if (unlikely(opt_userstat))
+  {
+    thd->update_stats(true);
+    update_global_user_stats(thd, true, time(NULL));
+  }
 
   DBUG_VOID_RETURN;
 }