Hi! Review of commit 7919442a866491d78c342c809f080c3fb006d626 (HEAD -> svoj-mdl) MDEV-19749 - MDL scalability regression after backup locks + --metadata-locks-instances=# + Number of fast lanes to create for metadata locks. Can be + used to improve DML scalability by eliminating + MDL_lock::rwlock load. Use 0 to disable MDL fast lanes. + Supported MDL namespaces: BACKUP If possible, I suggest we use --metadata-locks-instances=1 to disable the fast lanes. (Assuming there is no speedup or other gain by using 1 instance of fast lanes). If we do this, we should allow values 1-256 for the variable. I think the change makes sense as the user is defined lock instances, not 'fastlines'. Having 0 lock instances can imply to users that there are no locks, which is not really the case. Some observarion while looking at the code: - Most of the locks related to MDL_BACKUP_### are used with a single MDL. Should we create a fast path for these where we do not have even have to call fast_lane() or fast_lane_execption() as we already know which type of lock we are using? - In MDL_LOCK you have all variable declartions in the middle of function declarations. It would be helpful if these could be moved to be first in the class, like we have in MariaDB for almost all other classes. This makes it easier to find the variable declaration as one now has to search forward in the class in some cases and backward in others. - It took me some time to understand the benefit of having a different m_disable_count in each fast_lane instead of having a counter in MDL_lock and avoid a loop over all lanes when enable/disabling the lanes. It would be good to add a comment before m_disable_count definition to explain this. Something like /* We have to have a distinct counter in each fast_lane to ensure that enable/disable of a lane is done under the lane mutex, not under a global mutex */ + bool add_ticket(MDL_ticket *ticket) + { + DBUG_ASSERT(!ticket->m_fast_lane.load(std::memory_order_relaxed)); + mysql_mutex_lock(&m_mutex); + bool res= enabled(); + DBUG_ASSERT(res || m_list.empty()); + if (res) + { + m_list.push_back(*ticket); + ticket->m_fast_lane.store(this, std::memory_order_relaxed); + } + mysql_mutex_unlock(&m_mutex); + return res; + } As you only check for enabled() within the mutex (and no other variables), I think you can safely avoid the mutex for the common case: bool add_ticket(MDL_ticket *ticket) { if (!enabled()) return 0; DBUG_ASSERT(!ticket->m_fast_lane.load(std::memory_order_relaxed)); mysql_mutex_lock(&m_mutex); bool res= enabled(); .... /* Original code */ } This does not work in remote_ticket() as the condition is more complex. I asume that remove_ticket() is also only use if we are fast lane? If not, then we could do the following change: + bool remove_ticket(MDL_ticket *ticket) + { + mysql_mutex_lock(&m_mutex); + DBUG_ASSERT(enabled() || m_list.empty()); + Fast_lane *lane= static_cast<Fast_lane *>( + ticket->m_fast_lane.load(std::memory_order_relaxed)); + if (lane) + { + DBUG_ASSERT(enabled()); + DBUG_ASSERT(lane == this); + m_list.remove(*ticket); + } + mysql_mutex_unlock(&m_mutex); + return lane != nullptr; + } -> bool remove_ticket(MDL_ticket *ticket) { Fast_lane *lane= static_cast<Fast_lane *>( ticket->m_fast_lane.load(std::memory_order_relaxed)); if (!lane) return 0; mysql_mutex_lock(&m_mutex); .... /* Original code */ } Conclusion of slack discussion of above: - Callers of add_ticket() and remove_tickets() have checked that fast_line are used and it is only during very rare circumstances that the if (enabled()) and if (lane) are not true. - However we should consdier adding likely() to the if's and add a comment under which conditions the if() could not be true. /** Disables fast lane, moves tickets to MDL_lock::m_granted. I like the idea how disable() converts 'fast line' tickets to MDL. Great idea as it simplifies things a lot! + /** MDL_lock::iterate helper. */ + bool iterate(mdl_iterator_callback callback, void *argument) const + { + mysql_mutex_lock(&m_mutex); + DBUG_ASSERT(enabled() || m_list.empty()); + bool res= std::any_of(m_list.begin(), m_list.end(), + [callback, argument](MDL_ticket &ticket) { + return callback(&ticket, argument, true); + }); + mysql_mutex_unlock(&m_mutex); + return res; + } Any chance you could code this without std:: ??? It makes code much harder to read for C coders (like me). I also do not want too see too much depdendence of std:: in MariaDB but instead use our list primitives. This keeps the code more uniform and easier to debug! It also ensures that all memory allocations are accounted for! I greatly prefer inline code that is easy to debug step by step! (I would not be able to debug the above if there would ever be a problem with the list!) However if you greatly prefer you can keep the std:: code, but then I cannot help you with debugging this part of the code. ------- @@ -678,10 +918,15 @@ class MDL_lock { DBUG_ASSERT(key_arg->mdl_namespace() == MDL_key::BACKUP); mysql_prlock_init(key_MDL_lock_rwlock, &m_rwlock); + m_fast_lane= + mdl_instances ? new (std::nothrow) Fast_lane[mdl_instances] : nullptr; } If we use '1' as default for no fastlines, we have to change the above line. void downgrade(MDL_ticket *ticket, Functor update_ticket_type) { + DBUG_ASSERT(!m_strategy->fast_lane(ticket->get_type())); + DBUG_ASSERT(!ticket->m_fast_lane.load(std::memory_order_relaxed)); + DBUG_ASSERT(m_strategy->fast_lane_exception(ticket->get_type()) || + fast_lanes_all_disabled()); Isn't fast_lines_all_disabled() always true here in the current implementation? (At least the comment indicates this is the case) Looking at the upgrade() call, it looks like if we would remove a ticket, which can call enable_fast_lanes(), and we call downgrade again, then the comment for downgrade would not hold (it says that tickes for downgrade are never fast_lane). If the above assumption is true, we should update the comment for downgrade that the assumption about only using convention locks assumes that one does not do downgrade-upgrade-downgrade of a lock (which we do not do now). template<typename Functor> void upgrade(MDL_ticket *ticket, Functor update_ticket_type, MDL_ticket *remove) + if (m_fast_lane && !m_strategy->fast_lane_exception(remove->get_type())) + enable_fast_lanes(); Please add an explanation under which conditions in the current codebase the call to enable_fast_lanes() is happening. This is needed as the move to fast_lines here is somewhat in conflict with the function comment that says we are never using fastlane locks here. I don't understand how we can enable fast_lanes for a ticket for which we have !m_strategy->fast_lane(ticket->get_type()), which I assume means that the ticket is not suitable for fast_lane. I did some testing and found out that the enable_fast_lanes() code happens when upgrading the global read lock from MDL_BACKUP_FTWRL1 to MDL_BACKUP_FTWRL2. As fast_lane(MDL_BACKUP_FTWRL2) and fast_lane_execption(MDL_BACKUP_FTWRL2) are both false why are we enabling fast_lanes for MDL_BACKUP_FTWRL2 ? Looking at the try_acquire_lock() the rule looks like: - If m_fast_lane is set - If fast_lane(ticket_type) is true, use fast_lanes else if !fast_lane_exception(ticket_type) disable fast lanes This does not explain the case why MDL_BACKUP_FTWRL2 would enable fast lanes. Answer from slack discussion: "Both MDL_BACKUP_FTWRL1 and MDL_BACKUP_FTWRL2 disable fast lanes, and they need to re-enable them once they're gone." in try_acquire_lock((): { + if (m_fast_lane) + { + DBUG_ASSERT(mdl_instances); + if (m_strategy->fast_lane(ticket->get_type())) + { + uint lane= mdl_context->get_thd()->thread_id % mdl_instances; + if (m_fast_lane[lane].add_ticket(ticket)) + return res; + } + else if (!m_strategy->fast_lane_exception(ticket->get_type())) + need_disable_fast_lanes= true; + } Under backup or FTWRL when fast lines are disabled, the above code may use an extra mutex for every BACKUP lock. This may potentially degrade performance notable when backup is running! Is this the case? We may have to consider checking enabled() outside of the mutex in add_ticket() or the caller to not get a notable degradation when backup is running. If this is ONLY under the BACKUP_WAIT_COMMIT lock, then the current code is ok as this lock is intended to be kept very short (less than a second). However, if we will start using MDL locks as soon as the backup starts and as the backup can take hours, this is a problem. We need to do a benchmark where we run hammardb while one connection is doing 'backup stage start' to understand the consequences. I have asked Steve to do this. In the try_acqure_code() we also may call disable_fast_lanes() even if fast_lines was disabled. It would good to add a comment why this is ok, like that enable_fast_lines() will be called when the ticket is released. else if (!m_strategy->fast_lane_exception(ticket->get_type())) need_disable_fast_lanes= true; ... if (likely(m_strategy)) Under what circumstances can m_strategy by 0 ? Is it possible that when m_strategy is 0, that m_fast_lane is not 0 ? If yes, then you will have a crash at the above if. I think this is impossible as m_strategy is always set for backup locks. In any case, please add a comment before 'if (likely(m_strategy))' when it could be 0. If it can never be zero, then change the if to a DBUG_ASSERT() Discussion on slack showed this is ok. Please add comment before if /* m_strategy can be 0 if a concurrent thread is deleting the MDL lock. In this case the caller will do a retry */ + Lock requests that were served by fast lanes are redirected to fast + lanes. Another thread can be disabling fast lanes concurrently and + resetting tiket->m_fast_lane. To handle such scenario properly tiket -> ticket @@ -1992,6 +2369,9 @@ void MDL_lock::remove_ticket(LF_PINS *pins, Ticket_list MDL_lock::*list, reschedule_waiters(); } mysql_prlock_unlock(&m_rwlock); + if (m_fast_lane && !m_strategy->fast_lane(ticket->get_type()) && + !m_strategy->fast_lane_exception(ticket->get_type())) + enable_fast_lanes(); You have the the above exact code in two places. Moving this to an inline function and checking fast_lane and fast_lane_exception bits at the same time would make a small improvement. Regards, Monty