Hi, Sergey! On Mar 19, Sergey Vojtovich wrote:
=== 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.
Okay. In that case, please rename the method to back_slow() or something that would prevent it being mindlessly used everywhere. Something that would make one to to stop and think "hmm, this is an expensive method, do I really want to use it here?" Optionally, you can also add this simple back() as above, so that one would have a fast alternative. Or, indeed, you can remove back() implementation from I_P_List and move it to table_cache.cc, if that's possible. I'm ok with either approach. Regards, Sergei