Merge lp:~akopytov/percona-server/bug1131187-5.6 into lp:percona-server/5.6

Proposed by Alexey Kopytov
Status: Merged
Approved by: Laurynas Biveinis
Approved revision: 353
Merged at revision: 354
Proposed branch: lp:~akopytov/percona-server/bug1131187-5.6
Merge into: lp:percona-server/5.6
Diff against target: 424 lines (+113/-73)
7 files modified
Percona-Server/storage/innobase/include/read0read.h (+17/-4)
Percona-Server/storage/innobase/include/trx0purge.h (+4/-0)
Percona-Server/storage/innobase/include/trx0trx.h (+1/-3)
Percona-Server/storage/innobase/read/read0read.cc (+79/-51)
Percona-Server/storage/innobase/row/row0sel.cc (+1/-1)
Percona-Server/storage/innobase/trx/trx0purge.cc (+7/-2)
Percona-Server/storage/innobase/trx/trx0trx.cc (+4/-12)
To merge this branch: bzr merge lp:~akopytov/percona-server/bug1131187-5.6
Reviewer Review Type Date Requested Status
Laurynas Biveinis (community) Approve
Review via email: mp+165452@code.launchpad.net

Description of the change

    Manual merge of the fix for bug #1131187 "Remove malloc() from
    read_view_create_low()".

    This patch also incorporates the fix for #1170103 "Memory leak @
    read_view_open_now (read0read.c:152/166) in Percona Server 5.5.30-30.2
    GA"

http://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/114/

To post a comment you must log in.
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

Everything is OK. Just one follow-up question:

    - Upstream read_view_create_low() allocates the read view and its
      trx_ids in a single allocation. After the fix we are back to
      two allocations. It is possible to have a single allocation
      for us too, but I'm not sure whether it's worth it: on one
      hand, it's a single ut_realloc() anyway when trx_ids grows, on
      the other, reallocating the whole view would improve access
      locality and saves one memcpy() in read_view_clone(). Shall we
      open a bug for this or there is no benefit?

review: Needs Information
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) :
review: Approve
Revision history for this message
Alexey Kopytov (akopytov) wrote :

Hi Laurynas,

On Fri, 24 May 2013 09:25:35 -0000, Laurynas Biveinis wrote:
>
> - Upstream read_view_create_low() allocates the read view and its
> trx_ids in a single allocation. After the fix we are back to
> two allocations. It is possible to have a single allocation
> for us too, but I'm not sure whether it's worth it: on one
> hand, it's a single ut_realloc() anyway when trx_ids grows, on
> the other, reallocating the whole view would improve access
> locality and saves one memcpy() in read_view_clone(). Shall we
> open a bug for this or there is no benefit?
>

Yes I was considering that, but didn't like it. That would reduce
flexibility for some further work (and the fact that upstream code
relies on specific layout of data structures in memory is also
problematic). I don't think one extra mempcy() in read_view_clone() will
have any measurable effect.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Percona-Server/storage/innobase/include/read0read.h'
2--- Percona-Server/storage/innobase/include/read0read.h 2013-05-06 15:43:51 +0000
3+++ Percona-Server/storage/innobase/include/read0read.h 2013-05-23 17:46:30 +0000
4@@ -44,8 +44,8 @@
5 /*===============*/
6 trx_id_t cr_trx_id, /*!< in: trx_id of creating
7 transaction, or 0 used in purge */
8- mem_heap_t* heap); /*!< in: memory heap from which
9- allocated */
10+ read_view_t*& view); /*!< in,out: pre-allocated view array or
11+ NULL if a new one needs to be created */
12 /*********************************************************************//**
13 Makes a copy of the oldest existing read view, or opens a new. The view
14 must be closed with ..._close.
15@@ -54,8 +54,11 @@
16 read_view_t*
17 read_view_purge_open(
18 /*=================*/
19- mem_heap_t* heap); /*!< in: memory heap from which
20- allocated */
21+ read_view_t*& clone_view, /*!< in,out: pre-allocated view that
22+ will be used to clone the oldest view if
23+ exists */
24+ read_view_t*& view); /*!< in,out: pre-allocated view array or
25+ NULL if a new one needs to be created */
26 /*********************************************************************//**
27 Remove a read view from the trx_sys->view_list. */
28 UNIV_INLINE
29@@ -66,6 +69,13 @@
30 bool own_mutex); /*!< in: true if caller owns the
31 trx_sys_t::mutex */
32 /*********************************************************************//**
33+Frees memory allocated by a read view. */
34+UNIV_INTERN
35+void
36+read_view_free(
37+/*===========*/
38+ read_view_t*& view); /*< in,out: read view */
39+/*********************************************************************//**
40 Closes a consistent read view for MySQL. This function is called at an SQL
41 statement end if the trx isolation level is <= TRX_ISO_READ_COMMITTED. */
42 UNIV_INTERN
43@@ -146,6 +156,9 @@
44 this is the "low water mark". */
45 ulint n_trx_ids;
46 /*!< Number of cells in the trx_ids array */
47+ ulint max_trx_ids;
48+ /*!< Maximum number of cells in the trx_ids
49+ array */
50 trx_id_t* trx_ids;/*!< Additional trx ids which the read should
51 not see: typically, these are the read-write
52 active transactions at the time when the read
53
54=== modified file 'Percona-Server/storage/innobase/include/trx0purge.h'
55--- Percona-Server/storage/innobase/include/trx0purge.h 2013-05-12 09:13:00 +0000
56+++ Percona-Server/storage/innobase/include/trx0purge.h 2013-05-23 17:46:30 +0000
57@@ -156,6 +156,10 @@
58 parallelized purge operation */
59 read_view_t* view; /*!< The purge will not remove undo logs
60 which are >= this view (purge view) */
61+ read_view_t* prebuilt_clone; /*!< Pre-built view which is used as a
62+ temporary clone of the oldest view in
63+ read_view_purge_open() */
64+ read_view_t* prebuilt_view; /*!< Pre-built view array */
65 volatile ulint n_submitted; /*!< Count of total tasks submitted
66 to the task queue */
67 volatile ulint n_completed; /*!< Count of total tasks completed */
68
69=== modified file 'Percona-Server/storage/innobase/include/trx0trx.h'
70--- Percona-Server/storage/innobase/include/trx0trx.h 2013-05-12 06:24:46 +0000
71+++ Percona-Server/storage/innobase/include/trx0trx.h 2013-05-23 17:46:30 +0000
72@@ -911,9 +911,6 @@
73 survive over a transaction commit, if
74 it is a stored procedure with a COMMIT
75 WORK statement, for instance */
76- mem_heap_t* global_read_view_heap;
77- /*!< memory heap for the global read
78- view */
79 read_view_t* global_read_view;
80 /*!< consistent read view associated
81 to a transaction or NULL */
82@@ -923,6 +920,7 @@
83 associated to a transaction (i.e.
84 same as global_read_view) or read view
85 associated to a cursor */
86+ read_view_t* prebuilt_view; /* pre-built view array */
87 /*------------------------------*/
88 UT_LIST_BASE_NODE_T(trx_named_savept_t)
89 trx_savepoints; /*!< savepoints set with SAVEPOINT ...,
90
91=== modified file 'Percona-Server/storage/innobase/read/read0read.cc'
92--- Percona-Server/storage/innobase/read/read0read.cc 2013-05-06 15:43:51 +0000
93+++ Percona-Server/storage/innobase/read/read0read.cc 2013-05-23 17:46:30 +0000
94@@ -183,16 +183,29 @@
95 read_view_create_low(
96 /*=================*/
97 ulint n, /*!< in: number of cells in the trx_ids array */
98- mem_heap_t* heap) /*!< in: memory heap from which allocated */
99+ read_view_t*& view) /*!< in,out: pre-allocated view array or NULL if
100+ a new one needs to be created */
101 {
102- read_view_t* view;
103-
104- view = static_cast<read_view_t*>(
105- mem_heap_alloc(
106- heap, sizeof(*view) + n * sizeof(*view->trx_ids)));
107+ if (view == NULL) {
108+ view = static_cast<read_view_t*>(
109+ ut_malloc(sizeof(read_view_t)));
110+ view->max_trx_ids = 0;
111+ view->trx_ids = NULL;
112+ }
113+
114+ if (UNIV_UNLIKELY(view->max_trx_ids < n)) {
115+
116+ /* avoid frequent reallocations by extending the array to the
117+ desired size + 10% */
118+
119+ view->max_trx_ids = n + n / 10;
120+ view->trx_ids = static_cast<trx_id_t*>(
121+ ut_realloc(view->trx_ids,
122+ view->max_trx_ids *
123+ sizeof *view->trx_ids));
124+ }
125
126 view->n_trx_ids = n;
127- view->trx_ids = (trx_id_t*) &view[1];
128
129 return(view);
130 }
131@@ -207,37 +220,30 @@
132 read_view_t*
133 read_view_clone(
134 /*============*/
135- const read_view_t* view, /*!< in: view to clone */
136- mem_heap_t* heap) /*!< in: memory heap
137- from which allocated */
138+ const read_view_t* view, /*!< in: view to clone */
139+ read_view_t*& prebuilt_clone) /*!< in,out: prebuilt view or
140+ NULL */
141 {
142- ulint sz;
143 read_view_t* clone;
144- read_view_t* new_view;
145+ trx_id_t* old_trx_ids;
146+ ulint old_max_trx_ids;
147
148 ut_ad(mutex_own(&trx_sys->mutex));
149
150- /* Allocate space for two views. */
151-
152- sz = sizeof(*view) + view->n_trx_ids * sizeof(*view->trx_ids);
153-
154- /* Add an extra trx_id_t slot for the new view. */
155-
156- clone = static_cast<read_view_t*>(
157- mem_heap_alloc(heap, (sz * 2) + sizeof(trx_id_t)));
158-
159- /* Only the contents of the old view are important, the new view
160- will be created from this and so we don't copy that across. */
161-
162- memcpy(clone, view, sz);
163-
164- clone->trx_ids = (trx_id_t*) &clone[1];
165-
166- new_view = (read_view_t*) &clone->trx_ids[clone->n_trx_ids];
167- new_view->trx_ids = (trx_id_t*) &new_view[1];
168- new_view->n_trx_ids = clone->n_trx_ids + 1;
169-
170- ut_a(new_view->n_trx_ids == view->n_trx_ids + 1);
171+ clone = read_view_create_low(view->n_trx_ids, prebuilt_clone);
172+
173+ old_trx_ids = clone->trx_ids;
174+ old_max_trx_ids = clone->max_trx_ids;
175+
176+ memcpy(clone, view, sizeof(*view));
177+
178+ clone->trx_ids = old_trx_ids;
179+ clone->max_trx_ids = old_max_trx_ids;
180+
181+ if (view->n_trx_ids) {
182+ memcpy(clone->trx_ids, view->trx_ids,
183+ view->n_trx_ids * sizeof(trx_id_t));
184+ }
185
186 return(clone);
187 }
188@@ -331,15 +337,14 @@
189 /*===================*/
190 trx_id_t cr_trx_id, /*!< in: trx_id of creating
191 transaction, or 0 used in purge */
192- mem_heap_t* heap) /*!< in: memory heap from which
193- allocated */
194+ read_view_t*& view) /*!< in,out: pre-allocated view array or
195+ NULL if a new one needs to be created */
196 {
197- read_view_t* view;
198 ulint n_trx = UT_LIST_GET_LEN(trx_sys->rw_trx_list);
199
200 ut_ad(mutex_own(&trx_sys->mutex));
201
202- view = read_view_create_low(n_trx, heap);
203+ view = read_view_create_low(n_trx, view);
204
205 view->undo_no = 0;
206 view->type = VIEW_NORMAL;
207@@ -379,14 +384,12 @@
208 /*===============*/
209 trx_id_t cr_trx_id, /*!< in: trx_id of creating
210 transaction, or 0 used in purge */
211- mem_heap_t* heap) /*!< in: memory heap from which
212- allocated */
213+ read_view_t*& view) /*!< in,out: pre-allocated view array or
214+ NULL if a new one needs to be created */
215 {
216- read_view_t* view;
217-
218 mutex_enter(&trx_sys->mutex);
219
220- view = read_view_open_now_low(cr_trx_id, heap);
221+ view = read_view_open_now_low(cr_trx_id, view);
222
223 mutex_exit(&trx_sys->mutex);
224
225@@ -403,8 +406,11 @@
226 read_view_t*
227 read_view_purge_open(
228 /*=================*/
229- mem_heap_t* heap) /*!< in: memory heap from which
230- allocated */
231+ read_view_t*& prebuilt_clone, /*!< in,out: pre-allocated view that
232+ will be used to clone the oldest view if
233+ exists */
234+ read_view_t*& prebuilt_view) /*!< in,out: pre-allocated view array or
235+ NULL if a new one needs to be created */
236 {
237 ulint i;
238 read_view_t* view;
239@@ -418,16 +424,16 @@
240
241 if (oldest_view == NULL) {
242
243- view = read_view_open_now_low(0, heap);
244+ view = read_view_open_now_low(0, prebuilt_view);
245
246 mutex_exit(&trx_sys->mutex);
247
248 return(view);
249 }
250
251- /* Allocate space for both views, the oldest and the new purge view. */
252+ /* Clone the oldest view to a pre-allocated clone view */
253
254- oldest_view = read_view_clone(oldest_view, heap);
255+ oldest_view = read_view_clone(oldest_view, prebuilt_clone);
256
257 ut_ad(read_view_validate(oldest_view));
258
259@@ -436,7 +442,7 @@
260 ut_a(oldest_view->creator_trx_id > 0);
261 creator_trx_id = oldest_view->creator_trx_id;
262
263- view = (read_view_t*) &oldest_view->trx_ids[oldest_view->n_trx_ids];
264+ view = read_view_create_low(oldest_view->n_trx_ids + 1, prebuilt_view);
265
266 /* Add the creator transaction id in the trx_ids array in the
267 correct slot. */
268@@ -490,8 +496,6 @@
269
270 read_view_remove(trx->global_read_view, false);
271
272- mem_heap_empty(trx->global_read_view_heap);
273-
274 trx->read_view = NULL;
275 trx->global_read_view = NULL;
276 }
277@@ -566,6 +570,28 @@
278 }
279
280 /*********************************************************************//**
281+Frees resource allocated by a read view. */
282+UNIV_INTERN
283+void
284+read_view_free(
285+/*===========*/
286+ read_view_t*& view) /*< in,out: read view */
287+{
288+ if (view == NULL) {
289+
290+ return;
291+ }
292+
293+ if (view->trx_ids != NULL) {
294+ ut_free(view->trx_ids);
295+ }
296+
297+ ut_free(view);
298+
299+ view = NULL;
300+}
301+
302+/*********************************************************************//**
303 Create a high-granularity consistent cursor view for mysql to be used
304 in cursors. In this consistent read view modifications done by the
305 creating transaction after the cursor is created or future transactions
306@@ -601,7 +627,8 @@
307
308 n_trx = UT_LIST_GET_LEN(trx_sys->rw_trx_list);
309
310- curview->read_view = read_view_create_low(n_trx, curview->heap);
311+ curview->read_view = NULL;
312+ read_view_create_low(n_trx, curview->read_view);
313
314 view = curview->read_view;
315 view->undo_no = cr_trx->undo_no;
316@@ -653,6 +680,7 @@
317 trx->n_mysql_tables_in_use += curview->n_mysql_tables_in_use;
318
319 read_view_remove(curview->read_view, false);
320+ read_view_free(curview->read_view);
321
322 trx->read_view = trx->global_read_view;
323
324
325=== modified file 'Percona-Server/storage/innobase/row/row0sel.cc'
326--- Percona-Server/storage/innobase/row/row0sel.cc 2013-05-13 04:25:56 +0000
327+++ Percona-Server/storage/innobase/row/row0sel.cc 2013-05-23 17:46:30 +0000
328@@ -5206,7 +5206,7 @@
329 && !trx->read_view) {
330
331 trx->read_view = read_view_open_now(
332- trx->id, trx->global_read_view_heap);
333+ trx->id, trx->prebuilt_view);
334
335 trx->global_read_view = trx->read_view;
336 }
337
338=== modified file 'Percona-Server/storage/innobase/trx/trx0purge.cc'
339--- Percona-Server/storage/innobase/trx/trx0purge.cc 2013-05-12 09:13:00 +0000
340+++ Percona-Server/storage/innobase/trx/trx0purge.cc 2013-05-23 17:46:30 +0000
341@@ -152,7 +152,8 @@
342 purge_sys->query = trx_purge_graph_build(
343 purge_sys->trx, n_purge_threads);
344
345- purge_sys->view = read_view_purge_open(purge_sys->heap);
346+ purge_sys->view = read_view_purge_open(purge_sys->prebuilt_clone,
347+ purge_sys->prebuilt_view);
348 }
349
350 /************************************************************************
351@@ -173,6 +174,9 @@
352
353 purge_sys->sess = NULL;
354
355+ read_view_free(purge_sys->prebuilt_view);
356+ read_view_free(purge_sys->prebuilt_clone);
357+
358 purge_sys->view = NULL;
359
360 rw_lock_free(&purge_sys->latch);
361@@ -1200,7 +1204,8 @@
362
363 mem_heap_empty(purge_sys->heap);
364
365- purge_sys->view = read_view_purge_open(purge_sys->heap);
366+ purge_sys->view = read_view_purge_open(purge_sys->prebuilt_clone,
367+ purge_sys->prebuilt_view);
368
369 rw_lock_x_unlock(&purge_sys->latch);
370
371
372=== modified file 'Percona-Server/storage/innobase/trx/trx0trx.cc'
373--- Percona-Server/storage/innobase/trx/trx0trx.cc 2013-05-22 08:57:37 +0000
374+++ Percona-Server/storage/innobase/trx/trx0trx.cc 2013-05-23 17:46:30 +0000
375@@ -132,8 +132,6 @@
376
377 trx->search_latch_timeout = BTR_SEA_TIMEOUT;
378
379- trx->global_read_view_heap = mem_heap_create(256);
380-
381 trx->io_reads = 0;
382 trx->io_read = 0;
383 trx->io_reads_wait_timer = 0;
384@@ -240,10 +238,6 @@
385
386 ut_a(UT_LIST_GET_LEN(trx->lock.trx_locks) == 0);
387
388- if (trx->global_read_view_heap) {
389- mem_heap_free(trx->global_read_view_heap);
390- }
391-
392 ut_a(ib_vector_is_empty(trx->autoinc_locks));
393 /* We allocated a dedicated heap for the vector. */
394 ib_vector_free(trx->autoinc_locks);
395@@ -255,6 +249,8 @@
396
397 mutex_free(&trx->mutex);
398
399+ read_view_free(trx->prebuilt_view);
400+
401 mem_free(trx);
402 }
403
404@@ -1130,8 +1126,6 @@
405
406 if (trx->global_read_view != NULL) {
407
408- mem_heap_empty(trx->global_read_view_heap);
409-
410 trx->global_read_view = NULL;
411 }
412
413@@ -1383,10 +1377,8 @@
414 }
415
416 if (!trx->read_view) {
417-
418- trx->read_view = read_view_open_now(
419- trx->id, trx->global_read_view_heap);
420-
421+ trx->read_view = read_view_open_now(trx->id,
422+ trx->prebuilt_view);
423 trx->global_read_view = trx->read_view;
424 }
425

Subscribers

People subscribed via source and target branches