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

[Alexey Kopytov]

    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/

[Laurynas Biveinis (laurynas-biveinis)]

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

[Laurynas Biveinis (laurynas-biveinis)]

review: Approve

[Alexey Kopytov (akopytov)]

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.

=== 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 @@
 /*===============*/
 	trx_id_t	cr_trx_id,	/*!< in: trx_id of creating
 					transaction, or 0 used in purge */
-	mem_heap_t*	heap);		/*!< in: memory heap from which
-					allocated */
+	read_view_t*&	view);		/*!< in,out: pre-allocated view array or
+					NULL if a new one needs to be created */
 /*********************************************************************//**
 Makes a copy of the oldest existing read view, or opens a new. The view
 must be closed with ..._close.
@@ -54,8 +54,11 @@
 read_view_t*
 read_view_purge_open(
 /*=================*/
-	mem_heap_t*	heap);		/*!< in: memory heap from which
-					allocated */
+	read_view_t*&	clone_view,	/*!< in,out: pre-allocated view that
+					will be used to clone the oldest view if
+					exists */
+	read_view_t*&	view);		/*!< in,out: pre-allocated view array or
+					NULL if a new one needs to be created */
 /*********************************************************************//**
 Remove a read view from the trx_sys->view_list. */
 UNIV_INLINE
@@ -66,6 +69,13 @@
 	bool		own_mutex);	/*!< in: true if caller owns the
 					trx_sys_t::mutex */
 /*********************************************************************//**
+Frees memory allocated by a read view. */
+UNIV_INTERN
+void
+read_view_free(
+/*===========*/
+	read_view_t*&	view);	/*< in,out: read view */
+/*********************************************************************//**
 Closes a consistent read view for MySQL. This function is called at an SQL
 statement end if the trx isolation level is <= TRX_ISO_READ_COMMITTED. */
 UNIV_INTERN
@@ -146,6 +156,9 @@
 				this is the "low water mark". */
 	ulint		n_trx_ids;
 				/*!< Number of cells in the trx_ids array */
+	ulint		max_trx_ids;
+				/*!< Maximum number of cells in the trx_ids
+				array */
 	trx_id_t*	trx_ids;/*!< Additional trx ids which the read should
 				not see: typically, these are the read-write
 				active transactions at the time when the read

=== 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 @@
 					parallelized purge operation */
 	read_view_t*	view;		/*!< The purge will not remove undo logs
 					which are >= this view (purge view) */
+	read_view_t*	prebuilt_clone;	/*!< Pre-built view which is used as a
+					temporary clone of the oldest view in
+					read_view_purge_open() */
+	read_view_t*	prebuilt_view;	/*!< Pre-built view array */
 	volatile ulint	n_submitted;	/*!< Count of total tasks submitted
 					to the task queue */
 	volatile ulint	n_completed;	/*!< Count of total tasks completed */

=== 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 @@
 					survive over a transaction commit, if
 					it is a stored procedure with a COMMIT
 					WORK statement, for instance */
-	mem_heap_t*	global_read_view_heap;
-					/*!< memory heap for the global read
-					view */
 	read_view_t*	global_read_view;
 					/*!< consistent read view associated
 					to a transaction or NULL */
@@ -923,6 +920,7 @@
 					associated to a transaction (i.e.
 					same as global_read_view) or read view
 					associated to a cursor */
+	read_view_t*	prebuilt_view;	/* pre-built view array */
 	/*------------------------------*/
 	UT_LIST_BASE_NODE_T(trx_named_savept_t)
 			trx_savepoints;	/*!< savepoints set with SAVEPOINT ...,

=== 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 @@
 read_view_create_low(
 /*=================*/
 	ulint		n,	/*!< in: number of cells in the trx_ids array */
-	mem_heap_t*	heap)	/*!< in: memory heap from which allocated */
+	read_view_t*&	view)	/*!< in,out: pre-allocated view array or NULL if
+				a new one needs to be created */
 {
-	read_view_t*	view;
-
-	view = static_cast<read_view_t*>(
-		mem_heap_alloc(
-			heap, sizeof(*view) + n * sizeof(*view->trx_ids)));
+	if (view == NULL) {
+		view = static_cast<read_view_t*>(
+			ut_malloc(sizeof(read_view_t)));
+		view->max_trx_ids = 0;
+		view->trx_ids = NULL;
+	}
+
+	if (UNIV_UNLIKELY(view->max_trx_ids < n)) {
+
+		/* avoid frequent reallocations by extending the array to the
+		desired size + 10% */
+
+		view->max_trx_ids = n + n / 10;
+		view->trx_ids = static_cast<trx_id_t*>(
+			ut_realloc(view->trx_ids,
+				   view->max_trx_ids *
+				   sizeof *view->trx_ids));
+	}
 
 	view->n_trx_ids = n;
-	view->trx_ids = (trx_id_t*) &view[1];
 
 	return(view);
 }
@@ -207,37 +220,30 @@
 read_view_t*
 read_view_clone(
 /*============*/
-	const read_view_t*	view,	/*!< in: view to clone */
-	mem_heap_t*		heap)	/*!< in: memory heap
-					from which allocated */
+	const read_view_t*	view,		/*!< in: view to clone */
+	read_view_t*&		prebuilt_clone)	/*!< in,out: prebuilt view or
+						NULL */
 {
-	ulint		sz;
 	read_view_t*	clone;
-	read_view_t*	new_view;
+	trx_id_t*	old_trx_ids;
+	ulint		old_max_trx_ids;
 
 	ut_ad(mutex_own(&trx_sys->mutex));
 
-	/* Allocate space for two views. */
-
-	sz = sizeof(*view) + view->n_trx_ids * sizeof(*view->trx_ids);
-
-	/* Add an extra trx_id_t slot for the new view. */
-
-	clone = static_cast<read_view_t*>(
-		mem_heap_alloc(heap, (sz * 2) + sizeof(trx_id_t)));
-
-	/* Only the contents of the old view are important, the new view
-	will be created from this and so we don't copy that across. */
-
-	memcpy(clone, view, sz);
-
-	clone->trx_ids = (trx_id_t*) &clone[1];
-
-	new_view = (read_view_t*) &clone->trx_ids[clone->n_trx_ids];
-	new_view->trx_ids = (trx_id_t*) &new_view[1];
-	new_view->n_trx_ids = clone->n_trx_ids + 1;
-
-	ut_a(new_view->n_trx_ids == view->n_trx_ids + 1);
+	clone = read_view_create_low(view->n_trx_ids, prebuilt_clone);
+
+	old_trx_ids = clone->trx_ids;
+	old_max_trx_ids = clone->max_trx_ids;
+
+	memcpy(clone, view, sizeof(*view));
+
+	clone->trx_ids = old_trx_ids;
+	clone->max_trx_ids = old_max_trx_ids;
+
+	if (view->n_trx_ids) {
+		memcpy(clone->trx_ids, view->trx_ids,
+		       view->n_trx_ids * sizeof(trx_id_t));
+	}
 
 	return(clone);
 }
@@ -331,15 +337,14 @@
 /*===================*/
 	trx_id_t	cr_trx_id,	/*!< in: trx_id of creating
 					transaction, or 0 used in purge */
-	mem_heap_t*	heap)		/*!< in: memory heap from which
-					allocated */
+	read_view_t*&	view)		/*!< in,out: pre-allocated view array or
+					NULL if a new one needs to be created */
 {
-	read_view_t*	view;
 	ulint		n_trx = UT_LIST_GET_LEN(trx_sys->rw_trx_list);
 
 	ut_ad(mutex_own(&trx_sys->mutex));
 
-	view = read_view_create_low(n_trx, heap);
+	view = read_view_create_low(n_trx, view);
 
 	view->undo_no = 0;
 	view->type = VIEW_NORMAL;
@@ -379,14 +384,12 @@
 /*===============*/
 	trx_id_t	cr_trx_id,	/*!< in: trx_id of creating
 					transaction, or 0 used in purge */
-	mem_heap_t*	heap)		/*!< in: memory heap from which
-					allocated */
+	read_view_t*&	view)		/*!< in,out: pre-allocated view array or
+					NULL if a new one needs to be created */
 {
-	read_view_t*	view;
-
 	mutex_enter(&trx_sys->mutex);
 
-	view = read_view_open_now_low(cr_trx_id, heap);
+	view = read_view_open_now_low(cr_trx_id, view);
 
 	mutex_exit(&trx_sys->mutex);
 
@@ -403,8 +406,11 @@
 read_view_t*
 read_view_purge_open(
 /*=================*/
-	mem_heap_t*	heap)		/*!< in: memory heap from which
-					allocated */
+	read_view_t*&	prebuilt_clone,	/*!< in,out: pre-allocated view that
+					will be used to clone the oldest view if
+					exists */
+	read_view_t*&	prebuilt_view)	/*!< in,out: pre-allocated view array or
+					NULL if a new one needs to be created */
 {
 	ulint		i;
 	read_view_t*	view;
@@ -418,16 +424,16 @@
 
 	if (oldest_view == NULL) {
 
-		view = read_view_open_now_low(0, heap);
+		view = read_view_open_now_low(0, prebuilt_view);
 
 		mutex_exit(&trx_sys->mutex);
 
 		return(view);
 	}
 
-	/* Allocate space for both views, the oldest and the new purge view. */
+	/* Clone the oldest view to a pre-allocated clone view */
 
-	oldest_view = read_view_clone(oldest_view, heap);
+	oldest_view = read_view_clone(oldest_view, prebuilt_clone);
 
 	ut_ad(read_view_validate(oldest_view));
 
@@ -436,7 +442,7 @@
 	ut_a(oldest_view->creator_trx_id > 0);
 	creator_trx_id = oldest_view->creator_trx_id;
 
-	view = (read_view_t*) &oldest_view->trx_ids[oldest_view->n_trx_ids];
+	view = read_view_create_low(oldest_view->n_trx_ids + 1, prebuilt_view);
 
 	/* Add the creator transaction id in the trx_ids array in the
 	correct slot. */
@@ -490,8 +496,6 @@
 
 	read_view_remove(trx->global_read_view, false);
 
-	mem_heap_empty(trx->global_read_view_heap);
-
 	trx->read_view = NULL;
 	trx->global_read_view = NULL;
 }
@@ -566,6 +570,28 @@
 }
 
 /*********************************************************************//**
+Frees resource allocated by a read view. */
+UNIV_INTERN
+void
+read_view_free(
+/*===========*/
+	read_view_t*&	view)	/*< in,out: read view */
+{
+	if (view == NULL) {
+
+		return;
+	}
+
+	if (view->trx_ids != NULL) {
+		ut_free(view->trx_ids);
+	}
+
+	ut_free(view);
+
+	view = NULL;
+}
+
+/*********************************************************************//**
 Create a high-granularity consistent cursor view for mysql to be used
 in cursors. In this consistent read view modifications done by the
 creating transaction after the cursor is created or future transactions
@@ -601,7 +627,8 @@
 
 	n_trx = UT_LIST_GET_LEN(trx_sys->rw_trx_list);
 
-	curview->read_view = read_view_create_low(n_trx, curview->heap);
+	curview->read_view = NULL;
+	read_view_create_low(n_trx, curview->read_view);
 
 	view = curview->read_view;
 	view->undo_no = cr_trx->undo_no;
@@ -653,6 +680,7 @@
 	trx->n_mysql_tables_in_use += curview->n_mysql_tables_in_use;
 
 	read_view_remove(curview->read_view, false);
+	read_view_free(curview->read_view);
 
 	trx->read_view = trx->global_read_view;
 

=== 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 @@
 		    && !trx->read_view) {
 
 			trx->read_view = read_view_open_now(
-				trx->id, trx->global_read_view_heap);
+				trx->id, trx->prebuilt_view);
 
 			trx->global_read_view = trx->read_view;
 		}

=== 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 @@
 	purge_sys->query = trx_purge_graph_build(
 		purge_sys->trx, n_purge_threads);
 
-	purge_sys->view = read_view_purge_open(purge_sys->heap);
+	purge_sys->view = read_view_purge_open(purge_sys->prebuilt_clone,
+					       purge_sys->prebuilt_view);
 }
 
 /************************************************************************
@@ -173,6 +174,9 @@
 
 	purge_sys->sess = NULL;
 
+	read_view_free(purge_sys->prebuilt_view);
+	read_view_free(purge_sys->prebuilt_clone);
+
 	purge_sys->view = NULL;
 
 	rw_lock_free(&purge_sys->latch);
@@ -1200,7 +1204,8 @@
 
 	mem_heap_empty(purge_sys->heap);
 
-	purge_sys->view = read_view_purge_open(purge_sys->heap);
+	purge_sys->view = read_view_purge_open(purge_sys->prebuilt_clone,
+					       purge_sys->prebuilt_view);
 
 	rw_lock_x_unlock(&purge_sys->latch);
 

=== 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 @@
 
 	trx->search_latch_timeout = BTR_SEA_TIMEOUT;
 
-	trx->global_read_view_heap = mem_heap_create(256);
-
 	trx->io_reads = 0;
 	trx->io_read = 0;
 	trx->io_reads_wait_timer = 0;
@@ -240,10 +238,6 @@
 
 	ut_a(UT_LIST_GET_LEN(trx->lock.trx_locks) == 0);
 
-	if (trx->global_read_view_heap) {
-		mem_heap_free(trx->global_read_view_heap);
-	}
-
 	ut_a(ib_vector_is_empty(trx->autoinc_locks));
 	/* We allocated a dedicated heap for the vector. */
 	ib_vector_free(trx->autoinc_locks);
@@ -255,6 +249,8 @@
 
 	mutex_free(&trx->mutex);
 
+	read_view_free(trx->prebuilt_view);
+
 	mem_free(trx);
 }
 
@@ -1130,8 +1126,6 @@
 
 	if (trx->global_read_view != NULL) {
 
-		mem_heap_empty(trx->global_read_view_heap);
-
 		trx->global_read_view = NULL;
 	}
 
@@ -1383,10 +1377,8 @@
 	}
 
 	if (!trx->read_view) {
-
-		trx->read_view = read_view_open_now(
-			trx->id, trx->global_read_view_heap);
-
+		trx->read_view = read_view_open_now(trx->id,
+						    trx->prebuilt_view);
 		trx->global_read_view = trx->read_view;
 	}