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
=== modified file 'Percona-Server/storage/innobase/include/read0read.h'
--- Percona-Server/storage/innobase/include/read0read.h 2013-05-06 15:43:51 +0000
+++ Percona-Server/storage/innobase/include/read0read.h 2013-05-23 17:46:30 +0000
@@ -44,8 +44,8 @@
44/*===============*/44/*===============*/
45 trx_id_t cr_trx_id, /*!< in: trx_id of creating45 trx_id_t cr_trx_id, /*!< in: trx_id of creating
46 transaction, or 0 used in purge */46 transaction, or 0 used in purge */
47 mem_heap_t* heap); /*!< in: memory heap from which47 read_view_t*& view); /*!< in,out: pre-allocated view array or
48 allocated */48 NULL if a new one needs to be created */
49/*********************************************************************//**49/*********************************************************************//**
50Makes a copy of the oldest existing read view, or opens a new. The view50Makes a copy of the oldest existing read view, or opens a new. The view
51must be closed with ..._close.51must be closed with ..._close.
@@ -54,8 +54,11 @@
54read_view_t*54read_view_t*
55read_view_purge_open(55read_view_purge_open(
56/*=================*/56/*=================*/
57 mem_heap_t* heap); /*!< in: memory heap from which57 read_view_t*& clone_view, /*!< in,out: pre-allocated view that
58 allocated */58 will be used to clone the oldest view if
59 exists */
60 read_view_t*& view); /*!< in,out: pre-allocated view array or
61 NULL if a new one needs to be created */
59/*********************************************************************//**62/*********************************************************************//**
60Remove a read view from the trx_sys->view_list. */63Remove a read view from the trx_sys->view_list. */
61UNIV_INLINE64UNIV_INLINE
@@ -66,6 +69,13 @@
66 bool own_mutex); /*!< in: true if caller owns the69 bool own_mutex); /*!< in: true if caller owns the
67 trx_sys_t::mutex */70 trx_sys_t::mutex */
68/*********************************************************************//**71/*********************************************************************//**
72Frees memory allocated by a read view. */
73UNIV_INTERN
74void
75read_view_free(
76/*===========*/
77 read_view_t*& view); /*< in,out: read view */
78/*********************************************************************//**
69Closes a consistent read view for MySQL. This function is called at an SQL79Closes a consistent read view for MySQL. This function is called at an SQL
70statement end if the trx isolation level is <= TRX_ISO_READ_COMMITTED. */80statement end if the trx isolation level is <= TRX_ISO_READ_COMMITTED. */
71UNIV_INTERN81UNIV_INTERN
@@ -146,6 +156,9 @@
146 this is the "low water mark". */156 this is the "low water mark". */
147 ulint n_trx_ids;157 ulint n_trx_ids;
148 /*!< Number of cells in the trx_ids array */158 /*!< Number of cells in the trx_ids array */
159 ulint max_trx_ids;
160 /*!< Maximum number of cells in the trx_ids
161 array */
149 trx_id_t* trx_ids;/*!< Additional trx ids which the read should162 trx_id_t* trx_ids;/*!< Additional trx ids which the read should
150 not see: typically, these are the read-write163 not see: typically, these are the read-write
151 active transactions at the time when the read164 active transactions at the time when the read
152165
=== modified file 'Percona-Server/storage/innobase/include/trx0purge.h'
--- Percona-Server/storage/innobase/include/trx0purge.h 2013-05-12 09:13:00 +0000
+++ Percona-Server/storage/innobase/include/trx0purge.h 2013-05-23 17:46:30 +0000
@@ -156,6 +156,10 @@
156 parallelized purge operation */156 parallelized purge operation */
157 read_view_t* view; /*!< The purge will not remove undo logs157 read_view_t* view; /*!< The purge will not remove undo logs
158 which are >= this view (purge view) */158 which are >= this view (purge view) */
159 read_view_t* prebuilt_clone; /*!< Pre-built view which is used as a
160 temporary clone of the oldest view in
161 read_view_purge_open() */
162 read_view_t* prebuilt_view; /*!< Pre-built view array */
159 volatile ulint n_submitted; /*!< Count of total tasks submitted163 volatile ulint n_submitted; /*!< Count of total tasks submitted
160 to the task queue */164 to the task queue */
161 volatile ulint n_completed; /*!< Count of total tasks completed */165 volatile ulint n_completed; /*!< Count of total tasks completed */
162166
=== modified file 'Percona-Server/storage/innobase/include/trx0trx.h'
--- Percona-Server/storage/innobase/include/trx0trx.h 2013-05-12 06:24:46 +0000
+++ Percona-Server/storage/innobase/include/trx0trx.h 2013-05-23 17:46:30 +0000
@@ -911,9 +911,6 @@
911 survive over a transaction commit, if911 survive over a transaction commit, if
912 it is a stored procedure with a COMMIT912 it is a stored procedure with a COMMIT
913 WORK statement, for instance */913 WORK statement, for instance */
914 mem_heap_t* global_read_view_heap;
915 /*!< memory heap for the global read
916 view */
917 read_view_t* global_read_view;914 read_view_t* global_read_view;
918 /*!< consistent read view associated915 /*!< consistent read view associated
919 to a transaction or NULL */916 to a transaction or NULL */
@@ -923,6 +920,7 @@
923 associated to a transaction (i.e.920 associated to a transaction (i.e.
924 same as global_read_view) or read view921 same as global_read_view) or read view
925 associated to a cursor */922 associated to a cursor */
923 read_view_t* prebuilt_view; /* pre-built view array */
926 /*------------------------------*/924 /*------------------------------*/
927 UT_LIST_BASE_NODE_T(trx_named_savept_t)925 UT_LIST_BASE_NODE_T(trx_named_savept_t)
928 trx_savepoints; /*!< savepoints set with SAVEPOINT ...,926 trx_savepoints; /*!< savepoints set with SAVEPOINT ...,
929927
=== modified file 'Percona-Server/storage/innobase/read/read0read.cc'
--- Percona-Server/storage/innobase/read/read0read.cc 2013-05-06 15:43:51 +0000
+++ Percona-Server/storage/innobase/read/read0read.cc 2013-05-23 17:46:30 +0000
@@ -183,16 +183,29 @@
183read_view_create_low(183read_view_create_low(
184/*=================*/184/*=================*/
185 ulint n, /*!< in: number of cells in the trx_ids array */185 ulint n, /*!< in: number of cells in the trx_ids array */
186 mem_heap_t* heap) /*!< in: memory heap from which allocated */186 read_view_t*& view) /*!< in,out: pre-allocated view array or NULL if
187 a new one needs to be created */
187{188{
188 read_view_t* view;189 if (view == NULL) {
189190 view = static_cast<read_view_t*>(
190 view = static_cast<read_view_t*>(191 ut_malloc(sizeof(read_view_t)));
191 mem_heap_alloc(192 view->max_trx_ids = 0;
192 heap, sizeof(*view) + n * sizeof(*view->trx_ids)));193 view->trx_ids = NULL;
194 }
195
196 if (UNIV_UNLIKELY(view->max_trx_ids < n)) {
197
198 /* avoid frequent reallocations by extending the array to the
199 desired size + 10% */
200
201 view->max_trx_ids = n + n / 10;
202 view->trx_ids = static_cast<trx_id_t*>(
203 ut_realloc(view->trx_ids,
204 view->max_trx_ids *
205 sizeof *view->trx_ids));
206 }
193207
194 view->n_trx_ids = n;208 view->n_trx_ids = n;
195 view->trx_ids = (trx_id_t*) &view[1];
196209
197 return(view);210 return(view);
198}211}
@@ -207,37 +220,30 @@
207read_view_t*220read_view_t*
208read_view_clone(221read_view_clone(
209/*============*/222/*============*/
210 const read_view_t* view, /*!< in: view to clone */223 const read_view_t* view, /*!< in: view to clone */
211 mem_heap_t* heap) /*!< in: memory heap224 read_view_t*& prebuilt_clone) /*!< in,out: prebuilt view or
212 from which allocated */225 NULL */
213{226{
214 ulint sz;
215 read_view_t* clone;227 read_view_t* clone;
216 read_view_t* new_view;228 trx_id_t* old_trx_ids;
229 ulint old_max_trx_ids;
217230
218 ut_ad(mutex_own(&trx_sys->mutex));231 ut_ad(mutex_own(&trx_sys->mutex));
219232
220 /* Allocate space for two views. */233 clone = read_view_create_low(view->n_trx_ids, prebuilt_clone);
221234
222 sz = sizeof(*view) + view->n_trx_ids * sizeof(*view->trx_ids);235 old_trx_ids = clone->trx_ids;
223236 old_max_trx_ids = clone->max_trx_ids;
224 /* Add an extra trx_id_t slot for the new view. */237
225238 memcpy(clone, view, sizeof(*view));
226 clone = static_cast<read_view_t*>(239
227 mem_heap_alloc(heap, (sz * 2) + sizeof(trx_id_t)));240 clone->trx_ids = old_trx_ids;
228241 clone->max_trx_ids = old_max_trx_ids;
229 /* Only the contents of the old view are important, the new view242
230 will be created from this and so we don't copy that across. */243 if (view->n_trx_ids) {
231244 memcpy(clone->trx_ids, view->trx_ids,
232 memcpy(clone, view, sz);245 view->n_trx_ids * sizeof(trx_id_t));
233246 }
234 clone->trx_ids = (trx_id_t*) &clone[1];
235
236 new_view = (read_view_t*) &clone->trx_ids[clone->n_trx_ids];
237 new_view->trx_ids = (trx_id_t*) &new_view[1];
238 new_view->n_trx_ids = clone->n_trx_ids + 1;
239
240 ut_a(new_view->n_trx_ids == view->n_trx_ids + 1);
241247
242 return(clone);248 return(clone);
243}249}
@@ -331,15 +337,14 @@
331/*===================*/337/*===================*/
332 trx_id_t cr_trx_id, /*!< in: trx_id of creating338 trx_id_t cr_trx_id, /*!< in: trx_id of creating
333 transaction, or 0 used in purge */339 transaction, or 0 used in purge */
334 mem_heap_t* heap) /*!< in: memory heap from which340 read_view_t*& view) /*!< in,out: pre-allocated view array or
335 allocated */341 NULL if a new one needs to be created */
336{342{
337 read_view_t* view;
338 ulint n_trx = UT_LIST_GET_LEN(trx_sys->rw_trx_list);343 ulint n_trx = UT_LIST_GET_LEN(trx_sys->rw_trx_list);
339344
340 ut_ad(mutex_own(&trx_sys->mutex));345 ut_ad(mutex_own(&trx_sys->mutex));
341346
342 view = read_view_create_low(n_trx, heap);347 view = read_view_create_low(n_trx, view);
343348
344 view->undo_no = 0;349 view->undo_no = 0;
345 view->type = VIEW_NORMAL;350 view->type = VIEW_NORMAL;
@@ -379,14 +384,12 @@
379/*===============*/384/*===============*/
380 trx_id_t cr_trx_id, /*!< in: trx_id of creating385 trx_id_t cr_trx_id, /*!< in: trx_id of creating
381 transaction, or 0 used in purge */386 transaction, or 0 used in purge */
382 mem_heap_t* heap) /*!< in: memory heap from which387 read_view_t*& view) /*!< in,out: pre-allocated view array or
383 allocated */388 NULL if a new one needs to be created */
384{389{
385 read_view_t* view;
386
387 mutex_enter(&trx_sys->mutex);390 mutex_enter(&trx_sys->mutex);
388391
389 view = read_view_open_now_low(cr_trx_id, heap);392 view = read_view_open_now_low(cr_trx_id, view);
390393
391 mutex_exit(&trx_sys->mutex);394 mutex_exit(&trx_sys->mutex);
392395
@@ -403,8 +406,11 @@
403read_view_t*406read_view_t*
404read_view_purge_open(407read_view_purge_open(
405/*=================*/408/*=================*/
406 mem_heap_t* heap) /*!< in: memory heap from which409 read_view_t*& prebuilt_clone, /*!< in,out: pre-allocated view that
407 allocated */410 will be used to clone the oldest view if
411 exists */
412 read_view_t*& prebuilt_view) /*!< in,out: pre-allocated view array or
413 NULL if a new one needs to be created */
408{414{
409 ulint i;415 ulint i;
410 read_view_t* view;416 read_view_t* view;
@@ -418,16 +424,16 @@
418424
419 if (oldest_view == NULL) {425 if (oldest_view == NULL) {
420426
421 view = read_view_open_now_low(0, heap);427 view = read_view_open_now_low(0, prebuilt_view);
422428
423 mutex_exit(&trx_sys->mutex);429 mutex_exit(&trx_sys->mutex);
424430
425 return(view);431 return(view);
426 }432 }
427433
428 /* Allocate space for both views, the oldest and the new purge view. */434 /* Clone the oldest view to a pre-allocated clone view */
429435
430 oldest_view = read_view_clone(oldest_view, heap);436 oldest_view = read_view_clone(oldest_view, prebuilt_clone);
431437
432 ut_ad(read_view_validate(oldest_view));438 ut_ad(read_view_validate(oldest_view));
433439
@@ -436,7 +442,7 @@
436 ut_a(oldest_view->creator_trx_id > 0);442 ut_a(oldest_view->creator_trx_id > 0);
437 creator_trx_id = oldest_view->creator_trx_id;443 creator_trx_id = oldest_view->creator_trx_id;
438444
439 view = (read_view_t*) &oldest_view->trx_ids[oldest_view->n_trx_ids];445 view = read_view_create_low(oldest_view->n_trx_ids + 1, prebuilt_view);
440446
441 /* Add the creator transaction id in the trx_ids array in the447 /* Add the creator transaction id in the trx_ids array in the
442 correct slot. */448 correct slot. */
@@ -490,8 +496,6 @@
490496
491 read_view_remove(trx->global_read_view, false);497 read_view_remove(trx->global_read_view, false);
492498
493 mem_heap_empty(trx->global_read_view_heap);
494
495 trx->read_view = NULL;499 trx->read_view = NULL;
496 trx->global_read_view = NULL;500 trx->global_read_view = NULL;
497}501}
@@ -566,6 +570,28 @@
566}570}
567571
568/*********************************************************************//**572/*********************************************************************//**
573Frees resource allocated by a read view. */
574UNIV_INTERN
575void
576read_view_free(
577/*===========*/
578 read_view_t*& view) /*< in,out: read view */
579{
580 if (view == NULL) {
581
582 return;
583 }
584
585 if (view->trx_ids != NULL) {
586 ut_free(view->trx_ids);
587 }
588
589 ut_free(view);
590
591 view = NULL;
592}
593
594/*********************************************************************//**
569Create a high-granularity consistent cursor view for mysql to be used595Create a high-granularity consistent cursor view for mysql to be used
570in cursors. In this consistent read view modifications done by the596in cursors. In this consistent read view modifications done by the
571creating transaction after the cursor is created or future transactions597creating transaction after the cursor is created or future transactions
@@ -601,7 +627,8 @@
601627
602 n_trx = UT_LIST_GET_LEN(trx_sys->rw_trx_list);628 n_trx = UT_LIST_GET_LEN(trx_sys->rw_trx_list);
603629
604 curview->read_view = read_view_create_low(n_trx, curview->heap);630 curview->read_view = NULL;
631 read_view_create_low(n_trx, curview->read_view);
605632
606 view = curview->read_view;633 view = curview->read_view;
607 view->undo_no = cr_trx->undo_no;634 view->undo_no = cr_trx->undo_no;
@@ -653,6 +680,7 @@
653 trx->n_mysql_tables_in_use += curview->n_mysql_tables_in_use;680 trx->n_mysql_tables_in_use += curview->n_mysql_tables_in_use;
654681
655 read_view_remove(curview->read_view, false);682 read_view_remove(curview->read_view, false);
683 read_view_free(curview->read_view);
656684
657 trx->read_view = trx->global_read_view;685 trx->read_view = trx->global_read_view;
658686
659687
=== modified file 'Percona-Server/storage/innobase/row/row0sel.cc'
--- Percona-Server/storage/innobase/row/row0sel.cc 2013-05-13 04:25:56 +0000
+++ Percona-Server/storage/innobase/row/row0sel.cc 2013-05-23 17:46:30 +0000
@@ -5206,7 +5206,7 @@
5206 && !trx->read_view) {5206 && !trx->read_view) {
52075207
5208 trx->read_view = read_view_open_now(5208 trx->read_view = read_view_open_now(
5209 trx->id, trx->global_read_view_heap);5209 trx->id, trx->prebuilt_view);
52105210
5211 trx->global_read_view = trx->read_view;5211 trx->global_read_view = trx->read_view;
5212 }5212 }
52135213
=== modified file 'Percona-Server/storage/innobase/trx/trx0purge.cc'
--- Percona-Server/storage/innobase/trx/trx0purge.cc 2013-05-12 09:13:00 +0000
+++ Percona-Server/storage/innobase/trx/trx0purge.cc 2013-05-23 17:46:30 +0000
@@ -152,7 +152,8 @@
152 purge_sys->query = trx_purge_graph_build(152 purge_sys->query = trx_purge_graph_build(
153 purge_sys->trx, n_purge_threads);153 purge_sys->trx, n_purge_threads);
154154
155 purge_sys->view = read_view_purge_open(purge_sys->heap);155 purge_sys->view = read_view_purge_open(purge_sys->prebuilt_clone,
156 purge_sys->prebuilt_view);
156}157}
157158
158/************************************************************************159/************************************************************************
@@ -173,6 +174,9 @@
173174
174 purge_sys->sess = NULL;175 purge_sys->sess = NULL;
175176
177 read_view_free(purge_sys->prebuilt_view);
178 read_view_free(purge_sys->prebuilt_clone);
179
176 purge_sys->view = NULL;180 purge_sys->view = NULL;
177181
178 rw_lock_free(&purge_sys->latch);182 rw_lock_free(&purge_sys->latch);
@@ -1200,7 +1204,8 @@
12001204
1201 mem_heap_empty(purge_sys->heap);1205 mem_heap_empty(purge_sys->heap);
12021206
1203 purge_sys->view = read_view_purge_open(purge_sys->heap);1207 purge_sys->view = read_view_purge_open(purge_sys->prebuilt_clone,
1208 purge_sys->prebuilt_view);
12041209
1205 rw_lock_x_unlock(&purge_sys->latch);1210 rw_lock_x_unlock(&purge_sys->latch);
12061211
12071212
=== modified file 'Percona-Server/storage/innobase/trx/trx0trx.cc'
--- Percona-Server/storage/innobase/trx/trx0trx.cc 2013-05-22 08:57:37 +0000
+++ Percona-Server/storage/innobase/trx/trx0trx.cc 2013-05-23 17:46:30 +0000
@@ -132,8 +132,6 @@
132132
133 trx->search_latch_timeout = BTR_SEA_TIMEOUT;133 trx->search_latch_timeout = BTR_SEA_TIMEOUT;
134134
135 trx->global_read_view_heap = mem_heap_create(256);
136
137 trx->io_reads = 0;135 trx->io_reads = 0;
138 trx->io_read = 0;136 trx->io_read = 0;
139 trx->io_reads_wait_timer = 0;137 trx->io_reads_wait_timer = 0;
@@ -240,10 +238,6 @@
240238
241 ut_a(UT_LIST_GET_LEN(trx->lock.trx_locks) == 0);239 ut_a(UT_LIST_GET_LEN(trx->lock.trx_locks) == 0);
242240
243 if (trx->global_read_view_heap) {
244 mem_heap_free(trx->global_read_view_heap);
245 }
246
247 ut_a(ib_vector_is_empty(trx->autoinc_locks));241 ut_a(ib_vector_is_empty(trx->autoinc_locks));
248 /* We allocated a dedicated heap for the vector. */242 /* We allocated a dedicated heap for the vector. */
249 ib_vector_free(trx->autoinc_locks);243 ib_vector_free(trx->autoinc_locks);
@@ -255,6 +249,8 @@
255249
256 mutex_free(&trx->mutex);250 mutex_free(&trx->mutex);
257251
252 read_view_free(trx->prebuilt_view);
253
258 mem_free(trx);254 mem_free(trx);
259}255}
260256
@@ -1130,8 +1126,6 @@
11301126
1131 if (trx->global_read_view != NULL) {1127 if (trx->global_read_view != NULL) {
11321128
1133 mem_heap_empty(trx->global_read_view_heap);
1134
1135 trx->global_read_view = NULL;1129 trx->global_read_view = NULL;
1136 }1130 }
11371131
@@ -1383,10 +1377,8 @@
1383 }1377 }
13841378
1385 if (!trx->read_view) {1379 if (!trx->read_view) {
13861380 trx->read_view = read_view_open_now(trx->id,
1387 trx->read_view = read_view_open_now(1381 trx->prebuilt_view);
1388 trx->id, trx->global_read_view_heap);
1389
1390 trx->global_read_view = trx->read_view;1382 trx->global_read_view = trx->read_view;
1391 }1383 }
13921384

Subscribers

People subscribed via source and target branches