maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

thanks for your comments. Did you also review the first patch?
revno: 4009
committer: Sergey Vojtovich <svoj@xxxxxxxxxxx>
branch nick: 10.0
timestamp: Fri 2014-02-14 16:25:41 +0400
message:
  MDEV-5674 - Performance: my_hash_sort_bin is called too often

  - reduced number of my_hash_sort_bin() calls from 4 to 2 per query.
    Let MDL subsystem use pre-calculated hash value for hash
    inserts and deletes.
  - reduced number of memory accesses done by my_hash_sort_bin()


On Sat, Mar 01, 2014 at 07:46:58PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
> 
> On Feb 20, Sergey Vojtovich wrote:
> > At lp:maria/10.0
> > 
> > ------------------------------------------------------------
> > revno: 4007
> > revision-id: svoj@xxxxxxxxxxx-20140220072533-ko14y1714ouvd2f7
> > parent: svoj@xxxxxxxxxxx-20140214122541-tenvcllk760deai8
> > committer: Sergey Vojtovich <svoj@xxxxxxxxxxx>
> > branch nick: 10.0
> > timestamp: Thu 2014-02-20 11:25:33 +0400
> > message:
> >   MDEV-5674 - Performance: my_hash_sort_bin is called too often
> 
> It's MDEV-5675, not MDEV-5674
Right.

> 
> >   Reuse MDL hash value in table cache:
> >   - MDL namespace is excluded from hash value calculation, so that
> >     hash value can be used by table cache as is
> >   - hash value for MDL is calculated as resulting hash value + MDL
> >     namespace
> >   - extended hash implementation to accept user defined hash function
> 
> This looks ok.
> I have a couple of comments, but they are mostly a question of taste.
> Up to you, you can push as is too.
> 
> > === modified file 'sql/mdl.cc'
> > --- a/sql/mdl.cc	2014-02-14 12:25:41 +0000
> > +++ b/sql/mdl.cc	2014-02-20 07:25:33 +0000
> > @@ -760,13 +760,22 @@ void MDL_map::init()
> >  }
> >  
> >  
> > +my_hash_value_type mdl_hash_function(const CHARSET_INFO *cs,
> > +                                     const uchar *key, size_t length)
> > +{
> > +  return *((my_hash_value_type*) (key - offsetof(MDL_key, m_ptr) +
> > +                                        offsetof(MDL_key, m_hash_value))) +
> > +                                       key[0];
> 
> I'd prefer to avoid pointer and offset juggling. Because you use a
> custom hash function anyway, you can directly pass MDL_key as your
> "key", then here you can use ((MDL_key*)key)->hash_value()
I'd prefer to avoid it too. But there is hashcmp() function which is called
when there are many records in a hash bucket. It does my_strnncoll() and
MDL_key as it is now is not good for strnncoll.

There is another option: store hash value in m_ptr, but I don't find it any
better than offset juggling.

> 
> > +}
> > +
> >  /** Initialize the partition in the container with all MDL locks. */
> >  
> > === modified file 'sql/mdl.h'
> > --- a/sql/mdl.h	2014-02-14 12:25:41 +0000
> > +++ b/sql/mdl.h	2014-02-20 07:25:33 +0000
> > @@ -397,7 +396,11 @@ class MDL_key
> >    {
> >      return & m_namespace_to_wait_state_name[(int)mdl_namespace()];
> >    }
> > -  ulong hash_value() const
> > +  my_hash_value_type hash_value() const
> > +  {
> > +    return m_hash_value + m_ptr[0];
> 
> I'm not sure it's a good idea, + is not a very good hash function :)
> On the other hand, I don't see anything immediately wrong with it in
> this particular case. Ok, let it be.
Well, it's a poor hash function indeed and suggestions are very welcome.
But what's wrong about the idea? :)

> 
> Anyway, could you please use mdl_namespace() instead of m_ptr[0] where
> possible?
> 
> > @@ -405,12 +408,14 @@ class MDL_key
> >  private:
> >    MDL_key(const MDL_key &);                     /* not implemented */
> >    MDL_key &operator=(const MDL_key &);          /* not implemented */
> > +  friend my_hash_value_type mdl_hash_function(const CHARSET_INFO *,
> > +                                              const uchar *, size_t);
> 
> You probably wouldn't need this friend, if you'd change
> mdl_hash_function() to use MDL_key::hash_value()
Right.

Thanks,
Sergey