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

thanks for taking care of this bug. I think it is really important
to have this problem fixed, but please expect multiple commit/review
cycles.

This e-mail is just a quick-look at this patch, identifying obvious
problems.

Btw, I'm just curious if you did try different approaches (e.g. other
than hash) to fix this problem?

On Thu, Sep 30, 2010 at 09:29:56AM +0000, Magne Mahre wrote:
> #At file:///export/home/tmp/x/mysql-next-mr-bugfixing-49177/ based on
> revid:marc.alff@stripped
> 
>  3309 Magne Mahre	2010-09-30
>       Bug#49177 MyISAM share list scalability problems
>       
>       (Note: MyISAM issue only)
>             
>       As the table cache size is increased, cache lookups and 
>       (in particular) misses became increasingly more expensive.
>             
>       The problem was caused by the cache lookup mechanism, which
>       was based on traversing a linked list, of <table cache size>
>       length, comparing the file names. 
Strictly speaking the above statement is wrong. I cannot say that
myisam_open_list is cache. Imho cache is something that store
temporarily unused objects. But all objects in myisam_open_list
are in use.

Another inaccuracy is that linked list is in no way of
<table cache size>. Basically this list has no size, but it should
not grow bigger than <table cache size>.

>       As the list was replaced by a hash table, the lookup 
>       time dropped significantly when used on a large table cache.
>       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.
> 
>     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/myisamdef.h
> === 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-09-30 09:29:51 +0000
> @@ -642,7 +642,9 @@ ha_myisam::ha_myisam(handlerton *hton, T
>                    HA_CAN_INSERT_DELAYED | HA_CAN_BIT_FIELD | HA_CAN_RTREEKEYS |
>                    HA_HAS_RECORDS | HA_STATS_RECORDS_IS_EXACT),
>     can_enable_indexes(1)
> -{}
> +{
> +  mi_init_table_cache(table_cache_size);
> +}
Why? I think it is enough to initialize it in myisam_init(). Again, imho
it is not cache. May be myisam_open_table_hash?

...skip...
 
> === 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-09-30 09:29:51 +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_cache->records; ++idx)
>    {
> -    MI_INFO *info=(MI_INFO*) pos->data;
> +    MI_INFO *info=(MI_INFO*) my_hash_element(myisam_open_cache, idx);
>      MYISAM_SHARE *share=info->s;
>      if (!strcmp(share->unique_file_name,filename))
>      {
> 
No action needed yet. We could probably optimize this loop the same way
as we optimized test_if_reopent().

...skip...

> === 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-09-30 09:29:51 +0000
> @@ -36,24 +36,78 @@ if (pos > end_pos)             \
>    goto err;                    \
>  }
>  
> +/**
> + * Get the value used as hash key (helper function for the 
> + * table cache hash).  Function is used as a callback 
> + * from the hash table
> + *
> + */
Coding style:
When writing multi-line comments please put the '/*' and '*/' on their own
lines, put the '*/' below the '/*', put a line break and a two-space indent
after the '/*', do not use additional asterisks on the left of the comment.

There are a couple more same coding style issues. I'll skip them.

> +static uchar *mi_table_cache_key(const uchar *record, size_t *length,
> +                                 my_bool not_used __attribute__((unused)))
> +{
> +  MI_INFO *info= (MI_INFO *) record;
> +  *length= strlen(info->s->unique_file_name);
> +  return (uchar*) info->s->unique_file_name;
> +}
I'd avoid strlen() here. If not switching to LEX_STRING, can we at least
have MI_INFO::unique_file_name_length?


> +
> +/**
> + * Retrieve the shared struct if the table is already
> + * open (i.e  in the cache)
> + *
> + * @param 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)
> +  /* Initialize table cache on first call */
> +  if (!myisam_open_cache)
> +    mi_init_table_cache(0);
Ehm. A problem here: if we fail to initialize hash, myisam cannot operate.

...skip...
> @@ -629,7 +683,8 @@ MI_INFO *mi_open(const char *name, int m
>    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);
> +
> +  my_hash_insert(myisam_open_cache, (uchar *) m_info);
If we fail to insert new hash entry, we must fail.

>    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-09-30 09:29:51 +0000
> @@ -26,54 +26,55 @@
>  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 (myisam_open_cache)
>    {
> -    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 */
> +    for (idx= 0; idx < myisam_open_cache->records; ++idx)
> +    {
> +      info=(MI_INFO*) my_hash_element(myisam_open_cache, idx);
> +      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;
> +        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 (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;
> -      }
> +        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
> -      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->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
> -    case HA_PANIC_READ:			/* Restore to before WRITE */
> +      case HA_PANIC_READ:			/* Restore to before WRITE */
>  #ifdef CANT_OPEN_FILES_TWICE
>        {					/* Open closed files */
>  	char name_buff[FN_REFLEN];
> @@ -103,6 +104,7 @@ int mi_panic(enum ha_panic_function flag
>  	info->was_locked=0;
>        }
>        break;
> +      }
>      }
>    }
>    if (flag == HA_PANIC_CLOSE)
Concerns regarding mi_panic() are in e-mail from Kostja.


> === 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-09-30 09:29:51 +0000
> @@ -22,7 +22,7 @@
>  #include "myisamdef.h"
>  #endif
>  
> -LIST	*myisam_open_list=0;
> +
You remove myisam_open_list, but keep MI_INFO::open_list. Why?

>  uchar	myisam_file_magic[]=
>  { (uchar) 254, (uchar) 254,'\007', '\001', };
>  uchar	myisam_pack_file_magic[]=
> @@ -41,6 +41,14 @@ ulonglong myisam_max_temp_length= MAX_FI
>  ulong    myisam_data_pointer_size=4;
>  ulonglong    myisam_mmap_size= SIZE_T_MAX, myisam_mmap_used= 0;
>  
> +/* 
> +   MyISAM table cache.  After initialization,
> +   myisam_open_cache will point to myisam_open_hash
> +*/
> +HASH    *myisam_open_cache=0; 
> +HASH    myisam_open_hash;
Ehm. I understand why they're two, but I'd prefer one variable here.

What was nice with your solution, which keeps redundant myisam_open_list?
It was rock-solid, it could fall back to myisam_open_list, when hash is
not available/broken. It was lass intrusive, basically we only need to
optimize test_if_reopen(). I don't request to get myisam_open_list back,
just a prog.

Regards,
Sergey
-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
Thread
bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309) Bug#49177Magne Mahre30 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309)Bug#49177Konstantin Osipov7 Oct
    • Re: bzr commit into mysql-next-mr-bugfixing branch(magne.mahre:3309) Bug#49177Sergey Vojtovich7 Oct
    • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309)Bug#49177Magne Mæhre21 Oct
  • Re: bzr commit into mysql-next-mr-bugfixing branch(magne.mahre:3309) Bug#49177Sergey Vojtovich8 Oct
    • Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309)Bug#49177Magne Mæhre21 Oct