From: Sergey Vojtovich Date: October 8 2010 3:25pm Subject: Re: bzr commit into mysql-next-mr-bugfixing branch (magne.mahre:3309) Bug#49177 List-Archive: http://lists.mysql.com/commits/120399 Message-Id: <20101008152542.GA4106@june> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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 > 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
. Basically this list has no size, but it should not grow bigger than
. > 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 MySQL AB, Software Engineer Izhevsk, Russia, www.mysql.com