[RP004] Optimize InnoDB rw_lock s-lock operation cost on high concurrency

- Removes rw_lock_t::reader_thread(*) from except for debug build. It makes s-lock cost "very" expensive at high concurrency, even without any concurrent x-locks. - In addition, removes also updating rw_lock_t::last_s_[file_name|line]. Though legacy code, it also make the s-lock cost expensive. Anyway, this is uncertain information, and not worth to reduce performance. The cost is cache coherency cost amoung cpu cores or chips. The cache coherency bandwidth is limited resource, and should be saved for actual performance. (*) The rw_lock_t::reader_thread was introduced at > commit a8dcfe2 > Author: Marcin Babij <marcin.babij@oracle.com> > Date: Tue Jan 26 13:13:57 2021 +0100 > > Bug #32252477 RW_LOCK COULD STORE AND PRINT INFO ON S-LATCHER, IF THERE IS A SINGLE ONE

Author: buildup-db

MYSQL_VERSION

@@ -3,4 +3,4 @@ MYSQL_VERSION_MINOR=0
 MYSQL_VERSION_PATCH=42
 MYSQL_VERSION_EXTRA=
 MYSQL_VERSION_STABILITY="LTS"
-MYSQL_RP_REVISION="-RP003"
+MYSQL_RP_REVISION="-RP004"

storage/innobase/include/sync0rw.h

@@ -392,9 +392,11 @@ struct rw_lock_t
  thread which already released or passed the lock. */
  std::atomic<std::thread::id> writer_thread;
 
+#ifdef UNIV_DEBUG
  /** XOR of reader threads' IDs. If there is exactly one reader it should allow
  to retrieve the thread ID of that reader. */
  Atomic_xor_of_thread_id reader_thread;
+#endif /* UNIV_DEBUG */
 
  /** Used by sync0arr.cc for thread queueing */
  os_event_t event;

storage/innobase/include/sync0rw.ic

@@ -251,6 +251,7 @@ bool rw_lock_s_lock_low(rw_lock_t *lock, ulint pass [[maybe_unused]],
 
  ut_d(rw_lock_add_debug_info(lock, pass, RW_LOCK_S, location));
 
+#ifdef UNIV_DEBUG
  /* These debugging values are not set safely: they may be incorrect
  or even refer to a line that is invalid for the file name. */
  lock->last_s_file_name = location.filename;
@@ -259,6 +260,7 @@ bool rw_lock_s_lock_low(rw_lock_t *lock, ulint pass [[maybe_unused]],
  std::numeric_limits<decltype(lock->last_s_line)>::max());
  lock->last_s_line = location.line;
  lock->reader_thread.xor_thing(std::this_thread::get_id());
+#endif /* UNIV_DEBUG */
 
  return true; /* locking succeeded */
 }
@@ -353,7 +355,9 @@ static inline void rw_lock_s_unlock_func(IF_DEBUG(ulint pass, )
  ut_ad(lock->lock_word > -X_LOCK_DECR);
  ut_ad(lock->lock_word != 0);
  ut_ad(lock->lock_word < X_LOCK_DECR);
+#ifdef UNIV_DEBUG
  lock->reader_thread.xor_thing(std::this_thread::get_id());
+#endif /* UNIV_DEBUG */
 
  ut_d(rw_lock_remove_debug_info(lock, pass, RW_LOCK_S));
 

storage/innobase/sync/sync0arr.cc

@@ -419,10 +419,12 @@ void sync_array_cell_print(FILE *file, const sync_cell_t *cell) {
 
  const auto readers_count = rw_lock_get_reader_count(rwlock);
  fprintf(file, "number of readers " ULINTPF, readers_count);
+#ifdef UNIV_DEBUG
  if (readers_count == 1) {
  fprintf(file, " (thread id %s)",
  to_string(rwlock->reader_thread.recover_if_single()).c_str());
  }
+#endif /* UNIV_DEBUG */
  fprintf(file,
  ", waiters flag %d"
  ", lock_word: %lx\n"
@@ -574,24 +576,6 @@ bool sync_array_detect_rwlock_deadlock(sync_cell_t *cell, sync_array_t *arr,
  }
  }
  }
- {
- if (rw_lock_get_reader_count(lock) == 1) {
- /* You might be worried about a race-condition: could it happen that the
- number of s-lockers has changed from 1 to say 3, and the XOR we recover in
- the line below corresponds to some unrelated fourth thread?
- For example 0x101 xor 0x110 xor 0x111 = 0x100.
- This isn't a problem in practice, because conflicts(RW_LOCKS,..) is true
- only if the waiter waits for RW_LOCK_X_WAIT, which means it already has
- announced its presence via lock_word, so no more s-locks should be granted
- to not starve it. Thus the number of readers can only decrease. We double
- check it is still 1 after recovering the xor, so it can't be 0 nor torn.*/
- const auto thread = lock->reader_thread.recover_if_single();
- if (rw_lock_get_reader_count(lock) == 1 && thread != none &&
- std::forward<F>(conflicts)(RW_LOCK_S, thread == waiter)) {
- suspects[suspects_cnt++] = thread;
- }
- }
- }
  for (size_t i = 0; i < suspects_cnt; ++i) {
  if (sync_array_deadlock_step(arr, suspects[i], depth)) {
  sync_array_cell_print(stderr, cell);

storage/innobase/sync/sync0rw.cc

@@ -223,7 +223,11 @@ void rw_lock_create_func(rw_lock_t *lock,
  std::numeric_limits<decltype(lock->clocation.line)>::max());
 
  lock->count_os_wait = 0;
+#ifdef UNIV_DEBUG
  lock->last_s_file_name = "not yet reserved";
+#else
+ lock->last_s_file_name = "not recorded";
+#endif /* UNIV_DEBUG */
  lock->last_x_file_name = "not yet reserved";
  lock->last_s_line = 0;
  lock->last_x_line = 0;

unittest/gunit/innodb/sync0rw-t.cc

@@ -137,8 +137,10 @@ TEST(sync0rw, rw_lock_reader_thread) {
  EXPECT_EQ(rw_lock_get_reader_count(rw_locks[0]), 1);
  EXPECT_EQ(rw_lock_get_reader_count(rw_locks[1]), 2);
  EXPECT_EQ(rw_lock_get_reader_count(rw_locks[2]), 0);
+#ifdef UNIV_DEBUG
  EXPECT_EQ(rw_locks[0]->reader_thread.recover_if_single(),
  thread_1_id.load());
+#endif /* UNIV_DEBUG */
 
  FILE *tmp_file = tmpfile();
  EXPECT_NE(tmp_file, nullptr);
@@ -176,9 +178,13 @@ TEST(sync0rw, rw_lock_reader_thread) {
  EXPECT_THAT(cell_print, testing::HasSubstr("number of readers 0, waiters"));
  /* Note that std::to_string(thread_id_to_uint64(thread_id) could be
  something different than to_string(thread_id). */
+#ifdef UNIV_DEBUG
  EXPECT_THAT(cell_print, testing::HasSubstr(
  "number of readers 1 (thread id " +
  to_string(thread_1_id.load()) + "), waiters"));
+#else
+ EXPECT_THAT(cell_print, testing::HasSubstr("number of readers 1, waiters"));
+#endif /* UNIV_DEBUG */
  EXPECT_THAT(cell_print, testing::HasSubstr("number of readers 2, waiters"));
  };