maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

Ok, see comments below.

But why is this supposed to improve scalability?
Are you talking about the use case of one table being access by multiple
threads? Or this helps in other cases too?
And what is the effect of the more expensive tc_remove_all_unused_tables() ?

On Jun 29, Sergey Vojtovich wrote:
> revision-id: 48d1ba6097939efe5efd28dd3ed9d281cc2bc2f4 (mariadb-10.2.0-93-g48d1ba6)
> parent(s): 8bec9746f0d5b363f385713035ca3f2daff34e1c
> committer: Sergey Vojtovich
> timestamp: 2016-06-29 16:33:08 +0400
> message:
> 
> MDEV-10296 - Multi-instance table cache
> 
> Improve scalability by implementing multi-instance table cache.
> 
> diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result
> index e075c64..22b12a9 100644
> --- a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result
> +++ b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result
> @@ -4559,6 +4559,20 @@ NUMERIC_BLOCK_SIZE	1
>  ENUM_VALUE_LIST	NULL
>  READ_ONLY	NO
>  COMMAND_LINE_ARGUMENT	REQUIRED
> +VARIABLE_NAME	TABLE_OPEN_CACHE_INSTANCES
> +SESSION_VALUE	NULL
> +GLOBAL_VALUE	1
> +GLOBAL_VALUE_ORIGIN	COMPILE-TIME

Default value 1 and it's not auto-tuned on startup?
You think multi-instance table cache is better stay unused? :)
If not, either use a reasonable default or, may be, choose it on
startup based on my_gencpus()

> +DEFAULT_VALUE	1
> +VARIABLE_SCOPE	GLOBAL
> +VARIABLE_TYPE	BIGINT UNSIGNED
> +VARIABLE_COMMENT	The number of table cache instances
> +NUMERIC_MIN_VALUE	1
> +NUMERIC_MAX_VALUE	64
> +NUMERIC_BLOCK_SIZE	1
> +ENUM_VALUE_LIST	NULL
> +READ_ONLY	YES
> +COMMAND_LINE_ARGUMENT	REQUIRED
>  VARIABLE_NAME	THREAD_CACHE_SIZE
>  SESSION_VALUE	NULL
>  GLOBAL_VALUE	151
> diff --git a/sql/table_cache.cc b/sql/table_cache.cc
> index bfe51df..f1eb006 100644
> --- a/sql/table_cache.cc
> +++ b/sql/table_cache.cc
> @@ -125,67 +114,101 @@ static int fix_thd_pins(THD *thd)
>    part of table definition cache.
>  */
>  
> +struct Share_free_tables
>  {
> +  typedef I_P_List <TABLE, TABLE_share> List;
> +  List list;
> +  char pad[CPU_LEVEL1_DCACHE_LINESIZE];

a comment, please, explaining the padding.
Or see a suggestion at the end.

> +};
>  
> +
> +struct Table_cache_instance
> +{
> +  /**
> +    Protects free_tables (TABLE::global_free_next and TABLE::global_free_prev),
> +    records, Share_free_tables::List (TABLE::prev and TABLE::next),
> +    TABLE::in_use.
> +  */
> +  mysql_mutex_t LOCK_table_cache;
> +  I_P_List <TABLE, I_P_List_adapter<TABLE, &TABLE::global_free_next,
> +                                    &TABLE::global_free_prev>,
> +            I_P_List_null_counter, I_P_List_fast_push_back<TABLE> >
> +    free_tables;
> +  ulong records;
> +  char pad[CPU_LEVEL1_DCACHE_LINESIZE];

why is it padded?

> +
> +  Table_cache_instance(): records(0)
>    {
> +    mysql_mutex_init(key_LOCK_table_cache, &LOCK_table_cache,
> +                     MY_MUTEX_INIT_FAST);
>    }
> +
> +  ~Table_cache_instance()
> +  {
> +    mysql_mutex_destroy(&LOCK_table_cache);
> +    DBUG_ASSERT(free_tables.is_empty());
> +    DBUG_ASSERT(records == 0);
> +  }
> +};
>  
>  
> +static Table_cache_instance *tc;
>  
> +
> +static inline Share_free_tables *SHARE_FREE_TABLES(TDC_element *element)

fairly unconventional letter case choice :)
Besides, instead of pointer arithmetics, you could've simply put


   Share_free_tables free_tables[1];

at the end of TDC_element.

>  {
> +  return (Share_free_tables*) (element + 1);
>  }
>  
>  
> +static void intern_close_table(TABLE *table)
> +{
> +  delete table->triggers;
> +  DBUG_ASSERT(table->file);

old intern_close_table had

  if (table->file) // Not true if placeholder

do you mean, these placeholder TABLEs can no longer be in the table
cache?

> +  closefrm(table);
> +  tdc_release_share(table->s);

old intern_close_table() also did table->alias.free();

> +  my_free(table);
> +}
>  
> +
> +/**
> +  Get number of TABLE objects (used and unused) in table cache.
>  */
>  
> +uint tc_records(void)
>  {
> +  ulong total= 0;
> +  for (ulong i= 0; i < tc_instances; i++)
> +  {
> +    mysql_mutex_lock(&tc[i].LOCK_table_cache);
> +    total+= tc[i].records;
> +    mysql_mutex_unlock(&tc[i].LOCK_table_cache);
> +  }
> +  return total;
>  }
>  
>  
>  /**
>    Remove TABLE object from table cache.
>  */
>  
>  static void tc_remove_table(TABLE *table)
>  {
> +  TDC_element *element= table->s->tdc;
> +
> +  mysql_mutex_lock(&element->LOCK_table_share);
> +  /* Wait for MDL deadlock detector to complete traversing tdc.all_tables. */
> +  while (element->all_tables_refs)
> +    mysql_cond_wait(&element->COND_release, &element->LOCK_table_share);
> +  element->all_tables.remove(table);
> +  mysql_mutex_unlock(&element->LOCK_table_share);
> +
> +  intern_close_table(table);
>  }
>  
>  
>  static void tc_remove_all_unused_tables(TDC_element *element,
> +                                        Share_free_tables::List *purge_tables,
>                                          bool mark_flushed)
>  {
>    TABLE *table;
> @@ -200,10 +223,18 @@ static void tc_remove_all_unused_tables(TDC_element *element,
>    */
>    if (mark_flushed)
>      element->flushed= true;
> +  for (ulong i= 0; i < tc_instances; i++)
>    {
> +    mysql_mutex_lock(&tc[i].LOCK_table_cache);
> +    while ((table= SHARE_FREE_TABLES(element)[i].list.pop_front()))
> +    {
> +      tc[i].records--;
> +      tc[i].free_tables.remove(table);
> +      DBUG_ASSERT(element->all_tables_refs == 0);
> +      element->all_tables.remove(table);
> +      purge_tables->push_front(table);
> +    }
> +    mysql_mutex_unlock(&tc[i].LOCK_table_cache);
>    }
>  }
>  
> @@ -225,7 +256,7 @@ static void tc_remove_all_unused_tables(TDC_element *element,
>  
>  struct tc_purge_arg
>  {
> +  Share_free_tables::List purge_tables;
>    bool mark_flushed;
>  };
>  
> @@ -281,79 +298,33 @@ static TABLE *tc_free_tables_back(TDC_element *element)
>    - free evicted object
>  */
>  
> +void tc_add_table(THD *thd, TABLE *table)
>  {
> +  ulong i= thd->thread_id % tc_instances;
> +  TABLE *LRU_table= 0;
> +  TDC_element *element= table->s->tdc;
>  
> +  DBUG_ASSERT(table->in_use == thd);
>    mysql_mutex_lock(&element->LOCK_table_share);
> +  /* Wait for MDL deadlock detector to complete traversing tdc.all_tables. */
> +  while (element->all_tables_refs)
> +    mysql_cond_wait(&element->COND_release, &element->LOCK_table_share);
> +  element->all_tables.push_front(table);
>    mysql_mutex_unlock(&element->LOCK_table_share);
>  
> +  mysql_mutex_lock(&tc[i].LOCK_table_cache);
> +  if (tc[i].records == tc_size && (LRU_table= tc[i].free_tables.pop_front()))
>    {
> +    SHARE_FREE_TABLES(LRU_table->s->tdc)[i].list.remove(LRU_table);
> +    /* Needed if MDL deadlock detector chimes in before tc_remove_table() */
> +    LRU_table->in_use= thd;

How can MDL deadlock detector get to the unused table?  It traverses the
lock dependency graph, and an unused table cannot be locked, can it?

>    }
> +  else
> +    tc[i].records++;
> +  mysql_mutex_unlock(&tc[i].LOCK_table_cache);
> +
> +  if (LRU_table)
> +    tc_remove_table(LRU_table);
>  }
>  
>  
> @@ -369,10 +340,11 @@ void tc_add_table(THD *thd, TABLE *table)
>  
>  static TABLE *tc_acquire_table(THD *thd, TDC_element *element)
>  {
> +  ulong i= thd->thread_id % tc_instances;
>    TABLE *table;
>  
> -  mysql_mutex_lock(&element->LOCK_table_share);
> -  table= element->free_tables.pop_front();
> +  mysql_mutex_lock(&tc[i].LOCK_table_cache);
> +  table= SHARE_FREE_TABLES(element)[i].list.pop_front();
>    if (table)
>    {
>      DBUG_ASSERT(!table->in_use);
> @@ -381,8 +353,9 @@ static TABLE *tc_acquire_table(THD *thd, TDC_element *element)
>      DBUG_ASSERT(table->db_stat && table->file);
>      /* The children must be detached from the table. */
>      DBUG_ASSERT(!table->file->extra(HA_EXTRA_IS_ATTACHED_CHILDREN));
> +    tc[i].free_tables.remove(table);
>    }
> -  mysql_mutex_unlock(&element->LOCK_table_share);
> +  mysql_mutex_unlock(&tc[i].LOCK_table_cache);
>    return table;
>  }
>  
> @@ -413,40 +386,27 @@ static TABLE *tc_acquire_table(THD *thd, TDC_element *element)
>      @retval false object released
>  */
>  
> -bool tc_release_table(TABLE *table)
> +void tc_release_table(TABLE *table)
>  {
> +  ulong i= table->in_use->thread_id % tc_instances;
>    DBUG_ASSERT(table->in_use);
>    DBUG_ASSERT(table->file);
>  
> -  if (table->needs_reopen() || tc_records() > tc_size)
> +  mysql_mutex_lock(&tc[i].LOCK_table_cache);
> +  if (table->needs_reopen() || table->s->tdc->flushed ||
> +      tc[i].records > tc_size)
>    {
> -    mysql_mutex_lock(&table->s->tdc->LOCK_table_share);
> -    goto purge;
> +    tc[i].records--;
> +    mysql_mutex_unlock(&tc[i].LOCK_table_cache);
> +    tc_remove_table(table);

Does it mean that if the table cache is full, you free the table,
instead of adding it to the cache? That's not very LRU-ish

> +  }
> +  else
> +  {
> +    table->in_use= 0;
> +    SHARE_FREE_TABLES(table->s->tdc)[i].list.push_front(table);
> +    tc[i].free_tables.push_back(table);
> +    mysql_mutex_unlock(&tc[i].LOCK_table_cache);
>    }
>  }
>  
>  
> @@ -573,23 +535,29 @@ static uchar *tdc_hash_key(const TDC_element *element, size_t *length,
>    Initialize table definition cache.
>  */
>  
> -void tdc_init(void)
> +bool tdc_init(void)
>  {
>    DBUG_ENTER("tdc_init");
>  #ifdef HAVE_PSI_INTERFACE
> -  init_tc_psi_keys();
> +  mysql_mutex_register("sql", all_tc_mutexes, array_elements(all_tc_mutexes));
> +  mysql_cond_register("sql", all_tc_conds, array_elements(all_tc_conds));
>  #endif
> +  /* Extra instance is allocated to avoid false sharing */
> +  if (!(tc= new Table_cache_instance[tc_instances + 1]))
> +    DBUG_RETURN(true);
>    tdc_inited= true;
>    mysql_mutex_init(key_LOCK_unused_shares, &LOCK_unused_shares,
>                     MY_MUTEX_INIT_FAST);
>    tdc_version= 1L;  /* Increments on each reload */
> -  lf_hash_init(&tdc_hash, sizeof(TDC_element), LF_HASH_UNIQUE, 0, 0,
> +  lf_hash_init(&tdc_hash, sizeof(TDC_element) +
> +                          sizeof(Share_free_tables) * tc_instances,
> +               LF_HASH_UNIQUE, 0, 0,
>                 (my_hash_get_key) tdc_hash_key,
>                 &my_charset_bin);
>    tdc_hash.alloc.constructor= lf_alloc_constructor;
>    tdc_hash.alloc.destructor= lf_alloc_destructor;
>    tdc_hash.initializer= (lf_hash_initializer) tdc_hash_initializer;
> -  DBUG_VOID_RETURN;
> +  DBUG_RETURN(false);
>  }
>  
>  
> @@ -631,6 +599,7 @@ void tdc_deinit(void)
>      tdc_inited= false;
>      lf_hash_destroy(&tdc_hash);
>      mysql_mutex_destroy(&LOCK_unused_shares);
> +    delete [] tc;
>    }
>    DBUG_VOID_RETURN;
>  }
> @@ -1038,7 +1007,7 @@ bool tdc_remove_table(THD *thd, enum_tdc_remove_table_type remove_type,
>                        const char *db, const char *table_name,
>                        bool kill_delayed_threads)
>  {
> -  TDC_element::TABLE_list purge_tables;
> +  Share_free_tables::List purge_tables;
>    TABLE *table;
>    TDC_element *element;
>    uint my_refs= 1;
> diff --git a/sql/table_cache.h b/sql/table_cache.h
> index c05baae..4f449cb 100644
> --- a/sql/table_cache.h
> +++ b/sql/table_cache.h
> @@ -45,7 +43,7 @@ struct TDC_element
>      for this share.
>    */
>    All_share_tables_list all_tables;
> -  TABLE_list free_tables;
> +  char pad[CPU_LEVEL1_DCACHE_LINESIZE]; // free_tables follows this immediately

hmm. may be you'd rather put padding in one place - at the beginning of
Share_free_tables?

>  };

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx