maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

Please, see below.

On Jan 30, Sergey Vojtovich wrote:
> revno: 3965
> revision-id: svoj@xxxxxxxxxxx-20140130123642-wsysj31e1wwlqz0o
> parent: psergey@xxxxxxxxxxxx-20140120123657-g1z4g11xmsdiw2dc
> committer: Sergey Vojtovich <svoj@xxxxxxxxxxx>
> branch nick: 10.0
> timestamp: Thu 2014-01-30 16:36:42 +0400
> message:
>   MDEV-5403 - Reduce usage of LOCK_open: tc_count
>   
>   Lock-free tc_count.
> === modified file 'sql/table_cache.cc'
> --- a/sql/table_cache.cc	2013-12-12 17:49:14 +0000
> +++ b/sql/table_cache.cc	2014-01-30 12:36:42 +0000
> @@ -173,7 +175,9 @@ void tc_purge(void)
>      {
>        share->tdc.all_tables.remove(table);
>        purge_tables.push_front(table);
> -      tc_count--;
> +      my_atomic_rwlock_wrlock(&LOCK_tdc_atomics);
> +      my_atomic_add32(&tc_count, -1);
> +      my_atomic_rwlock_wrunlock(&LOCK_tdc_atomics);

Okay, so in this new code tc_count is always equal or greater than the
actual number of tables in the cache, right?
(because you first remove the table from the cache, then decrement the
counter)

>      }
>    }
>    tdc_it.deinit();
> @@ -245,47 +249,52 @@ static void check_unused(THD *thd)
>  
>  void tc_add_table(THD *thd, TABLE *table)
>  {
> +  bool need_purge;
>    DBUG_ASSERT(table->in_use == thd);
>    mysql_mutex_lock(&LOCK_open);
>    table->s->tdc.all_tables.push_front(table);
> +  mysql_mutex_unlock(&LOCK_open);
> +
>    /* If we have too many TABLE instances around, try to get rid of them */
> -  if (tc_count == tc_size)
> +  my_atomic_rwlock_wrlock(&LOCK_tdc_atomics);
> +  need_purge= my_atomic_add32(&tc_count, 1) >= (int32) tc_size;

Hmm, but here you increment after adding a table.

I'd suggest to keep tc_count consistently either always no smaller than
the number of tables in the cache (that is, decrement after removing,
increment before adding), or always no larger than that (derement before
removing, increment after adding).

> +  my_atomic_rwlock_wrunlock(&LOCK_tdc_atomics);
> +
> +  if (need_purge)
>    {
> +    TABLE *purge_table= 0;
> +    TABLE_SHARE *share;
>      TDC_iterator tdc_it;
> -    mysql_mutex_unlock(&LOCK_open);
>  
>      tdc_it.init();
>      mysql_mutex_lock(&LOCK_open);
> -    if (tc_count == tc_size)
> -    {
> -      TABLE *purge_table= 0;
> -      TABLE_SHARE *share;
>        while ((share= tdc_it.next()))
>        {
>          TABLE_SHARE::TABLE_list::Iterator it(share->tdc.free_tables);
>          TABLE *entry;
>          while ((entry= it++))
>            if (!purge_table || entry->tc_time < purge_table->tc_time)
>              purge_table= entry;
>        }
> +    tdc_it.deinit();
> +
>        if (purge_table)
>        {
> -        tdc_it.deinit();
>          purge_table->s->tdc.free_tables.remove(purge_table);
>          purge_table->s->tdc.all_tables.remove(purge_table);
>          mysql_rwlock_rdlock(&LOCK_flush);
>          mysql_mutex_unlock(&LOCK_open);
>          intern_close_table(purge_table);
>          mysql_rwlock_unlock(&LOCK_flush);
> +
> +      my_atomic_rwlock_wrlock(&LOCK_tdc_atomics);
> +      my_atomic_add32(&tc_count, -1);
> +      my_atomic_rwlock_wrunlock(&LOCK_tdc_atomics);
>          check_unused(thd);
> -        return;
>        }
> -    }
> -    tdc_it.deinit();
> -  }
> -  /* Nothing to evict, increment tc_count. */
> -  tc_count++;
> +    else
>    mysql_mutex_unlock(&LOCK_open);
> +  }
>  }
>  
>  
> @@ -357,25 +366,35 @@ bool tc_release_table(TABLE *table)
>    DBUG_ASSERT(table->in_use);
>    DBUG_ASSERT(table->file);
>  
> +  if (table->needs_reopen() || tc_records() > tc_size)
> +  {
> +    mysql_mutex_lock(&LOCK_open);
> +    table->in_use= 0;
> +    goto purge;
> +  }

Why did you move this up?

>    table->tc_time= my_interval_timer();
>  
>    mysql_mutex_lock(&LOCK_open);
>    table->in_use= 0;
> -  if (table->s->has_old_version() || table->needs_reopen() || tc_count > tc_size)
> -  {
> -    tc_count--;
> +  if (table->s->has_old_version())
> +    goto purge;
> +  /* Add table to the list of unused TABLE objects for this share. */
> +  table->s->tdc.free_tables.push_front(table);
> +  mysql_mutex_unlock(&LOCK_open);
> +  check_unused(thd);
> +  return false;
> +
> +purge:
> +  my_atomic_rwlock_wrlock(&LOCK_tdc_atomics);
> +  my_atomic_add32(&tc_count, -1);
> +  my_atomic_rwlock_wrunlock(&LOCK_tdc_atomics);
>      table->s->tdc.all_tables.remove(table);

and now you decrement before removal

>      mysql_rwlock_rdlock(&LOCK_flush);
>      mysql_mutex_unlock(&LOCK_open);
>      intern_close_table(table);
>      mysql_rwlock_unlock(&LOCK_flush);
>      return true;
> -  }
> -  /* Add table to the list of unused TABLE objects for this share. */
> -  table->s->tdc.free_tables.push_front(table);
> -  mysql_mutex_unlock(&LOCK_open);
> -  check_unused(thd);
> -  return false;
>  }
>  
>  
Regards,
Sergei