Percona Server moved to https://jira.percona.com/projects/PS

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

  1. bug1123921
  2. Merge into 5.5
Proposed by Alexey Kopytov
Status: Merged
Approved by: Laurynas Biveinis
Approved revision: no longer in the source branch.
Merged at revision: 459
Proposed branch: lp:~akopytov/percona-server/bug1123921
Merge into: lp:percona-server/5.5
Diff against target: 217 lines (+26/-24)
5 files modified
Percona-Server/storage/innobase/buf/buf0buf.c (+16/-14)
Percona-Server/storage/innobase/lock/lock0lock.c (+2/-2)
Percona-Server/storage/innobase/os/os0file.c (+3/-3)
Percona-Server/storage/innobase/srv/srv0srv.c (+2/-2)
Percona-Server/storage/innobase/trx/trx0trx.c (+3/-3)
To merge this branch: bzr merge lp:~akopytov/percona-server/bug1123921
Reviewer Review Type Date Requested Status
Laurynas Biveinis (community) Approve
Stewart Smith (community) Needs Fixing
Sergei Glushchenko (community) g2 Approve
Review via email: mp+149830@code.launchpad.net

Description of the change

  Bug #1123921: Suspicious logic in XtraDB extra slow log stats collection

  Fixed unnecessary calls to innobase_get_slow_log() and added compiler
  branch prediction hints.

To post a comment you must log in.
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

Approve

review: Approve (g2)
Revision history for this message
Stewart Smith (stewart) wrote :

http://jenkins.percona.com/view/Merge/job/merge-PS-5.5-staging/52/

Many test failures

review: Needs Fixing
Revision history for this message
Alexey Kopytov (akopytov) wrote :

Fixed. The problem was that not all code branches were checking the (trx != NULL && trx->take_stats) invariant before calling _increment_page_get_statistics().

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

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Percona-Server/storage/innobase/buf/buf0buf.c'
2--- Percona-Server/storage/innobase/buf/buf0buf.c 2013-02-07 04:38:31 +0000
3+++ Percona-Server/storage/innobase/buf/buf0buf.c 2013-02-26 09:04:21 +0000
4@@ -66,9 +66,7 @@
5 byte block_hash_offset;
6
7 ut_ad(block);
8-
9- if (!innobase_get_slow_log() || !trx || !trx->take_stats)
10- return;
11+ ut_ad(trx && trx->take_stats);
12
13 if (!trx->distinct_page_access_hash) {
14 trx->distinct_page_access_hash = mem_alloc(DPAH_SIZE);
15@@ -1967,7 +1965,7 @@
16 ib_uint64_t finish_time;
17 buf_pool_t* buf_pool = buf_pool_get(space, offset);
18
19- if (innobase_get_slow_log()) {
20+ if (UNIV_UNLIKELY(innobase_get_slow_log())) {
21 trx = innobase_get_trx();
22 }
23 buf_pool->stat.n_page_gets++;
24@@ -2111,7 +2109,7 @@
25 /* Let us wait until the read operation
26 completes */
27
28- if (innobase_get_slow_log() && trx && trx->take_stats)
29+ if (UNIV_UNLIKELY(trx && trx->take_stats))
30 {
31 ut_usectime(&sec, &ms);
32 start_time = (ib_uint64_t)sec * 1000000 + ms;
33@@ -2132,7 +2130,7 @@
34 break;
35 }
36 }
37- if (innobase_get_slow_log() && trx && trx->take_stats && start_time)
38+ if (UNIV_UNLIKELY(start_time != 0))
39 {
40 ut_usectime(&sec, &ms);
41 finish_time = (ib_uint64_t)sec * 1000000 + ms;
42@@ -2487,7 +2485,7 @@
43 || ibuf_page_low(space, zip_size, offset,
44 FALSE, file, line, NULL));
45 #endif
46- if (innobase_get_slow_log()) {
47+ if (UNIV_UNLIKELY(innobase_get_slow_log())) {
48 trx = innobase_get_trx();
49 }
50 buf_pool->stat.n_page_gets++;
51@@ -2913,7 +2911,7 @@
52 /* Let us wait until the read operation
53 completes */
54
55- if (innobase_get_slow_log() && trx && trx->take_stats)
56+ if (UNIV_UNLIKELY(trx && trx->take_stats))
57 {
58 ut_usectime(&sec, &ms);
59 start_time = (ib_uint64_t)sec * 1000000 + ms;
60@@ -2935,7 +2933,7 @@
61 break;
62 }
63 }
64- if (innobase_get_slow_log() && trx && trx->take_stats && start_time)
65+ if (UNIV_UNLIKELY(start_time != 0))
66 {
67 ut_usectime(&sec, &ms);
68 finish_time = (ib_uint64_t)sec * 1000000 + ms;
69@@ -2974,7 +2972,7 @@
70 ut_a(ibuf_count_get(buf_block_get_space(block),
71 buf_block_get_page_no(block)) == 0);
72 #endif
73- if (innobase_get_slow_log()) {
74+ if (UNIV_UNLIKELY(trx && trx->take_stats)) {
75 _increment_page_get_statistics(block, trx);
76 }
77
78@@ -3079,7 +3077,7 @@
79 #if defined UNIV_DEBUG_FILE_ACCESSES || defined UNIV_DEBUG
80 ut_a(block->page.file_page_was_freed == FALSE);
81 #endif
82- if (innobase_get_slow_log()) {
83+ if (UNIV_UNLIKELY(innobase_get_slow_log())) {
84 trx = innobase_get_trx();
85 }
86
87@@ -3100,7 +3098,7 @@
88 buf_pool = buf_pool_from_block(block);
89 buf_pool->stat.n_page_gets++;
90
91- if (innobase_get_slow_log()) {
92+ if (UNIV_UNLIKELY(trx && trx->take_stats)) {
93 _increment_page_get_statistics(block, trx);
94 }
95 return(TRUE);
96@@ -3212,9 +3210,13 @@
97 #endif
98 buf_pool->stat.n_page_gets++;
99
100- if (innobase_get_slow_log()) {
101+ if (UNIV_UNLIKELY(innobase_get_slow_log())) {
102+
103 trx = innobase_get_trx();
104- _increment_page_get_statistics(block, trx);
105+ if (trx != NULL && trx->take_stats) {
106+
107+ _increment_page_get_statistics(block, trx);
108+ }
109 }
110
111 return(TRUE);
112
113=== modified file 'Percona-Server/storage/innobase/lock/lock0lock.c'
114--- Percona-Server/storage/innobase/lock/lock0lock.c 2013-01-18 12:02:28 +0000
115+++ Percona-Server/storage/innobase/lock/lock0lock.c 2013-02-26 09:04:21 +0000
116@@ -1829,7 +1829,7 @@
117 trx->que_state = TRX_QUE_LOCK_WAIT;
118 trx->was_chosen_as_deadlock_victim = FALSE;
119 trx->wait_started = time(NULL);
120- if (innobase_get_slow_log() && trx->take_stats) {
121+ if (UNIV_UNLIKELY(trx->take_stats)) {
122 ut_usectime(&sec, &ms);
123 trx->lock_que_wait_ustarted = (ib_uint64_t)sec * 1000000 + ms;
124 }
125@@ -3837,7 +3837,7 @@
126 return(DB_SUCCESS);
127 }
128
129- if (innobase_get_slow_log() && trx->take_stats) {
130+ if (UNIV_UNLIKELY(trx->take_stats)) {
131 ut_usectime(&sec, &ms);
132 trx->lock_que_wait_ustarted = (ib_uint64_t)sec * 1000000 + ms;
133 }
134
135=== modified file 'Percona-Server/storage/innobase/os/os0file.c'
136--- Percona-Server/storage/innobase/os/os0file.c 2013-02-06 09:26:12 +0000
137+++ Percona-Server/storage/innobase/os/os0file.c 2013-02-26 09:04:21 +0000
138@@ -2289,7 +2289,7 @@
139
140 os_n_file_reads++;
141
142- if (innobase_get_slow_log() && trx && trx->take_stats)
143+ if (UNIV_UNLIKELY(trx && trx->take_stats))
144 {
145 trx->io_reads++;
146 trx->io_read += n;
147@@ -2322,7 +2322,7 @@
148 os_n_pending_reads--;
149 os_mutex_exit(os_file_count_mutex);
150
151- if (innobase_get_slow_log() && trx && trx->take_stats && start_time)
152+ if (UNIV_UNLIKELY(start_time != 0))
153 {
154 ut_usectime(&sec, &ms);
155 finish_time = (ib_uint64_t)sec * 1000000 + ms;
156@@ -2376,7 +2376,7 @@
157 os_n_pending_reads--;
158 os_mutex_exit(os_file_count_mutex);
159
160- if (innobase_get_slow_log() && trx && trx->take_stats && start_time)
161+ if (UNIV_UNLIKELY(start_time != 0)
162 {
163 ut_usectime(&sec, &ms);
164 finish_time = (ib_uint64_t)sec * 1000000 + ms;
165
166=== modified file 'Percona-Server/storage/innobase/srv/srv0srv.c'
167--- Percona-Server/storage/innobase/srv/srv0srv.c 2013-02-20 06:41:32 +0000
168+++ Percona-Server/storage/innobase/srv/srv0srv.c 2013-02-26 09:04:21 +0000
169@@ -1448,7 +1448,7 @@
170 ut_ad(!sync_thread_levels_nonempty_trx(trx->has_search_latch));
171 #endif /* UNIV_SYNC_DEBUG */
172
173- if (innobase_get_slow_log() && trx->take_stats) {
174+ if (UNIV_UNLIKELY(trx->take_stats)) {
175 ut_usectime(&sec, &ms);
176 start_time = (ib_uint64_t)sec * 1000000 + ms;
177 } else {
178@@ -1463,7 +1463,7 @@
179
180 trx->op_info = "";
181
182- if (innobase_get_slow_log() && trx->take_stats && start_time) {
183+ if (UNIV_UNLIKELY(start_time != 0)) {
184 ut_usectime(&sec, &ms);
185 finish_time = (ib_uint64_t)sec * 1000000 + ms;
186 trx->innodb_que_wait_timer += (ulint)(finish_time - start_time);
187
188=== modified file 'Percona-Server/storage/innobase/trx/trx0trx.c'
189--- Percona-Server/storage/innobase/trx/trx0trx.c 2012-08-16 13:44:06 +0000
190+++ Percona-Server/storage/innobase/trx/trx0trx.c 2013-02-26 09:04:21 +0000
191@@ -236,7 +236,7 @@
192
193 mutex_exit(&kernel_mutex);
194
195- if (innobase_get_slow_log() && trx->take_stats) {
196+ if (UNIV_UNLIKELY(trx->take_stats)) {
197 trx->distinct_page_access_hash = mem_alloc(DPAH_SIZE);
198 memset(trx->distinct_page_access_hash, 0, DPAH_SIZE);
199 }
200@@ -1269,7 +1269,7 @@
201 thr = UT_LIST_GET_FIRST(trx->wait_thrs);
202 }
203
204- if (innobase_get_slow_log() && trx->take_stats) {
205+ if (UNIV_UNLIKELY(trx->take_stats)) {
206 ut_usectime(&sec, &ms);
207 now = (ib_uint64_t)sec * 1000000 + ms;
208 trx->lock_que_wait_timer += (ulint)(now - trx->lock_que_wait_ustarted);
209@@ -1304,7 +1304,7 @@
210 thr = UT_LIST_GET_FIRST(trx->wait_thrs);
211 }
212
213- if (innobase_get_slow_log() && trx->take_stats) {
214+ if (UNIV_UNLIKELY(trx->take_stats)) {
215 ut_usectime(&sec, &ms);
216 now = (ib_uint64_t)sec * 1000000 + ms;
217 trx->lock_que_wait_timer += (ulint)(now - trx->lock_que_wait_ustarted);

Subscribers

People subscribed via source and target branches