Bug #68647 function lock_number_of_rows_locked may be Inefficient

• Submitted: 12 Mar 2013 3:05
• Modified: 28 Aug 2014 19:20
• Reporter: zhai weixiang (OCA)
• Status: Closed
• Category: MySQL Server: InnoDB storage engine
• Severity: S3 (Non-critical)
• Version: 5.5.31,5.6.11,5.7.1
• OS: Any
• CPU Architecture: Any
• Tags: Contribution

[12 Mar 2013 3:05] zhai weixiang

Description:
In our environment， “show engine innodb status” was frequently executed by some tools used to monitor the stats of MySQL instance.  
From perf top, function lock_number_of_rows_locked may occupy more than 20% of CPU sometimes. So I guess this function may be Inefficient

quoted code from lock_number_of_rows_locked:

       for (lock = UT_LIST_GET_FIRST(trx_lock->trx_locks);
            lock != NULL;
            lock = UT_LIST_GET_NEXT(trx_locks, lock)) {

               if (lock_get_type_low(lock) == LOCK_REC) {
                       ulint   n_bit;
                       ulint   n_bits = lock_rec_get_n_bits(lock);

                       for (n_bit = 0; n_bit < n_bits; n_bit++) {
                               if (lock_rec_get_nth_bit(lock, n_bit)) {
                                       n_records++;
                               }
                       }
               }
       }

I think It's more efficient to add a counter in trx_lock_t, and then  increase/decrease it in lock_rec_set_nth_bit  and lock_rec_reset_nth_bit

How to repeat:
read related code

Suggested fix:
a simple and rough patch  maybe (I just run all  mysql-test) based on 5.6.10

Index: storage/innobase/lock/lock0lock.cc
===================================================================
--- storage/innobase/lock/lock0lock.cc  (revision 3649)
+++ storage/innobase/lock/lock0lock.cc  (revision 3654)
@@ -1095,6 +1095,8 @@
        bit_index = i % 8;

        ((byte*) &lock[1])[byte_index] |= 1 << bit_index;
+
+       lock->trx->lock.n_rec_locks++;
 }

 /**********************************************************************//**
@@ -1142,6 +1144,8 @@
        bit_index = i % 8;

        ((byte*) &lock[1])[byte_index] &= ~(1 << bit_index);
+
+       lock->trx->lock.n_rec_locks--;
 }

 /*********************************************************************//**
@@ -1698,28 +1702,9 @@
 /*=======================*/
        const trx_lock_t*       trx_lock)       /*!< in: transaction locks */
 {
-       const lock_t*   lock;
-       ulint           n_records = 0;
-
        ut_ad(lock_mutex_own());

-       for (lock = UT_LIST_GET_FIRST(trx_lock->trx_locks);
-            lock != NULL;
-            lock = UT_LIST_GET_NEXT(trx_locks, lock)) {
-
-               if (lock_get_type_low(lock) == LOCK_REC) {
-                       ulint   n_bit;
-                       ulint   n_bits = lock_rec_get_n_bits(lock);
-
-                       for (n_bit = 0; n_bit < n_bits; n_bit++) {
-                               if (lock_rec_get_nth_bit(lock, n_bit)) {
-                                       n_records++;
-                               }
-                       }
-               }
-       }
-
-       return(n_records);
+      return (trx_lock->n_rec_locks);
 }

 /*============== RECORD LOCK CREATION AND QUEUE MANAGEMENT =============*/
@@ -6843,6 +6828,8 @@

        lock_release(trx);

+      trx->lock.n_rec_locks = 0;
+
        lock_mutex_exit();
 }

Index: storage/innobase/trx/trx0trx.cc
===================================================================
--- storage/innobase/trx/trx0trx.cc     (revision 3649)
+++ storage/innobase/trx/trx0trx.cc     (revision 3654)
@@ -125,6 +125,8 @@
        trx->lock.lock_heap = mem_heap_create_typed(
                256, MEM_HEAP_FOR_LOCK_HEAP);

+      trx->lock.n_rec_locks = 0;
+
        trx->search_latch_timeout = BTR_SEA_TIMEOUT;

        trx->global_read_view_heap = mem_heap_create(256);

Index: storage/innobase/include/trx0trx.h
===================================================================
--- storage/innobase/include/trx0trx.h  (revision 3649)
+++ storage/innobase/include/trx0trx.h  (revision 3654)
@@ -615,6 +615,7 @@
                                        mutex to prevent recursive deadlocks.
                                        Protected by both the lock sys mutex
                                        and the trx_t::mutex. */
+       ulint        n_rec_locks; /*!< number of rec locks in this trx */
 };

 #define TRX_MAGIC_N    91118598

[12 Mar 2013 19:15] Sveta Smirnova

Thank you for the report.

Verified as described based on code analysis.

[28 Aug 2014 19:20] Daniel Price

Fixed as of the upcoming 5.7.5 release, and here's the changelog entry:

The "lock_number_of_rows_locked" function used a bit vector to track the
number of record locks held by a transaction. To optimize reporting, the
bit vector was replaced by a simple counter. 

Thank you for the bug report.