Hi Sergey, thanks for looking into this!
On Wed, Nov 26, 2014 at 08:00:44PM +0100, Sergei Golubchik wrote: > Hi, Sergey > > I didn't review MDEV-7200 (InnoDB) and patches that are already in 10.1. Patch for MDEV-7200 is not good to go into 10.1 as is anyway. It is here just as a reminder that we can gain another 2% with simple fix. Some of my patches depend on Monty's patches that haven't actually been pushed to 10.1 yet. I'll hold those. Further comments inline. > Almost everything else was fine, see few comments below. > > > ------------------------------------------------------------ > > revno: 4513 > > committer: Sergey Vojtovich <[email protected]> > > branch nick: 10.0-power > > timestamp: Fri 2014-11-07 22:03:18 +0400 > > message: > > Multi-rwlock lock for table definition cache hash. > > added: > > modified: > > diff: > > === modified file 'sql/table_cache.cc' > > --- sql/table_cache.cc 2014-10-21 12:20:39 +0000 > > +++ sql/table_cache.cc 2014-11-07 18:03:18 +0000 > > @@ -53,6 +53,77 @@ > > #include "table.h" > > #include "sql_base.h" > > > > + > > +/** > > + Table definition hash lock. > > + > > + Lock consists of multiple rw-locks (8 by default). Lock is write locked > > + when all instances are write locked. Lock is read locked when any > > instance > > + is read locked. > > + > > + This class was implemented to workaround scalability bottleneck of table > > + definition hash rw-lock. > > + > > + To be replaced with lock-free hash when we understand how to solve the > > + following problems: > > + - lock-free hash iterator (can be workarounded by introducing global > > + all_tables list); > > > I don't particularly like that solution. > Could you try to switch to lock-free hash now? > > I'll try to do the iterator asap This is the change that allowed us to break through 27kTPS to 33kTPS. I don't like it either. My raw lf-hash prototype shows similar results. I think we hardly need to have this bottleneck solved in 10.1 one way or another. So my question is: do you feel like we will be able to implement this all on time for 10.1? > > > + - lf_hash_search() may fail because of OOM. It is rather complex to > > handle > > + it properly. > > +*/ > > + > > +class TDC_lock > > +{ > > + uint m_instances; > > + bool m_write_locked; > > + char pad[128]; Interesting, why did I add the above padding... probably leftover from intermediate patch. Will remove it. > > + struct st_locks > > + { > > + mysql_rwlock_t lock; > > + char pad[128]; > > In these cases I usually do: pad[128 - sizeof(mysql_rwlock_t)]; Hmm... strange that I didn't define macro for this, I thought I did. Your suggestion guarantees that st_locks will be at least 128 bytes. But these 128 bytes may be split across 2 cache lines. Or malloc returns pointer aligned on cache line size? > > > + } *locks; > > +public: > > + int init(PSI_rwlock_key key, uint instances) > > + { > > + m_instances= instances; > > + m_write_locked= false; > > + locks= (st_locks *) my_malloc(sizeof(struct st_locks) * m_instances, > > MYF(MY_WME)); > > + if (!locks) > > + return 1; > > + for (uint i= 0; i < m_instances; i++) > > + mysql_rwlock_init(key, &locks[i].lock); > > + return 0; > > + } > > + void destroy(void) > > + { > > + for (uint i= 0; i < m_instances; i++) > > + mysql_rwlock_destroy(&locks[i].lock); > > + my_free(locks); > > + } > > + void wrlock(void) > > + { > > + for (uint i= 0; i < m_instances; i++) > > + mysql_rwlock_wrlock(&locks[i].lock); > > + m_write_locked= true; > > + } > > + void rdlock(THD *thd= 0) > > + { > > + mysql_rwlock_rdlock(&locks[thd ? thd->thread_id % m_instances : > > 0].lock); > > + } > > + void unlock(THD *thd= 0) > > + { > > + if (m_write_locked) > > + { > > + m_write_locked= false; > > + for (uint i= 0; i < m_instances; i++) > > + mysql_rwlock_unlock(&locks[i].lock); > > + } > > + else > > + mysql_rwlock_unlock(&locks[thd ? thd->thread_id % m_instances : > > 0].lock); > > + } > > +}; > > + > > + > > /** Configuration. */ > > ulong tdc_size; /**< Table definition cache threshold for LRU eviction. */ > > ulong tc_size; /**< Table cache threshold for LRU eviction. */ > > ------------------------------------------------------------ > > revno: 4512 > > committer: Sergey Vojtovich <[email protected]> > > branch nick: 10.0-power > > timestamp: Fri 2014-10-31 15:44:36 +0400 > > message: > > Preallocate locks on THD mem_root to avoid expensive malloc. > > modified: > > diff: > > === modified file 'sql/lock.cc' > > --- sql/lock.cc 2014-09-30 17:31:14 +0000 > > +++ sql/lock.cc 2014-10-31 11:44:36 +0000 > > @@ -700,7 +699,7 @@ MYSQL_LOCK *get_lock_data(THD *thd, TABL > > TABLE **to, **table_buf; > > DBUG_ENTER("get_lock_data"); > > > > - DBUG_ASSERT((flags == GET_LOCK_UNLOCK) || (flags == > > GET_LOCK_STORE_LOCKS)); > > + DBUG_ASSERT((flags & GET_LOCK_UNLOCK) || (flags & GET_LOCK_STORE_LOCKS)); > > Not the same, old assert also verified that these flags aren't set both. > New assert doesn't. > But I don't think it matters much. You're right, I'll change this: the stricter - the better. > > > DBUG_PRINT("info", ("count %d", count)); > > > > for (i=lock_count=table_count=0 ; i < count ; i++) > > ------------------------------------------------------------ > > revno: 4508 > > committer: Sergey Vojtovich <[email protected]> > > branch nick: 10.0 > > timestamp: Tue 2014-10-21 16:20:39 +0400 > > message: > > modified: > > diff: > > === modified file 'sql/handler.cc' > > --- sql/handler.cc 2014-11-13 10:01:31 +0000 > > +++ sql/handler.cc 2014-10-21 12:20:39 +0000 > > @@ -410,13 +410,16 @@ static int hton_ext_based_table_discover > > static void update_discovery_counters(handlerton *hton, int val) > > { > > if (hton->discover_table_existence == full_discover_for_existence) > > - my_atomic_add32(&need_full_discover_for_existence, val); > > + my_atomic_add32_explicit(&need_full_discover_for_existence, val, > > + MY_MEMORY_ORDER_RELAXED); > > > > if (hton->discover_table_names) > > - my_atomic_add32(&engines_with_discover_table_names, val); > > + my_atomic_add32_explicit(&engines_with_discover_table_names, val, > > + MY_MEMORY_ORDER_RELAXED); > > > > if (hton->discover_table) > > - my_atomic_add32(&engines_with_discover, val); > > + my_atomic_add32_explicit(&engines_with_discover, val, > > + MY_MEMORY_ORDER_RELAXED); > > I wouldn't do that. Technically there's a dependency between these counters > and loaded engines, so the order, kind of, matters. I don't think there > is any practical risk of bugs here, but, on the other hand, these > counters are only updated when an engine is loaded or unloaded, > so there's no need to bother with the relaxed memory ordering here. Ok, I'll revert this. > > > } > > > > int ha_finalize_handlerton(st_plugin_int *plugin) > > > > === modified file 'sql/mysqld.cc' > > --- sql/mysqld.cc 2014-11-18 21:25:47 +0000 > > +++ sql/mysqld.cc 2014-10-21 12:20:39 +0000 > > @@ -3884,7 +3884,7 @@ static void my_malloc_size_cb_func(long > > } > > // workaround for gcc 4.2.4-1ubuntu4 -fPIE (from DEB_BUILD_HARDENING=1) > > int64 volatile * volatile ptr=&global_status_var.memory_used; > > - my_atomic_add64(ptr, size); > > + my_atomic_add64_explicit(ptr, size, MY_MEMORY_ORDER_RELAXED); > > good! > > > } > > } > > > > > > === modified file 'sql/mysqld.h' > > --- sql/mysqld.h 2014-11-13 08:56:28 +0000 > > +++ sql/mysqld.h 2014-10-21 12:20:39 +0000 > > @@ -629,7 +629,7 @@ inline __attribute__((warn_unused_result > > { > > query_id_t id; > > my_atomic_rwlock_wrlock(&global_query_id_lock); > > - id= my_atomic_add64(&global_query_id, 1); > > + id= my_atomic_add64_explicit(&global_query_id, 1, > > MY_MEMORY_ORDER_RELAXED); > > I believe this is fine > > > my_atomic_rwlock_wrunlock(&global_query_id_lock); > > return (id); > > } > > === modified file 'sql/table_cache.cc' > > --- sql/table_cache.cc 2014-06-10 18:20:33 +0000 > > +++ sql/table_cache.cc 2014-10-21 12:20:39 +0000 > > @@ -139,7 +139,7 @@ uint tc_records(void) > > { > > uint count; > > my_atomic_rwlock_rdlock(&LOCK_tdc_atomics); > > - count= my_atomic_load32(&tc_count); > > + count= my_atomic_load32_explicit(&tc_count, MY_MEMORY_ORDER_RELAXED); > > I'm not sure about this file. You should know better whether > it's safe to use relaxed memory ordering here. It should be Ok. This counter should not have strict memory ordering constraints. Thanks, Sergey _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
