maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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

>   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()

> +}
> +
>  /** 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.

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()

Regards,
Sergei