Hi Sergei, thanks for the review. Answers inline. On Wed, Mar 19, 2014 at 02:47:43PM +0100, Sergei Golubchik wrote:
Hi, Sergey!
On Mar 15, Sergey Vojtovich wrote:
At lp:maria/10.0
------------------------------------------------------------ revno: 4053 revision-id: svoj@mariadb.org-20140315185301-ce4r6br023co80dm parent: sanja@montyprogram.com-20140314073116-nklb0jkjd4t1w40n committer: Sergey Vojtovich <svoj@mariadb.org> branch nick: 10.0 timestamp: Sat 2014-03-15 22:53:01 +0400 message: MDEV-5864 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.free_tables
Let TABLE_SHARE::tdc.free_tables, TABLE_SHARE::tdc.all_tables, TABLE_SHARE::tdc.flushed and corresponding invariants be protected by per-share TABLE_SHARE::tdc.LOCK_table_share instead of global LOCK_open.
This looks pretty good to me! A couple of comments, see below.
=== modified file 'sql/sql_plist.h' --- a/sql/sql_plist.h 2013-11-20 11:05:39 +0000 +++ b/sql/sql_plist.h 2014-03-15 18:53:01 +0000 @@ -142,6 +142,14 @@ class I_P_List : public C, public I
return result; } + inline T *back() + { + T *result= m_first; + if (result) + while (*B::next_ptr(result)) + result= *B::next_ptr(result); + return result; + }
I think this is wrong. The correct way would be to use I_P_List<> with the I_P_List_fast_push_back policy, then you could use have get_last() method to get the last element in the list. May be as
inline T *back() { return *I::get_last(); }
I was considering I_P_List_fast_push_back, but I had reasons not to use it: 1. It'll have to maitain m_last (last element pointer), which involves extra shared memory location write. Even though there are good chances for m_head and m_last to share the same cache line. This maintenance will happen on a hot path. 2. I'm aiming to replace I_P_List with single-linked list (updated atomically). I can remove this ugly back() implementation when I'm done, or alternatively I can move it now to table_cache.cc. 3. We nerver use back() on a hot path, I believe there is not much meaning to sacrifice hot path performance.
void swap(I_P_List<T, B, C> &rhs) { swap_variables(T *, m_first, rhs.m_first);
=== modified file 'sql/table_cache.cc' --- a/sql/table_cache.cc 2014-03-06 12:19:12 +0000 +++ b/sql/table_cache.cc 2014-03-15 18:53:01 +0000 @@ -70,13 +70,6 @@ static int32 tc_count; /**< Number of TA
/** - Protects TABLE_SHARE::tdc.free_tables, TABLE_SHARE::tdc.all_tables. -*/ - -mysql_mutex_t LOCK_open;
So, you've now removed LOCK_open completely? Cool! Yes, there is no more global mutex on a hot path. It means different tables won't interfere on a hot path at all. At list with regards to mutexes, there is still quite a few work before we'll make it scale well.
Originally I was aiming to keep LOCK_open for protection of TABLE_SHARE::tdc.all_tables. But it came to be easier just to protect all_tables by LOCK_table_share due to invariants between free_tables and all_tables.
- - -/** Protects unused shares list.
TABLE_SHARE::tdc.prev @@ -360,12 +378,15 @@ bool tc_release_table(TABLE *table) table->in_use= 0; /* 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); + mysql_mutex_unlock(&table->s->tdc.LOCK_table_share); return false;
purge: + /* Wait for MDL deadlock detector */ + while (table->s->tdc.all_tables_refs) + mysql_cond_wait(&table->s->tdc.COND_release, &table->s->tdc.LOCK_table_share);
you're using it many times, I'd put it in a small static inline function, like
static wait_for_mdl_deadlock_detector(TABLE *table) { mysql_mutex_assert_owner(&table->s->tdc.LOCK_table_share); while (table->s->tdc.all_tables_refs) mysql_cond_wait(&table->s->tdc.COND_release, &table->s->tdc.LOCK_table_share); }
I agree. Will make it as you suggest. Hope some day I'll move it to tc_remove_table() and there will be no need for this extra function. Thanks, Sergey