Re: [Maria-developers] MDEV-5674 - Performance: my_hash_sort_bin is called too often
Hi, Sergey! On Feb 20, Sergey Vojtovich wrote:
At lp:maria/10.0
------------------------------------------------------------ revno: 4007 revision-id: svoj@mariadb.org-20140220072533-ko14y1714ouvd2f7 parent: svoj@mariadb.org-20140214122541-tenvcllk760deai8 committer: Sergey Vojtovich <svoj@mariadb.org> 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
Hi Sergei, thanks for your comments. Did you also review the first patch? revno: 4009 committer: Sergey Vojtovich <svoj@mariadb.org> 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@mariadb.org-20140220072533-ko14y1714ouvd2f7 parent: svoj@mariadb.org-20140214122541-tenvcllk760deai8 committer: Sergey Vojtovich <svoj@mariadb.org> 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
Hi, Sergey! On Mar 03, Sergey Vojtovich wrote:
Hi Sergei,
thanks for your comments. Did you also review the first patch? revno: 4009 committer: Sergey Vojtovich <svoj@mariadb.org> 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()
No. I thought there's only one, and reviewed the last as usual. Now I've looked for it and couldn't find this patch. There were few revno:4009 mail, but none about my_hash_sort_bin. And few "MDEV-5674" (really MDEV-5675), but none with revno:4009. Regards, Sergei
Hi Segei, sorry for the mess with revisions and incorrect MDEV specification. The proper patch is: Date: Fri, 14 Feb 2014 12:25:50 +0000 (UTC) From: Sergey Vojtovich <svoj@mariadb.org> To: commits@mariadb.org Subject: [Commits] Rev 4006: MDEV-5674 - Performance: my_hash_sort_bin is called too often in lp:maria/10.0 Thanks, Sergey On Tue, Mar 04, 2014 at 12:21:24PM +0100, Sergei Golubchik wrote:
Hi, Sergey!
On Mar 03, Sergey Vojtovich wrote:
Hi Sergei,
thanks for your comments. Did you also review the first patch? revno: 4009 committer: Sergey Vojtovich <svoj@mariadb.org> 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()
No. I thought there's only one, and reviewed the last as usual. Now I've looked for it and couldn't find this patch. There were few revno:4009 mail, but none about my_hash_sort_bin. And few "MDEV-5674" (really MDEV-5675), but none with revno:4009.
Regards, Sergei
Hi, Sergey! On Mar 03, Sergey Vojtovich wrote:
=== 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.
But hashcmp() uses HASH::get_key() callback to get the key. You can use it to return MDL_key::m_ptr Regards, Sergei
Hi Sergei,
On Tue, Mar 04, 2014 at 01:05:33PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
>
> On Mar 03, Sergey Vojtovich wrote:
> > > > === 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.
>
> But hashcmp() uses HASH::get_key() callback to get the key.
> You can use it to return MDL_key::m_ptr
That's how it is now and that's the reason for this juggling. :)
I.e. my_hash_first_from_hash_value():
- calls hashcmp(key) which assumes key is MDL_key::m_ptr
- calls my_hash_rec_mask()/hash_function(key) which should assume key is MDL_key
Regards,
Sergey
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich