List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:November 19 2010 12:45pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch
(magne.mahre:3328) Bug#49177
View as plain text  
Hi Magne,

taking into account "WL#4305 - storage-engine private data area
per physical table", do you think we still should proceed with
this fix?

Mattias: is there any ETA for WL#4305?

Regards,
Sergey

On Thu, Oct 21, 2010 at 10:52:02AM +0000, Magne Mahre wrote:
> #At file:///export/home/tmp/x/mysql-next-mr-bugfixing-49177/ based on
> revid:anitha.gopi@stripped
> 
>  3328 Magne Mahre	2010-10-21
>       Bug#49177 MyISAM share list scalability problems
>             
>       (Note: MyISAM issue only)
>                   
>       As the number of open tables is increased, table lookup 
>       (testing if a table is already open)  and  (in particular) 
>       the case when a table is not open, became increasingly more 
>       expensive.
>                   
>       The problem was caused by the open table lookup mechanism, 
>       which was based on traversing a linked list comparing the 
>       file names. 
>             
>       As the list was replaced by a hash table, the lookup 
>       time dropped significantly when used on systems with
>       a large number of open tables.
>       
>       The performance on smaller installations remains in the same
>       ballpark.  Instead of growing exponentially, the lookup time
>       now grows more or less linearly, at least in the ballpark
>       with less than 100 000 open tables.
>      @ storage/myisam/myisamchk.c
>         Need to initialize MyISAM open table hash
>      @ storage/myisam/myisampack.c
>         Need to initialize MyISAM open table hash
> 
>     modified:
>       storage/myisam/ha_myisam.cc
>       storage/myisam/mi_close.c
>       storage/myisam/mi_dbug.c
>       storage/myisam/mi_keycache.c
>       storage/myisam/mi_open.c
>       storage/myisam/mi_panic.c
>       storage/myisam/mi_static.c
>       storage/myisam/myisamchk.c
>       storage/myisam/myisamdef.h
>       storage/myisam/myisampack.c
> === modified file 'storage/myisam/ha_myisam.cc'
> --- a/storage/myisam/ha_myisam.cc	2010-07-13 17:29:44 +0000
> +++ b/storage/myisam/ha_myisam.cc	2010-10-21 10:51:58 +0000
> @@ -2099,6 +2099,10 @@ static int myisam_init(void *p)
>    myisam_hton->create= myisam_create_handler;
>    myisam_hton->panic= myisam_panic;
>    myisam_hton->flags= HTON_CAN_RECREATE | HTON_SUPPORT_LOG_TABLES;
> +
> +  if (mi_init_open_table_hash(table_cache_size))
> +    return 1;
> +
>    return 0;
>  }
>  
> 
> === modified file 'storage/myisam/mi_close.c'
> --- a/storage/myisam/mi_close.c	2010-07-08 21:20:08 +0000
> +++ b/storage/myisam/mi_close.c	2010-10-21 10:51:58 +0000
> @@ -54,7 +54,9 @@ int mi_close(register MI_INFO *info)
>      info->opt_flag&= ~(READ_CACHE_USED | WRITE_CACHE_USED);
>    }
>    flag= !--share->reopen;
> -  myisam_open_list=list_delete(myisam_open_list,&info->open_list);
> +
> +  my_hash_delete(&myisam_open_table_hash, (uchar *) info);
> +
>    mysql_mutex_unlock(&share->intern_lock);
>  
>    my_free(mi_get_rec_buff_ptr(info, info->rec_buff));
> 
> === modified file 'storage/myisam/mi_dbug.c'
> --- a/storage/myisam/mi_dbug.c	2010-06-07 12:05:34 +0000
> +++ b/storage/myisam/mi_dbug.c	2010-10-21 10:51:58 +0000
> @@ -181,14 +181,14 @@ void _mi_print_key(FILE *stream, registe
>  my_bool check_table_is_closed(const char *name, const char *where)
>  {
>    char filename[FN_REFLEN];
> -  LIST *pos;
> +  uint idx;
>    DBUG_ENTER("check_table_is_closed");
>  
>    (void) fn_format(filename,name,"",MI_NAME_IEXT,4+16+32);
>    mysql_mutex_lock(&THR_LOCK_myisam);
> -  for (pos=myisam_open_list ; pos ; pos=pos->next)
> +  for (idx= 0; idx < myisam_open_table_hash.records; ++idx)
>    {
> -    MI_INFO *info=(MI_INFO*) pos->data;
> +    MI_INFO *info=(MI_INFO*) my_hash_element(&myisam_open_table_hash, idx);
>      MYISAM_SHARE *share=info->s;
>      if (!strcmp(share->unique_file_name,filename))
>      {
> 
> === modified file 'storage/myisam/mi_keycache.c'
> --- a/storage/myisam/mi_keycache.c	2009-12-05 01:26:15 +0000
> +++ b/storage/myisam/mi_keycache.c	2010-10-21 10:51:58 +0000
> @@ -137,16 +137,16 @@ int mi_assign_to_key_cache(MI_INFO *info
>  void mi_change_key_cache(KEY_CACHE *old_key_cache,
>  			 KEY_CACHE *new_key_cache)
>  {
> -  LIST *pos;
> +  uint idx;
>    DBUG_ENTER("mi_change_key_cache");
>  
>    /*
>      Lock list to ensure that no one can close the table while we manipulate it
>    */
>    mysql_mutex_lock(&THR_LOCK_myisam);
> -  for (pos=myisam_open_list ; pos ; pos=pos->next)
> +  for (idx= 0; idx < myisam_open_table_hash.records; ++idx)
>    {
> -    MI_INFO *info= (MI_INFO*) pos->data;
> +    MI_INFO *info=(MI_INFO*) my_hash_element(&myisam_open_table_hash, idx);
>      MYISAM_SHARE *share= info->s;
>      if (share->key_cache == old_key_cache)
>        mi_assign_to_key_cache(info, (ulonglong) ~0, new_key_cache);
> 
> === modified file 'storage/myisam/mi_open.c'
> --- a/storage/myisam/mi_open.c	2010-07-23 20:59:42 +0000
> +++ b/storage/myisam/mi_open.c	2010-10-21 10:51:58 +0000
> @@ -36,24 +36,76 @@ if (pos > end_pos)             \
>    goto err;                    \
>  }
>  
> +/*
> +  Get the value used as hash key (helper function for the 
> +  open table hash).  Function is used as a callback 
> +  from the hash table
> +*/
> +static uchar *mi_open_table_hash_key(const uchar *record, size_t *length,
> +                                     my_bool not_used __attribute__((unused)))
> +{
> +  MI_INFO *info= (MI_INFO *) record;
> +  *length= info->s->unique_name_length;
> +  return (uchar*) info->s->unique_file_name;
> +}
>  
> -/******************************************************************************
> -** Return the shared struct if the table is already open.
> -** In MySQL the server will handle version issues.
> -******************************************************************************/
> +/**
> +  Initialize open table hash
> + 
> +  Function is normally called from myisam_init
> +  with the system variable table_cache_size used
> +  as hash_size.
> +
> +  @param[in]      hash_size Initial has size (elements)
> +  @return         inidicates success or failure of initialization
> +    @retval 0 success
> +    @retval 1 failure
>  
> +*/
> +int mi_init_open_table_hash(ulong hash_size)
> +{
> +  if (hash_size == 0)
> +    hash_size= 32; /* default hash size */
> +
> +  if (my_hash_init(&myisam_open_table_hash, &my_charset_filename, 
> +                   hash_size, 0, 0, mi_open_table_hash_key, 0, 0))
> +    return 1; /* error */
> +  
> +  return 0;
> +}
> +
> +
> +/**
> +  Retrieve the shared struct if the table is already
> +  open (i.e  in the open table hash)
> +
> +  @param[in] filename table file name
> +  @return    shared struct, 0 if not in the cache
> +*/
>  MI_INFO *test_if_reopen(char *filename)
>  {
> -  LIST *pos;
> +  HASH_SEARCH_STATE current_record;
> +  int len= strlen(filename);
>  
> -  for (pos=myisam_open_list ; pos ; pos=pos->next)
> +  MI_INFO *info= (MI_INFO*) my_hash_first(&myisam_open_table_hash, 
> +                                          (uchar *) filename, 
> +                                          len, &current_record);
> +  /*
> +    There might be more than one instance of a table share for
> +    a given table in the hash table.  We're interested in the one with 
> +    last_version set, so we iterate until we find it
> +  */
> +  while (info)
>    {
> -    MI_INFO *info=(MI_INFO*) pos->data;
>      MYISAM_SHARE *share=info->s;
> -    if (!strcmp(share->unique_file_name,filename) &&
> share->last_version)
> -      return info;
> +    if (share->last_version)
> +      break;
> +    
> +    info= (MI_INFO*) my_hash_next(&myisam_open_table_hash, 
> +                                  (uchar *) filename, 
> +                                  len, &current_record);
>    }
> -  return 0;
> +  return info;
>  }
>  
>  
> @@ -628,8 +680,9 @@ MI_INFO *mi_open(const char *name, int m
>  #ifdef THREAD
>    thr_lock_data_init(&share->lock,&m_info->lock,(void*) m_info);
>  #endif
> -  m_info->open_list.data=(void*) m_info;
> -  myisam_open_list=list_add(myisam_open_list,&m_info->open_list);
> +
> +  if (my_hash_insert(&myisam_open_table_hash, (uchar *) m_info))
> +    goto err;
>  
>    mysql_mutex_unlock(&THR_LOCK_myisam);
>  
> 
> === modified file 'storage/myisam/mi_panic.c'
> --- a/storage/myisam/mi_panic.c	2009-12-05 01:26:15 +0000
> +++ b/storage/myisam/mi_panic.c	2010-10-21 10:51:58 +0000
> @@ -26,90 +26,102 @@
>  int mi_panic(enum ha_panic_function flag)
>  {
>    int error=0;
> -  LIST *list_element,*next_open;
>    MI_INFO *info;
> +  uint idx;
>    DBUG_ENTER("mi_panic");
>  
>    mysql_mutex_lock(&THR_LOCK_myisam);
> -  for (list_element=myisam_open_list ; list_element ; list_element=next_open)
> +  if (my_hash_inited(&myisam_open_table_hash))
>    {
> -    next_open=list_element->next;		/* Save if close */
> -    info=(MI_INFO*) list_element->data;
> -    switch (flag) {
> -    case HA_PANIC_CLOSE:
> -      mysql_mutex_unlock(&THR_LOCK_myisam);     /* Not exactly right... */
> -      if (mi_close(info))
> -	error=my_errno;
> -      mysql_mutex_lock(&THR_LOCK_myisam);
> -      break;
> -    case HA_PANIC_WRITE:		/* Do this to free databases */
> -#ifdef CANT_OPEN_FILES_TWICE
> -      if (info->s->options & HA_OPTION_READ_ONLY_DATA)
> -	break;
> -#endif
> -      if (flush_key_blocks(info->s->key_cache, info->s->kfile,
> FLUSH_RELEASE))
> -	error=my_errno;
> -      if (info->opt_flag & WRITE_CACHE_USED)
> -	if (flush_io_cache(&info->rec_cache))
> -	  error=my_errno;
> -      if (info->opt_flag & READ_CACHE_USED)
> +    if (flag == HA_PANIC_CLOSE)
> +    {
> +      while (myisam_open_table_hash.records)
>        {
> -	if (flush_io_cache(&info->rec_cache))
> -	  error=my_errno;
> -	reinit_io_cache(&info->rec_cache,READ_CACHE,0,
> -		       (pbool) (info->lock_type != F_UNLCK),1);
> +        /* 
> +           As long as there are records in the hash, fetch the
> +           first, and close it.
> +        */
> +        info=(MI_INFO*) my_hash_element(&myisam_open_table_hash, 0);
> +        mysql_mutex_unlock(&THR_LOCK_myisam);     /* Not exactly right... */
> +        if (mi_close(info))
> +          error= my_errno;
> +        mysql_mutex_lock(&THR_LOCK_myisam);
>        }
> -      if (info->lock_type != F_UNLCK && ! info->was_locked)
> +      (void) mi_log(0);				/* Close log if neaded */
> +      ft_free_stopwords();
> +    }
> +    else
> +    {
> +      for (idx= 0; idx < myisam_open_table_hash.records; ++idx)
>        {
> -	info->was_locked=info->lock_type;
> -	if (mi_lock_database(info,F_UNLCK))
> -	  error=my_errno;
> -      }
> +        info=(MI_INFO*) my_hash_element(&myisam_open_table_hash, idx);
> +        switch (flag) {
> +        case HA_PANIC_CLOSE: break;      /* To eliminate warning.  Handled above */
> +        case HA_PANIC_WRITE:		/* Do this to free databases */
>  #ifdef CANT_OPEN_FILES_TWICE
> -      if (info->s->kfile >= 0 &&
> mysql_file_close(info->s->kfile, MYF(0)))
> -	error = my_errno;
> -      if (info->dfile >= 0 && mysql_file_close(info->dfile,
> MYF(0)))
> -	error = my_errno;
> -      info->s->kfile=info->dfile= -1;	/* Files aren't open anymore */
> -      break;
> +          if (info->s->options & HA_OPTION_READ_ONLY_DATA)
> +            break;
>  #endif
> -    case HA_PANIC_READ:			/* Restore to before WRITE */
> +          if (flush_key_blocks(info->s->key_cache, info->s->kfile,
> FLUSH_RELEASE))
> +            error=my_errno;
> +          if (info->opt_flag & WRITE_CACHE_USED)
> +            if (flush_io_cache(&info->rec_cache))
> +              error=my_errno;
> +          if (info->opt_flag & READ_CACHE_USED)
> +          {
> +            if (flush_io_cache(&info->rec_cache))
> +              error=my_errno;
> +            reinit_io_cache(&info->rec_cache,READ_CACHE,0,
> +                            (pbool) (info->lock_type != F_UNLCK),1);
> +          }
> +          if (info->lock_type != F_UNLCK && ! info->was_locked)
> +          {
> +            info->was_locked=info->lock_type;
> +            if (mi_lock_database(info,F_UNLCK))
> +              error=my_errno;
> +          }
>  #ifdef CANT_OPEN_FILES_TWICE
> -      {					/* Open closed files */
> -	char name_buff[FN_REFLEN];
> -	if (info->s->kfile < 0)
> -          if ((info->s->kfile= mysql_file_open(mi_key_file_kfile,
> -                                               fn_format(name_buff,
> -                                                         info->filename, "",
> -                                                         N_NAME_IEXT, 4),
> -                                               info->mode, MYF(MY_WME))) < 0)
> -	    error = my_errno;
> -	if (info->dfile < 0)
> -	{
> -          if ((info->dfile= mysql_file_open(mi_key_file_dfile,
> -                                            fn_format(name_buff,
> -                                                      info->filename, "",
> -                                                      N_NAME_DEXT, 4),
> -                                            info->mode, MYF(MY_WME))) < 0)
> -	    error = my_errno;
> -	  info->rec_cache.file=info->dfile;
> -	}
> -      }
> +          if (info->s->kfile >= 0 &&
> mysql_file_close(info->s->kfile, MYF(0)))
> +            error = my_errno;
> +          if (info->dfile >= 0 && mysql_file_close(info->dfile,
> MYF(0)))
> +            error = my_errno;
> +          info->s->kfile=info->dfile= -1;	/* Files aren't open anymore */
> +          break;
>  #endif
> -      if (info->was_locked)
> -      {
> -	if (mi_lock_database(info, info->was_locked))
> -	  error=my_errno;
> -	info->was_locked=0;
> +        case HA_PANIC_READ:			/* Restore to before WRITE */
> +#ifdef CANT_OPEN_FILES_TWICE
> +          {					/* Open closed files */
> +            char name_buff[FN_REFLEN];
> +            if (info->s->kfile < 0)
> +              if ((info->s->kfile= mysql_file_open(mi_key_file_kfile,
> +                                                   fn_format(name_buff,
> +                                                             info->filename, "",
> +                                                             N_NAME_IEXT, 4),
> +                                                   info->mode, MYF(MY_WME))) <
> 0)
> +                error = my_errno;
> +            if (info->dfile < 0)
> +            {
> +              if ((info->dfile= mysql_file_open(mi_key_file_dfile,
> +                                                fn_format(name_buff,
> +                                                          info->filename, "",
> +                                                          N_NAME_DEXT, 4),
> +                                                info->mode, MYF(MY_WME))) <
> 0)
> +                error = my_errno;
> +              info->rec_cache.file=info->dfile;
> +            }
> +          }
> +#endif
> +          if (info->was_locked)
> +          {
> +            if (mi_lock_database(info, info->was_locked))
> +              error=my_errno;
> +            info->was_locked=0;
> +          }
> +          break;
> +        }
>        }
> -      break;
>      }
>    }
> -  if (flag == HA_PANIC_CLOSE)
> -  {
> -    (void) mi_log(0);				/* Close log if neaded */
> -    ft_free_stopwords();
> -  }
>    mysql_mutex_unlock(&THR_LOCK_myisam);
>    if (!error)
>      DBUG_RETURN(0);
> 
> === modified file 'storage/myisam/mi_static.c'
> --- a/storage/myisam/mi_static.c	2010-08-05 12:34:19 +0000
> +++ b/storage/myisam/mi_static.c	2010-10-21 10:51:58 +0000
> @@ -22,7 +22,7 @@
>  #include "myisamdef.h"
>  #endif
>  
> -LIST	*myisam_open_list=0;
> +
>  uchar	myisam_file_magic[]=
>  { (uchar) 254, (uchar) 254,'\007', '\001', };
>  uchar	myisam_pack_file_magic[]=
> @@ -40,6 +40,9 @@ ulong myisam_concurrent_insert= 0;
>  ulonglong myisam_max_temp_length= MAX_FILE_SIZE;
>  ulong    myisam_data_pointer_size=4;
>  ulonglong    myisam_mmap_size= SIZE_T_MAX, myisam_mmap_used= 0;
> +HASH     myisam_open_table_hash;
> +
> +
>  
>  static int always_valid(const char *filename __attribute__((unused)))
>  {
> 
> === modified file 'storage/myisam/myisamchk.c'
> --- a/storage/myisam/myisamchk.c	2010-07-23 20:16:29 +0000
> +++ b/storage/myisam/myisamchk.c	2010-10-21 10:51:58 +0000
> @@ -88,6 +88,13 @@ int main(int argc, char **argv)
>    get_options(&argc,(char***) &argv);
>    myisam_quick_table_bits=decode_bits;
>    error=0;
> +
> +  if (mi_init_open_table_hash(0))
> +  {
> +    fprintf(stderr, "Can't initialize MyISAM storage engine\n");
> +    exit(-1);
> +  }
> +
>    while (--argc >= 0)
>    {
>      int new_error=myisamchk(&check_param, *(argv++));
> 
> === modified file 'storage/myisam/myisamdef.h'
> --- a/storage/myisam/myisamdef.h	2010-07-26 11:34:07 +0000
> +++ b/storage/myisam/myisamdef.h	2010-10-21 10:51:58 +0000
> @@ -25,6 +25,7 @@
>  #include <my_no_pthread.h>
>  #endif
>  #include <mysql/psi/mysql_file.h>
> +#include "hash.h"
>  
>  /* undef map from my_nosys; We need test-if-disk full */
>  #if defined(my_write)
> @@ -287,7 +288,6 @@ struct st_myisam_info {
>    uint	data_changed;			/* Somebody has changed data */
>    uint	save_update;			/* When using KEY_READ */
>    int	save_lastinx;
> -  LIST	open_list;
>    IO_CACHE rec_cache;			/* When cacheing records */
>    uint  preload_buff_size;              /* When preloading indexes */
>    myf lock_wait;			/* is 0 or MY_DONT_WAIT */
> @@ -477,7 +477,7 @@ extern mysql_mutex_t THR_LOCK_myisam;
>  
>  	/* Some extern variables */
>  
> -extern LIST *myisam_open_list;
> +extern HASH myisam_open_table_hash;
>  extern uchar myisam_file_magic[], myisam_pack_file_magic[];
>  extern uint myisam_read_vec[], myisam_readnext_vec[];
>  extern uint myisam_quick_table_bits;
> @@ -759,6 +759,7 @@ my_bool mi_check_status(void* param);
>  void mi_disable_non_unique_index(MI_INFO *info, ha_rows rows);
>  
>  extern MI_INFO *test_if_reopen(char *filename);
> +extern int mi_init_open_table_hash(ulong size);
>  my_bool check_table_is_closed(const char *name, const char *where);
>  int mi_open_datafile(MI_INFO *info, MYISAM_SHARE *share, const char *orn_name,
>                       File file_to_dup);
> 
> === modified file 'storage/myisam/myisampack.c'
> --- a/storage/myisam/myisampack.c	2010-07-23 20:17:55 +0000
> +++ b/storage/myisam/myisampack.c	2010-10-21 10:51:58 +0000
> @@ -210,6 +210,13 @@ int main(int argc, char **argv)
>    if (load_defaults("my",load_default_groups,&argc,&argv))
>      exit(1);
>  
> +  if (mi_init_open_table_hash(0))
> +  {
> +    fputs("Can't initialize MyISAM storage engine", stderr);
> +    exit(1);
> +  }
> +    
> +
>    default_argv= argv;
>    get_options(&argc,&argv);
>  
> 


> 
> -- 
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=1


-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
Thread
bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3328) Bug#49177Magne Mahre21 Oct
  • Re: bzr commit into mysql-next-mr-bugfixing branch(magne.mahre:3328) Bug#49177Sergey Vojtovich19 Nov
    • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3328)Bug#49177Mattias Jonsson19 Nov
      • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3328)Bug#49177Magne Mæhre19 Nov
        • Re: bzr commit into mysql-next-mr-bugfixing branch(magne.mahre:3328) Bug#49177Sergey Vojtovich19 Nov
          • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3328)Bug#49177Magne Mæhre19 Nov
            • Re: bzr commit into mysql-next-mr-bugfixing branch(magne.mahre:3328) Bug#49177Sergey Vojtovich19 Nov