Description:
I found a very serious performance issue in the mysql-next-mr
tree. It's quite likely to also exist in the 5.1.44 release.
I managed to drill down to the following patch which fixes the
performance issue.
It improves the performance from 9500tps to 17700 tps for readonly
sysbench.
I am still puzzled about this, it's not the normal issue I am used
to finding.
I do however have a theory about what causes the issue.
The problem I think is changes like this one:
- char buff[FN_REFLEN+1];
+ char buff[FN_REFLEN];
Declaring a buffer which isn't aligned on a word boundary means
that things will still work, but the memcpy and other instructions
will have to use much less optimised instructions which move 1 byte
at a time instead of words at a time.
Solution here if FN_REFLEN isn't enough is to use an word-aligned
size (I think should be even aligned at 8 byte boundary but at least
at 4 bytes, but 8 bytes would be great here. Even better if buffer
is declared at top of declarations to ensure it is aligned on 8byte
boundary.
The patch I attached here fixes mysql-next-mr.
I think it is necessary to look for all kinds of issues like this,
there might be more of them that haven't been discovered before.
I'll make another round of experiments tomorrow to boil down even
further which part of this patch which is responsible for the
performance issue.
How to repeat:
readonly sysbench
Suggested fix:
=== modified file 'client/mysql.cc'
--- client/mysql.cc 2010-02-06 10:28:06 +0000
+++ client/mysql.cc 2010-02-17 22:28:56 +0000
@@ -3612,8 +3612,7 @@ print_table_data_vertically(MYSQL_RES *r
for (uint off=0; off < mysql_num_fields(result); off++)
{
field= mysql_fetch_field(result);
- if (column_names)
- tee_fprintf(PAGER, "%*s: ",(int) max_length,field->name);
+ tee_fprintf(PAGER, "%*s: ",(int) max_length,field->name);
if (cur[off])
{
unsigned int i;
=== modified file 'client/mysqldump.c'
--- client/mysqldump.c 2010-02-06 10:28:06 +0000
+++ client/mysqldump.c 2010-02-17 22:28:56 +0000
@@ -840,7 +840,7 @@ get_one_option(int optid, const struct m
&err_ptr, &err_len);
if (err_len)
{
- strmake(buff, err_ptr, min(sizeof(buff) - 1, err_len));
+ strmake(buff, err_ptr, min(sizeof(buff), err_len));
fprintf(stderr, "Invalid mode to --compatible: %s\n", buff);
exit(1);
}
@@ -4630,7 +4630,7 @@ static ulong find_set(TYPELIB *lib, cons
for (; pos != end && *pos != ','; pos++) ;
var_len= (uint) (pos - start);
- strmake(buff, start, min(sizeof(buff) - 1, var_len));
+ strmake(buff, start, min(sizeof(buff), var_len));
find= find_type(buff, lib, var_len);
if (!find)
{
=== modified file 'include/config-win.h'
--- include/config-win.h 2010-01-18 20:19:19 +0000
+++ include/config-win.h 2010-02-17 22:28:56 +0000
@@ -190,7 +190,7 @@ typedef SSIZE_T ssize_t;
#define isnan(X) _isnan(X)
#define finite(X) _finite(X)
-#ifndef MYSQL_CLIENT_NO_THREADS
+#ifndef UNDEF_THREAD_HACK
#define THREAD
#endif
#define VOID_SIGHANDLER
=== modified file 'libmysql/libmysql.c'
--- libmysql/libmysql.c 2010-01-18 20:19:19 +0000
+++ libmysql/libmysql.c 2010-02-17 22:28:56 +0000
@@ -409,10 +409,7 @@ my_bool STDCALL mysql_change_user(MYSQL
if (!passwd)
passwd="";
- /*
- Store user into the buffer.
- Advance position as strmake returns a pointer to the closing NUL.
- */
+ /* Store user into the buffer */
end= strmake(end, user, USERNAME_LENGTH) + 1;
/* write scrambled password according to server capabilities */
@@ -900,7 +897,7 @@ mysql_list_fields(MYSQL *mysql, const ch
{
MYSQL_RES *result;
MYSQL_FIELD *fields;
- char buff[258],*end;
+ char buff[257],*end;
DBUG_ENTER("mysql_list_fields");
DBUG_PRINT("enter",("table: '%s' wild: '%s'",table,wild ? wild : ""));
@@ -1906,7 +1903,7 @@ mysql_stmt_param_metadata(MYSQL_STMT *st
/* Store type of parameter in network buffer. */
-static void store_param_type(unsigned char **pos, MYSQL_BIND *param)
+static void store_param_type(char **pos, MYSQL_BIND *param)
{
uint typecode= param->buffer_type | (param->is_unsigned ? 32768 : 0);
int2store(*pos, typecode);
@@ -2185,7 +2182,7 @@ int cli_stmt_execute(MYSQL_STMT *stmt)
that is sent to the server.
*/
for (param= stmt->params; param < param_end ; param++)
- store_param_type(&net->write_pos, param);
+ store_param_type((char**) &net->write_pos, param);
}
for (param= stmt->params; param < param_end; param++)
=== modified file 'mysys/default.c'
--- mysys/default.c 2009-12-24 07:56:13 +0000
+++ mysys/default.c 2010-02-17 22:28:56 +0000
@@ -683,7 +683,7 @@ static int search_default_file_with_ext(
int recursion_level)
{
char name[FN_REFLEN + 10], buff[4096], curr_gr[4096], *ptr, *end, **tmp_ext;
- char *value, option[4096+2], tmp[FN_REFLEN];
+ char *value, option[4096], tmp[FN_REFLEN];
static const char includedir_keyword[]= "includedir";
static const char include_keyword[]= "include";
const int max_recursion_level= 10;
=== modified file 'mysys/mf_pack.c'
--- mysys/mf_pack.c 2009-12-18 18:44:24 +0000
+++ mysys/mf_pack.c 2010-02-17 22:28:56 +0000
@@ -245,7 +245,7 @@ my_bool my_use_symdir=0; /* Set this if
#ifdef USE_SYMDIR
void symdirget(char *dir)
{
- char buff[FN_REFLEN+1];
+ char buff[FN_REFLEN];
char *pos=strend(dir);
if (dir[0] && pos[-1] != FN_DEVCHAR && my_access(dir, F_OK))
{
@@ -257,7 +257,7 @@ void symdirget(char *dir)
*pos++=temp; *pos=0; /* Restore old filename */
if (file >= 0)
{
- if ((length= my_read(file, buff, sizeof(buff) - 1, MYF(0))) > 0)
+ if ((length= my_read(file, buff, sizeof(buff), MYF(0))) > 0)
{
for (pos= buff + length ;
pos > buff && (iscntrl(pos[-1]) || isspace(pos[-1])) ;
=== modified file 'sql/log.cc'
--- sql/log.cc 2010-02-15 11:16:49 +0000
+++ sql/log.cc 2010-02-17 22:28:56 +0000
@@ -2638,7 +2638,7 @@ const char *MYSQL_LOG::generate_name(con
{
char *p= fn_ext(log_name);
uint length= (uint) (p - log_name);
- strmake(buff, log_name, min(length, FN_REFLEN-1));
+ strmake(buff, log_name, min(length, FN_REFLEN));
return (const char*)buff;
}
return log_name;
@@ -3901,7 +3901,7 @@ int MYSQL_BIN_LOG::purge_logs_before_dat
if (stat_area.st_mtime < purge_time)
strmake(to_log,
log_info.log_file_name,
- sizeof(log_info.log_file_name) - 1);
+ sizeof(log_info.log_file_name));
else
break;
}
@@ -5370,11 +5370,11 @@ bool flush_error_log()
if (opt_error_log)
{
char err_renamed[FN_REFLEN], *end;
- end= strmake(err_renamed,log_error_file,FN_REFLEN-5);
+ end= strmake(err_renamed,log_error_file,FN_REFLEN-4);
strmov(end, "-old");
mysql_mutex_lock(&LOCK_error_log);
#ifdef __WIN__
- char err_temp[FN_REFLEN+5];
+ char err_temp[FN_REFLEN+4];
/*
On Windows is necessary a temporary file for to rename
the current error file.
=== modified file 'sql/sql_acl.cc'
--- sql/sql_acl.cc 2010-02-15 11:16:49 +0000
+++ sql/sql_acl.cc 2010-02-17 22:28:56 +0000
@@ -1044,7 +1044,7 @@ int acl_getroot(THD *thd, USER_RESOURCES
*mqh= acl_user->user_resource;
if (acl_user->host.hostname)
- strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME - 1);
+ strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME);
else
*sctx->priv_host= 0;
}
@@ -1145,7 +1145,7 @@ bool acl_getroot_no_password(Security_co
sctx->priv_user= acl_user->user ? user : (char *) "";
if (acl_user->host.hostname)
- strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME - 1);
+ strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME);
else
*sctx->priv_host= 0;
}
=== modified file 'sql/sql_connect.cc'
--- sql/sql_connect.cc 2010-01-25 16:48:45 +0000
+++ sql/sql_connect.cc 2010-02-17 22:28:56 +0000
@@ -727,7 +727,7 @@ static int check_connection(THD *thd)
ulong server_capabilites;
{
/* buff[] needs to big enough to hold the server_version variable */
- char buff[SERVER_VERSION_LENGTH + 1 + SCRAMBLE_LENGTH + 1 + 64];
+ char buff[SERVER_VERSION_LENGTH + SCRAMBLE_LENGTH + 64];
server_capabilites= CLIENT_BASIC_FLAGS;
if (opt_using_transactions)
=== modified file 'sql/sql_table.cc'
--- sql/sql_table.cc 2010-02-15 11:16:49 +0000
+++ sql/sql_table.cc 2010-02-17 22:28:56 +0000
@@ -2715,7 +2715,7 @@ mysql_prepare_create_table(THD *thd, HA_
!(sql_field->charset= get_charset_by_csname(sql_field->charset->csname,
MY_CS_BINSORT,MYF(0))))
{
- char tmp[65];
+ char tmp[64];
strmake(strmake(tmp, save_cs->csname, sizeof(tmp)-4),
STRING_WITH_LEN("_bin"));
my_error(ER_UNKNOWN_COLLATION, MYF(0), tmp);
Description: I found a very serious performance issue in the mysql-next-mr tree. It's quite likely to also exist in the 5.1.44 release. I managed to drill down to the following patch which fixes the performance issue. It improves the performance from 9500tps to 17700 tps for readonly sysbench. I am still puzzled about this, it's not the normal issue I am used to finding. I do however have a theory about what causes the issue. The problem I think is changes like this one: - char buff[FN_REFLEN+1]; + char buff[FN_REFLEN]; Declaring a buffer which isn't aligned on a word boundary means that things will still work, but the memcpy and other instructions will have to use much less optimised instructions which move 1 byte at a time instead of words at a time. Solution here if FN_REFLEN isn't enough is to use an word-aligned size (I think should be even aligned at 8 byte boundary but at least at 4 bytes, but 8 bytes would be great here. Even better if buffer is declared at top of declarations to ensure it is aligned on 8byte boundary. The patch I attached here fixes mysql-next-mr. I think it is necessary to look for all kinds of issues like this, there might be more of them that haven't been discovered before. I'll make another round of experiments tomorrow to boil down even further which part of this patch which is responsible for the performance issue. How to repeat: readonly sysbench Suggested fix: === modified file 'client/mysql.cc' --- client/mysql.cc 2010-02-06 10:28:06 +0000 +++ client/mysql.cc 2010-02-17 22:28:56 +0000 @@ -3612,8 +3612,7 @@ print_table_data_vertically(MYSQL_RES *r for (uint off=0; off < mysql_num_fields(result); off++) { field= mysql_fetch_field(result); - if (column_names) - tee_fprintf(PAGER, "%*s: ",(int) max_length,field->name); + tee_fprintf(PAGER, "%*s: ",(int) max_length,field->name); if (cur[off]) { unsigned int i; === modified file 'client/mysqldump.c' --- client/mysqldump.c 2010-02-06 10:28:06 +0000 +++ client/mysqldump.c 2010-02-17 22:28:56 +0000 @@ -840,7 +840,7 @@ get_one_option(int optid, const struct m &err_ptr, &err_len); if (err_len) { - strmake(buff, err_ptr, min(sizeof(buff) - 1, err_len)); + strmake(buff, err_ptr, min(sizeof(buff), err_len)); fprintf(stderr, "Invalid mode to --compatible: %s\n", buff); exit(1); } @@ -4630,7 +4630,7 @@ static ulong find_set(TYPELIB *lib, cons for (; pos != end && *pos != ','; pos++) ; var_len= (uint) (pos - start); - strmake(buff, start, min(sizeof(buff) - 1, var_len)); + strmake(buff, start, min(sizeof(buff), var_len)); find= find_type(buff, lib, var_len); if (!find) { === modified file 'include/config-win.h' --- include/config-win.h 2010-01-18 20:19:19 +0000 +++ include/config-win.h 2010-02-17 22:28:56 +0000 @@ -190,7 +190,7 @@ typedef SSIZE_T ssize_t; #define isnan(X) _isnan(X) #define finite(X) _finite(X) -#ifndef MYSQL_CLIENT_NO_THREADS +#ifndef UNDEF_THREAD_HACK #define THREAD #endif #define VOID_SIGHANDLER === modified file 'libmysql/libmysql.c' --- libmysql/libmysql.c 2010-01-18 20:19:19 +0000 +++ libmysql/libmysql.c 2010-02-17 22:28:56 +0000 @@ -409,10 +409,7 @@ my_bool STDCALL mysql_change_user(MYSQL if (!passwd) passwd=""; - /* - Store user into the buffer. - Advance position as strmake returns a pointer to the closing NUL. - */ + /* Store user into the buffer */ end= strmake(end, user, USERNAME_LENGTH) + 1; /* write scrambled password according to server capabilities */ @@ -900,7 +897,7 @@ mysql_list_fields(MYSQL *mysql, const ch { MYSQL_RES *result; MYSQL_FIELD *fields; - char buff[258],*end; + char buff[257],*end; DBUG_ENTER("mysql_list_fields"); DBUG_PRINT("enter",("table: '%s' wild: '%s'",table,wild ? wild : "")); @@ -1906,7 +1903,7 @@ mysql_stmt_param_metadata(MYSQL_STMT *st /* Store type of parameter in network buffer. */ -static void store_param_type(unsigned char **pos, MYSQL_BIND *param) +static void store_param_type(char **pos, MYSQL_BIND *param) { uint typecode= param->buffer_type | (param->is_unsigned ? 32768 : 0); int2store(*pos, typecode); @@ -2185,7 +2182,7 @@ int cli_stmt_execute(MYSQL_STMT *stmt) that is sent to the server. */ for (param= stmt->params; param < param_end ; param++) - store_param_type(&net->write_pos, param); + store_param_type((char**) &net->write_pos, param); } for (param= stmt->params; param < param_end; param++) === modified file 'mysys/default.c' --- mysys/default.c 2009-12-24 07:56:13 +0000 +++ mysys/default.c 2010-02-17 22:28:56 +0000 @@ -683,7 +683,7 @@ static int search_default_file_with_ext( int recursion_level) { char name[FN_REFLEN + 10], buff[4096], curr_gr[4096], *ptr, *end, **tmp_ext; - char *value, option[4096+2], tmp[FN_REFLEN]; + char *value, option[4096], tmp[FN_REFLEN]; static const char includedir_keyword[]= "includedir"; static const char include_keyword[]= "include"; const int max_recursion_level= 10; === modified file 'mysys/mf_pack.c' --- mysys/mf_pack.c 2009-12-18 18:44:24 +0000 +++ mysys/mf_pack.c 2010-02-17 22:28:56 +0000 @@ -245,7 +245,7 @@ my_bool my_use_symdir=0; /* Set this if #ifdef USE_SYMDIR void symdirget(char *dir) { - char buff[FN_REFLEN+1]; + char buff[FN_REFLEN]; char *pos=strend(dir); if (dir[0] && pos[-1] != FN_DEVCHAR && my_access(dir, F_OK)) { @@ -257,7 +257,7 @@ void symdirget(char *dir) *pos++=temp; *pos=0; /* Restore old filename */ if (file >= 0) { - if ((length= my_read(file, buff, sizeof(buff) - 1, MYF(0))) > 0) + if ((length= my_read(file, buff, sizeof(buff), MYF(0))) > 0) { for (pos= buff + length ; pos > buff && (iscntrl(pos[-1]) || isspace(pos[-1])) ; === modified file 'sql/log.cc' --- sql/log.cc 2010-02-15 11:16:49 +0000 +++ sql/log.cc 2010-02-17 22:28:56 +0000 @@ -2638,7 +2638,7 @@ const char *MYSQL_LOG::generate_name(con { char *p= fn_ext(log_name); uint length= (uint) (p - log_name); - strmake(buff, log_name, min(length, FN_REFLEN-1)); + strmake(buff, log_name, min(length, FN_REFLEN)); return (const char*)buff; } return log_name; @@ -3901,7 +3901,7 @@ int MYSQL_BIN_LOG::purge_logs_before_dat if (stat_area.st_mtime < purge_time) strmake(to_log, log_info.log_file_name, - sizeof(log_info.log_file_name) - 1); + sizeof(log_info.log_file_name)); else break; } @@ -5370,11 +5370,11 @@ bool flush_error_log() if (opt_error_log) { char err_renamed[FN_REFLEN], *end; - end= strmake(err_renamed,log_error_file,FN_REFLEN-5); + end= strmake(err_renamed,log_error_file,FN_REFLEN-4); strmov(end, "-old"); mysql_mutex_lock(&LOCK_error_log); #ifdef __WIN__ - char err_temp[FN_REFLEN+5]; + char err_temp[FN_REFLEN+4]; /* On Windows is necessary a temporary file for to rename the current error file. === modified file 'sql/sql_acl.cc' --- sql/sql_acl.cc 2010-02-15 11:16:49 +0000 +++ sql/sql_acl.cc 2010-02-17 22:28:56 +0000 @@ -1044,7 +1044,7 @@ int acl_getroot(THD *thd, USER_RESOURCES *mqh= acl_user->user_resource; if (acl_user->host.hostname) - strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME - 1); + strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME); else *sctx->priv_host= 0; } @@ -1145,7 +1145,7 @@ bool acl_getroot_no_password(Security_co sctx->priv_user= acl_user->user ? user : (char *) ""; if (acl_user->host.hostname) - strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME - 1); + strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME); else *sctx->priv_host= 0; } === modified file 'sql/sql_connect.cc' --- sql/sql_connect.cc 2010-01-25 16:48:45 +0000 +++ sql/sql_connect.cc 2010-02-17 22:28:56 +0000 @@ -727,7 +727,7 @@ static int check_connection(THD *thd) ulong server_capabilites; { /* buff[] needs to big enough to hold the server_version variable */ - char buff[SERVER_VERSION_LENGTH + 1 + SCRAMBLE_LENGTH + 1 + 64]; + char buff[SERVER_VERSION_LENGTH + SCRAMBLE_LENGTH + 64]; server_capabilites= CLIENT_BASIC_FLAGS; if (opt_using_transactions) === modified file 'sql/sql_table.cc' --- sql/sql_table.cc 2010-02-15 11:16:49 +0000 +++ sql/sql_table.cc 2010-02-17 22:28:56 +0000 @@ -2715,7 +2715,7 @@ mysql_prepare_create_table(THD *thd, HA_ !(sql_field->charset= get_charset_by_csname(sql_field->charset->csname, MY_CS_BINSORT,MYF(0)))) { - char tmp[65]; + char tmp[64]; strmake(strmake(tmp, save_cs->csname, sizeof(tmp)-4), STRING_WITH_LEN("_bin")); my_error(ER_UNKNOWN_COLLATION, MYF(0), tmp);