List:Commits« Previous MessageNext Message »
From:vasil.dimov Date:November 10 2011 8:33am
Subject:bzr push into mysql-trunk branch (vasil.dimov:3568 to 3569) Bug#11764622
View as plain text  
 3569 Vasil Dimov	2011-11-10
      Partial fix for Bug#11764622 57480: MEMORY LEAK WHEN HAVING 256+ TABLES
      
      In ha_innobase::open() we used to allocate two buffers: upd_buff and
      key_val_buff. Both with the size of the entire row of the table.
      
      With a table like 'c VARCHAR(64000), KEY (c(3072))' we used to allocate
      about 128KB at table open time for those two buffers.
      
      upd_buff is only used in ha_innobase::update_row(), so we delay its
      allocation until ha_innobase::update_row() is called. After all that
      method may never be called in the lifetime of the ha_innobase object.
      Renamed to upd_buf on purpose - to trigger a compilation error if there
      appears to be some other place where it is used and also for consistency
      with other InnoDB code that uses 'buf' instead of 'buff'.
      
      key_val_buff is only used in ha_innobase::index_read() and in
      ha_innobase::records_in_range() to be passed as a working space to
      row_sel_convert_mysql_key_to_innobase(). That buffer does not need to be
      the size of the whole row, only the part covered by indexes. So we
      allocate the buffer from the stack inside
      row_sel_convert_mysql_key_to_innobase() and delete it altogether from
      the ha_innobase object. This also makes the code cleaner.
      
      This change removes the following malloc() during open table:
      
      #0 malloc (size=128128)
      #1 my_malloc
      #2 my_multi_malloc
      #3 ha_innobase::open
      #4 handler::ha_open
      #5 ha_partition::open
      #6 handler::ha_open
      #7 open_table_from_share
      #8 open_table
      #9 open_and_process_table
      #10 open_tables
      #11 open_and_lock_tables
      #12 open_and_lock_tables
      #13 execute_sqlcom_select
      #14 mysql_execute_command
      #15 mysql_parse
      
      where the allocation size is roughly twice the row size.
      
      In exchange if UPDATE is ever done on that table, then during the first
      UPDATE an allocation of about the row size will be done (64k in this example).
      
      Also REC_VERSION_56_MAX_INDEX_COL_LEN (3072) will be allocated from the stack
      of row_sel_convert_mysql_key_to_innobase(). This function is only ever called
      from ha_innobase::index_read() (once) and from ha_innobase::records_in_range()
      (twice).

    modified:
      storage/innobase/handler/ha_innodb.cc
      storage/innobase/handler/ha_innodb.h
      storage/innobase/include/row0sel.h
      storage/innobase/row/row0sel.c
 3568 Vasil Dimov	2011-11-09
      Partial fix for Bug#11764622 57480: MEMORY LEAK WHEN HAVING 256+ TABLES
      
      In dict_load_foreigns() use onstack storage for the dtuple and foreign key
      id that are only used inside that function.
      
      Introduce a new function dtuple_create_from_mem() that initializes a
      dtuple from an already allocated memory chunk.
      
      This change removes two malloc() calls per one open table (for a table
      that does not have foreign keys!). It removes the following codepath
      (executed twice per open table):
      
      #0 malloc (size=376)
      #1 mem_area_alloc
      #2 mem_heap_create_block
      #3 mem_heap_create_func
      #4 dict_load_foreigns
      #5 dict_load_table
      #6 dict_table_open_on_name_low
      #7 dict_table_open_on_name
      #8 ha_innobase::open
      #9 handler::ha_open
      #10 ha_partition::open
      #11 handler::ha_open
      #12 open_table_from_share
      #13 open_table
      #14 open_and_process_table
      #15 open_tables
      
      and it causes the stack memory footprint of dict_load_foreigns() to grow
      with 504 bytes (DTUPLE_EST_ALLOC(1)==72 + 2*NAME_LEN+64==448 - removing
      two pointers, 8 bytes each).

    modified:
      storage/innobase/dict/dict0load.c
      storage/innobase/include/data0data.h
      storage/innobase/include/data0data.ic
=== modified file 'storage/innobase/handler/ha_innodb.cc'
--- a/storage/innobase/handler/ha_innodb.cc	revid:vasil.dimov@stripped
+++ b/storage/innobase/handler/ha_innodb.cc	revid:vasil.dimov@stripped
@@ -4049,22 +4049,9 @@ ha_innobase::open(
 		DBUG_RETURN(1);
 	}
 
-	/* Create buffers for packing the fields of a record. Why
-	table->reclength did not work here? Obviously, because char
-	fields when packed actually became 1 byte longer, when we also
-	stored the string length as the first byte. */
-
-	upd_and_key_val_buff_len =
-				table->s->reclength + table->s->max_key_length
-							+ MAX_REF_PARTS * 3;
-	if (!(uchar*) my_multi_malloc(MYF(MY_WME),
-			&upd_buff, upd_and_key_val_buff_len,
-			&key_val_buff, upd_and_key_val_buff_len,
-			NullS)) {
-		free_share(share);
-
-		DBUG_RETURN(1);
-	}
+	/* Will be allocated if it is needed in ::update_row() */
+	upd_buf = NULL;
+	upd_buf_size = 0;
 
 	/* We look for pattern #P# to see if the table is partitioned
 	MySQL table. The retry logic for partitioned tables is a
@@ -4177,7 +4164,6 @@ retry:
 				"how you can resolve the problem.\n",
 				norm_name);
 		free_share(share);
-		my_free(upd_buff);
 		my_errno = ENOENT;
 
 		DBUG_RETURN(HA_ERR_NO_SUCH_TABLE);
@@ -4197,7 +4183,6 @@ table_opened:
 				"how you can resolve the problem.\n",
 				norm_name);
 		free_share(share);
-		my_free(upd_buff);
 		my_errno = ENOENT;
 
 		dict_table_close(ib_table, FALSE);
@@ -4399,7 +4384,13 @@ ha_innobase::close()
 
 	row_prebuilt_free(prebuilt, FALSE);
 
-	my_free(upd_buff);
+	if (upd_buf != NULL) {
+		ut_ad(upd_buf_size != 0);
+		my_free(upd_buf);
+		upd_buf = NULL;
+		upd_buf_size = 0;
+	}
+
 	free_share(share);
 
 	MONITOR_INC(MONITOR_TABLE_CLOSE);
@@ -5911,6 +5902,23 @@ ha_innobase::update_row(
 
 	ut_a(prebuilt->trx == trx);
 
+	if (upd_buf == NULL) {
+		ut_ad(upd_buf_size == 0);
+
+		/* Create a buffer for packing the fields of a record. Why
+		table->reclength did not work here? Obviously, because char
+		fields when packed actually became 1 byte longer, when we also
+		stored the string length as the first byte. */
+
+		upd_buf_size = table->s->reclength + table->s->max_key_length
+			+ MAX_REF_PARTS * 3;
+		upd_buf = (uchar*) my_malloc(upd_buf_size, MYF(MY_WME));
+		if (upd_buf == NULL) {
+			upd_buf_size = 0;
+			DBUG_RETURN(HA_ERR_OUT_OF_MEM);
+		}
+	}
+
 	ha_statistic_increment(&SSV::ha_update_count);
 
 	if (table->timestamp_field_type & TIMESTAMP_AUTO_SET_ON_UPDATE)
@@ -5923,11 +5931,10 @@ ha_innobase::update_row(
 	}
 
 	/* Build an update vector from the modified fields in the rows
-	(uses upd_buff of the handle) */
+	(uses upd_buf of the handle) */
 
 	calc_row_difference(uvect, (uchar*) old_row, new_row, table,
-			upd_buff, (ulint)upd_and_key_val_buff_len,
-			prebuilt, user_thd);
+			    upd_buf, upd_buf_size, prebuilt, user_thd);
 
 	/* This is not a delete */
 	prebuilt->upd_node->is_delete = FALSE;
@@ -6307,8 +6314,6 @@ ha_innobase::index_read(
 
 		row_sel_convert_mysql_key_to_innobase(
 			prebuilt->search_tuple,
-			(byte*) key_val_buff,
-			(ulint)upd_and_key_val_buff_len,
 			index,
 			(byte*) key_ptr,
 			(ulint) key_len,
@@ -8220,12 +8225,6 @@ ha_innobase::records_in_range(
 {
 	KEY*		key;
 	dict_index_t*	index;
-	uchar*		key_val_buff2	= (uchar*) my_malloc(
-						  table->s->reclength
-					+ table->s->max_key_length + 100,
-								MYF(MY_FAE));
-	ulint		buff2_len = table->s->reclength
-					+ table->s->max_key_length + 100;
 	dtuple_t*	range_start;
 	dtuple_t*	range_end;
 	ib_int64_t	n_rows;
@@ -8276,17 +8275,14 @@ ha_innobase::records_in_range(
 	dict_index_copy_types(range_end, index, key->key_parts);
 
 	row_sel_convert_mysql_key_to_innobase(
-				range_start, (byte*) key_val_buff,
-				(ulint)upd_and_key_val_buff_len,
-				index,
+				range_start, index,
 				(byte*) (min_key ? min_key->key :
 					 (const uchar*) 0),
 				(ulint) (min_key ? min_key->length : 0),
 				prebuilt->trx);
 
 	row_sel_convert_mysql_key_to_innobase(
-				range_end, (byte*) key_val_buff2,
-				buff2_len, index,
+				range_end, index,
 				(byte*) (max_key ? max_key->key :
 					 (const uchar*) 0),
 				(ulint) (max_key ? max_key->length : 0),
@@ -8310,7 +8306,6 @@ ha_innobase::records_in_range(
 	mem_heap_free(heap);
 
 func_exit:
-	my_free(key_val_buff2);
 
 	prebuilt->trx->op_info = (char*)"";
 

=== modified file 'storage/innobase/handler/ha_innodb.h'
--- a/storage/innobase/handler/ha_innodb.h	revid:vasil.dimov@stripped
+++ b/storage/innobase/handler/ha_innodb.h	revid:vasil.dimov@stripped
@@ -76,13 +76,8 @@ class ha_innobase: public handler
 	INNOBASE_SHARE*	share;		/*!< information for MySQL
 					table locking */
 
-	uchar*		upd_buff;	/*!< buffer used in updates */
-	uchar*		key_val_buff;	/*!< buffer used in converting
-					search key values from MySQL format
-					to Innodb format */
-	ulong		upd_and_key_val_buff_len;
-					/* the length of each of the previous
-					two buffers */
+	uchar*		upd_buf;	/*!< buffer used in updates */
+	ulint		upd_buf_size;	/*!< the size of upd_buff in bytes */
 	Table_flags	int_table_flags;
 	uint		primary_key;
 	ulong		start_of_scan;	/*!< this is set to 1 when we are

=== modified file 'storage/innobase/include/row0sel.h'
--- a/storage/innobase/include/row0sel.h	revid:vasil.dimov@stripped
+++ b/storage/innobase/include/row0sel.h	revid:vasil.dimov@stripped
@@ -127,9 +127,6 @@ row_sel_convert_mysql_key_to_innobase(
 					NOTE: we assume that the type info
 					in the tuple is already according
 					to index! */
-	byte*		buf,		/*!< in: buffer to use in field
-					conversions */
-	ulint		buf_len,	/*!< in: buffer length */
 	dict_index_t*	index,		/*!< in: index of the key value */
 	const byte*	key_ptr,	/*!< in: MySQL key value */
 	ulint		key_len,	/*!< in: MySQL key value length */

=== modified file 'storage/innobase/row/row0sel.c'
--- a/storage/innobase/row/row0sel.c	revid:vasil.dimov@stripped
+++ b/storage/innobase/row/row0sel.c	revid:vasil.dimov@stripped
@@ -2309,15 +2309,14 @@ row_sel_convert_mysql_key_to_innobase(
 					NOTE: we assume that the type info
 					in the tuple is already according
 					to index! */
-	byte*		buf,		/*!< in: buffer to use in field
-					conversions */
-	ulint		buf_len,	/*!< in: buffer length */
 	dict_index_t*	index,		/*!< in: index of the key value */
 	const byte*	key_ptr,	/*!< in: MySQL key value */
 	ulint		key_len,	/*!< in: MySQL key value length */
 	trx_t*		trx)		/*!< in: transaction */
 {
-	byte*		original_buf	= buf;
+	byte		original_buf[REC_VERSION_56_MAX_INDEX_COL_LEN];
+	ulint		buf_len = sizeof(original_buf);
+	byte*		buf = original_buf;
 	const byte*	original_key_ptr = key_ptr;
 	dict_field_t*	field;
 	dfield_t*	dfield;
@@ -2442,6 +2441,7 @@ row_sel_convert_mysql_key_to_innobase(
 		/* Storing may use at most data_len bytes of buf */
 
 		if (UNIV_LIKELY(!is_null)) {
+			ut_a(buf + data_len <= original_buf + buf_len);
 			row_mysql_store_col_in_innobase_format(
 				dfield, buf,
 				FALSE, /* MySQL key value format col */

No bundle (reason: useless for push emails).
Thread
bzr push into mysql-trunk branch (vasil.dimov:3568 to 3569) Bug#11764622vasil.dimov11 Nov