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

Proposed by Alexey Kopytov
Status: Merged
Approved by: Stewart Smith
Approved revision: 451
Merged at revision: 481
Proposed branch: lp:~akopytov/percona-server/bug1128066
Merge into: lp:percona-server/5.5
Diff against target: 251 lines (+58/-41)
6 files modified
Percona-Server/sql/handler.cc (+7/-17)
Percona-Server/sql/sql_base.cc (+9/-3)
Percona-Server/sql/sql_class.cc (+1/-3)
Percona-Server/sql/sql_connect.cc (+9/-6)
Percona-Server/sql/sql_parse.cc (+6/-2)
Percona-Server/sql/sql_prepare.cc (+26/-10)
To merge this branch: bzr merge lp:~akopytov/percona-server/bug1128066
Reviewer Review Type Date Requested Status
Stewart Smith (community) Approve
Review via email: mp+149824@code.launchpad.net

Description of the change

  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/

To post a comment you must log in.
Stewart Smith (stewart) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Percona-Server/sql/handler.cc'
2--- Percona-Server/sql/handler.cc 2013-01-22 16:32:44 +0000
3+++ Percona-Server/sql/handler.cc 2013-02-21 13:05:37 +0000
4@@ -2348,8 +2348,13 @@
5 dup_ref=ref+ALIGN_SIZE(ref_length);
6 cached_table_flags= table_flags();
7 }
8- rows_read= rows_changed= 0;
9- memset(index_rows_read, 0, sizeof(index_rows_read));
10+
11+ if (unlikely(opt_userstat))
12+ {
13+ rows_read= rows_changed= 0;
14+ memset(index_rows_read, 0, sizeof(index_rows_read));
15+ }
16+
17 DBUG_RETURN(error);
18 }
19
20@@ -3861,12 +3866,6 @@
21 // Updates the global table stats with the TABLE this handler represents.
22 void handler::update_global_table_stats()
23 {
24- if (!opt_userstat)
25- {
26- rows_read= rows_changed= 0;
27- return;
28- }
29-
30 if (!rows_read && !rows_changed)
31 return; // Nothing to update.
32 // table_cache_key is db_name + '\0' + table_name + '\0'.
33@@ -3923,15 +3922,6 @@
34 if (!table->s || !table->s->table_cache_key.str || !table->s->table_name.str)
35 return;
36
37- if (!opt_userstat)
38- {
39- for (uint x= 0; x < table->s->keys; ++x)
40- {
41- index_rows_read[x]= 0;
42- }
43- return;
44- }
45-
46 for (uint x = 0; x < table->s->keys; ++x)
47 {
48 if (index_rows_read[x])
49
50=== modified file 'Percona-Server/sql/sql_base.cc'
51--- Percona-Server/sql/sql_base.cc 2013-01-09 23:45:25 +0000
52+++ Percona-Server/sql/sql_base.cc 2013-02-21 13:05:37 +0000
53@@ -1593,11 +1593,13 @@
54 table->mdl_ticket= NULL;
55
56 mysql_mutex_lock(&thd->LOCK_thd_data);
57- if(table->file)
58+
59+ if(unlikely(opt_userstat && table->file))
60 {
61 table->file->update_global_table_stats();
62 table->file->update_global_index_stats();
63 }
64+
65 *table_ptr=table->next;
66 mysql_mutex_unlock(&thd->LOCK_thd_data);
67
68@@ -2236,8 +2238,12 @@
69 DBUG_PRINT("tmptable", ("closing table: '%s'.'%s'",
70 table->s->db.str, table->s->table_name.str));
71
72- table->file->update_global_table_stats();
73- table->file->update_global_index_stats();
74+ if (unlikely(opt_userstat))
75+ {
76+ table->file->update_global_table_stats();
77+ table->file->update_global_index_stats();
78+ }
79+
80 free_io_cache(table);
81 closefrm(table, 0);
82 if (delete_table)
83
84=== modified file 'Percona-Server/sql/sql_class.cc'
85--- Percona-Server/sql/sql_class.cc 2013-01-28 11:27:47 +0000
86+++ Percona-Server/sql/sql_class.cc 2013-02-21 13:05:37 +0000
87@@ -1344,8 +1344,6 @@
88 // Updates 'diff' stats of a THD.
89 void THD::update_stats(bool ran_command)
90 {
91- if (opt_userstat)
92- {
93 diff_total_busy_time+= busy_time;
94 diff_total_cpu_time+= cpu_time;
95 diff_total_bytes_received+= bytes_received;
96@@ -1390,7 +1388,7 @@
97 /* reset counters to zero to avoid double-counting since values
98 are already store in diff_total_*.
99 */
100- }
101+
102 busy_time= 0;
103 cpu_time= 0;
104 bytes_received= 0;
105
106=== modified file 'Percona-Server/sql/sql_connect.cc'
107--- Percona-Server/sql/sql_connect.cc 2013-01-30 09:55:26 +0000
108+++ Percona-Server/sql/sql_connect.cc 2013-02-21 13:05:37 +0000
109@@ -562,9 +562,6 @@
110 const char* client_string= get_client_host(thd);
111 int return_value= 0;
112
113- if (!opt_userstat)
114- return return_value;
115-
116 if (acl_is_utility_user(thd->security_ctx->user, thd->security_ctx->host,
117 thd->security_ctx->ip))
118 return return_value;
119@@ -1263,8 +1260,9 @@
120 my_net_set_write_timeout(net, thd->variables.net_write_timeout);
121
122 thd->reset_stats();
123+
124 // Updates global user connection stats.
125- if (increment_connection_count(thd, true))
126+ if (opt_userstat && increment_connection_count(thd, true))
127 DBUG_RETURN(1);
128
129 DBUG_RETURN(0);
130@@ -1493,8 +1491,13 @@
131
132 end_thread:
133 close_connection(thd);
134- thd->update_stats(false);
135- update_global_user_stats(thd, create_user, time(NULL));
136+
137+ if (unlikely(opt_userstat))
138+ {
139+ thd->update_stats(false);
140+ update_global_user_stats(thd, create_user, time(NULL));
141+ }
142+
143 if (MYSQL_CALLBACK_ELSE(thd->scheduler, end_thread, (thd, 1), 0))
144 return; // Probably no-threads
145
146
147=== modified file 'Percona-Server/sql/sql_parse.cc'
148--- Percona-Server/sql/sql_parse.cc 2013-02-18 21:08:57 +0000
149+++ Percona-Server/sql/sql_parse.cc 2013-02-21 13:05:37 +0000
150@@ -5920,9 +5920,13 @@
151 else
152 thd->cpu_time = 0;
153 }
154+
155 // Updates THD stats and the global user stats.
156- thd->update_stats(true);
157- update_global_user_stats(thd, true, time(NULL));
158+ if (unlikely(opt_userstat))
159+ {
160+ thd->update_stats(true);
161+ update_global_user_stats(thd, true, time(NULL));
162+ }
163
164 DBUG_VOID_RETURN;
165 }
166
167=== modified file 'Percona-Server/sql/sql_prepare.cc'
168--- Percona-Server/sql/sql_prepare.cc 2013-01-17 22:50:22 +0000
169+++ Percona-Server/sql/sql_prepare.cc 2013-02-21 13:05:37 +0000
170@@ -2195,7 +2195,7 @@
171 double start_cpu_nsecs= 0;
172 double end_cpu_nsecs= 0;
173
174- if (opt_userstat)
175+ if (unlikely(opt_userstat))
176 {
177 #ifdef HAVE_CLOCK_GETTIME
178 /* get start cputime */
179@@ -2237,7 +2237,7 @@
180
181 /* check_prepared_statemnt sends the metadata packet in case of success */
182 end:
183- if (opt_userstat)
184+ if (unlikely(opt_userstat))
185 {
186 // Gets the end time.
187 if (!(end_time_error= gettimeofday(&end_time, NULL)))
188@@ -2279,9 +2279,13 @@
189 else
190 thd->cpu_time = 0;
191 }
192+
193 // Updates THD stats and the global user stats.
194- thd->update_stats(true);
195- update_global_user_stats(thd, true, time(NULL));
196+ if (unlikely(opt_userstat))
197+ {
198+ thd->update_stats(true);
199+ update_global_user_stats(thd, true, time(NULL));
200+ }
201
202 DBUG_VOID_RETURN;
203 }
204@@ -2730,9 +2734,13 @@
205 else
206 thd->cpu_time = 0;
207 }
208+
209 // Updates THD stats and the global user stats.
210- thd->update_stats(true);
211- update_global_user_stats(thd, true, time(NULL));
212+ if (unlikely(opt_userstat))
213+ {
214+ thd->update_stats(true);
215+ update_global_user_stats(thd, true, time(NULL));
216+ }
217
218 DBUG_VOID_RETURN;
219 }
220@@ -2905,9 +2913,13 @@
221 } else
222 thd->cpu_time= 0;
223 }
224+
225 // Updates THD stats and the global user stats.
226- thd->update_stats(true);
227- update_global_user_stats(thd, true, time(NULL));
228+ if (unlikely(opt_userstat))
229+ {
230+ thd->update_stats(true);
231+ update_global_user_stats(thd, true, time(NULL));
232+ }
233
234 DBUG_VOID_RETURN;
235 }
236@@ -3031,9 +3043,13 @@
237 else
238 thd->cpu_time= 0;
239 }
240+
241 // Updates THD stats and the global user stats.
242- thd->update_stats(true);
243- update_global_user_stats(thd, true, time(NULL));
244+ if (unlikely(opt_userstat))
245+ {
246+ thd->update_stats(true);
247+ update_global_user_stats(thd, true, time(NULL));
248+ }
249
250 DBUG_VOID_RETURN;
251 }

Subscribers

People subscribed via source and target branches