MySQL Bugs: #69576: Wasted work in method lock_rec_other_has_expl

Bug #69576	Wasted work in method lock_rec_other_has_expl_req()
Submitted:	25 Jun 2013 21:23	Modified:	8 Jan 2014 18:59
Reporter:	Po-Chun Chang (OCA)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S5 (Performance)
Version:	5.6, 5.7	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	patch, performance

Description:
The problem appears in version 5.6 and in the revision 5216 in version 5.7. I attached a simple patch that fixes it. The patch just adds one line, this check:

"if (gap || !(heap_no == PAGE_HEAP_NO_SUPREMUM))"

The patch looks large because the entire loop is indented one level,
but the above check is the only code added.

In method "lock_rec_other_has_expl_req" in "lock0lock.cc", the loop on line 
1711 should only be executed when at lease one of "gap" or
"!(heap_no==PAGE_HEAP_NO_SUPREMUM)" are "true",
because the loop has has no side effects when both "gap" and
"!(heap_no==PAGE_HEAP_NO_SUPREMUM)" are "false".
When they are both "false", this expression is false:

(gap || !(lock_rec_get_gap(lock) || heap_no == PAGE_HEAP_NO_SUPREMUM))

and therefore the entire condition inside the loop is "false", which
means that the loop's only side effect (the "return(lock);" statement)
is never executed.  Note that "gap" and "heap_no" are not modified
inside the loop.

How to repeat:
Once lock_rec_other_has_expl_req() is executed.

Suggested fix:
One line check before the loop:
 "if (gap || !(heap_no == PAGE_HEAP_NO_SUPREMUM))"

Suggested patch (*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: patch.diff (text/plain), 1.09 KiB.

Thank you for the report and contribution.

gap is always passed in as 0 by all the callers of lock_rec_other_has_expl_req(). We can  simply remove gap. This is debug code and therefore not important as such for performance. A more complete fix would look like:

=== modified file 'storage/innobase/lock/lock0lock.cc'
--- storage/innobase/lock/lock0lock.cc	revid:vasil.dimov@oracle.com-20130618141427-ceqchpd34zgaheao
+++ storage/innobase/lock/lock0lock.cc	2013-06-26 22:58:26 +0000
@@ -1528,9 +1528,6 @@ const lock_t*
 lock_rec_other_has_expl_req(
 /*========================*/
 	enum lock_mode		mode,	/*!< in: LOCK_S or LOCK_X */
-	ulint			gap,	/*!< in: LOCK_GAP if also gap
-					locks are taken into account,
-					or 0 if not */
 	ulint			wait,	/*!< in: LOCK_WAIT if also
 					waiting locks are taken into
 					account, or 0 if not */
@@ -1541,21 +1538,20 @@ lock_rec_other_has_expl_req(
 					requests by all transactions
 					are taken into account */
 {
-	const lock_t*	lock;
-
 	ut_ad(lock_mutex_own());
 	ut_ad(mode == LOCK_X || mode == LOCK_S);
-	ut_ad(gap == 0 || gap == LOCK_GAP);
 	ut_ad(wait == 0 || wait == LOCK_WAIT);
 
-	for (lock = lock_rec_get_first(block, heap_no);
+	if (heap_no == PAGE_HEAP_NO_SUPREMUM) {
+		return(NULL);
+	}
+
+	for (const lock_t* lock = lock_rec_get_first(block, heap_no);
 	     lock != NULL;
 	     lock = lock_rec_get_next_const(heap_no, lock)) {
 
 		if (lock->trx != trx
-		    && (gap
-			|| !(lock_rec_get_gap(lock)
-			     || heap_no == PAGE_HEAP_NO_SUPREMUM))
+		    && !lock_rec_get_gap(lock)
 		    && (wait || !lock_get_wait(lock))
 		    && lock_mode_stronger_or_eq(lock_get_mode(lock), mode)) {
 
@@ -2006,7 +2002,7 @@ lock_rec_add_to_queue(
 			? LOCK_X
 			: LOCK_S;
 		const lock_t*	other_lock
-			= lock_rec_other_has_expl_req(mode, 0, LOCK_WAIT,
+			= lock_rec_other_has_expl_req(mode, LOCK_WAIT,
 						      block, heap_no, trx);
 		ut_a(!other_lock);
 	}
@@ -5565,7 +5561,7 @@ lock_rec_queue_validate(
 		because lock_trx_release_locks() acquires lock_sys->mutex */
 
 		if (impl_trx != NULL
-		    && lock_rec_other_has_expl_req(LOCK_S, 0, LOCK_WAIT,
+		    && lock_rec_other_has_expl_req(LOCK_S, LOCK_WAIT,
 						   block, heap_no, impl_trx)) {
 
 			ut_a(lock_rec_has_expl(LOCK_X | LOCK_REC_NOT_GAP,
@@ -5593,7 +5589,7 @@ lock_rec_queue_validate(
 				mode = LOCK_S;
 			}
 			ut_a(!lock_rec_other_has_expl_req(
-				     mode, 0, 0, block, heap_no, lock->trx));
+				     mode, 0, block, heap_no, lock->trx));
 
 		} else if (lock_get_wait(lock) && !lock_rec_get_gap(lock)) {

Fixed as of 5.7.4, and here's the changelog entry:

The "lock_rec_other_has_expl_req" function in "lock0lock.cc" would perform
unnecessary work.

Thank you for the bug report.