MDEV-34296 extern thread_local is a CPU waste

In commit 99bd2260 (MDEV-31558)
we wrongly thought that there would be minimal overhead for accessing
a thread-local variable mariadb_stats.

It turns out that in C++11, each access to an extern thread_local
variable requires conditionally invoking an initialization function.
In fact, the initializer expression of mariadb_stats is dynamic, and
those calls were actually unavoidable.

In C++20, one could declare constinit thread_local variables, but
the address of a thread_local variable (&mariadb_dummy_stats) is not
a compile-time constant. We did not want to declare mariadb_dummy_stats
without thread_local, because then the dummy accesses could lead to
cache line contention between threads.

mariadb_stats: Declare as __thread or __declspec(thread) so that
there will be no dynamic initialization, but zero-initialization.

mariadb_dummy_stats: Remove. It is a lesser evil to let
the environment perform zero-initialization and check if
!mariadb_stats.

Reviewed by: Sergei Petrunia

storage/innobase/buf/buf0buf.cc

@@ -2165,6 +2165,12 @@ void buf_page_free(fil_space_t *space, uint32_t page, mtr_t *mtr)
   mtr->memo_push(block, MTR_MEMO_PAGE_X_MODIFY);
 }
 
+static void buf_inc_get(ha_handler_stats *stats)
+{
+  mariadb_increment_pages_accessed(stats);
+  ++buf_pool.stat.n_page_gets;
+}
+
 /** Get read access to a compressed page (usually of type
 FIL_PAGE_TYPE_ZBLOB or FIL_PAGE_TYPE_ZBLOB2).
 The page must be released with unfix().
@@ -2180,8 +2186,8 @@ buf_page_t* buf_page_get_zip(const page_id_t page_id, ulint zip_size)
 {
   ut_ad(zip_size);
   ut_ad(ut_is_2pow(zip_size));
-  ++buf_pool.stat.n_page_gets;
-  mariadb_increment_pages_accessed();
+  ha_handler_stats *const stats= mariadb_stats;
+  buf_inc_get(stats);
 
   buf_pool_t::hash_chain &chain= buf_pool.page_hash.cell_get(page_id.fold());
   page_hash_latch &hash_lock= buf_pool.page_hash.lock_get(chain);
@@ -2283,7 +2289,7 @@ buf_page_t* buf_page_get_zip(const page_id_t page_id, ulint zip_size)
   switch (dberr_t err= buf_read_page(page_id, zip_size)) {
   case DB_SUCCESS:
   case DB_SUCCESS_LOCKED_REC:
-    mariadb_increment_pages_read();
+    mariadb_increment_pages_read(stats);
     goto lookup;
   default:
     ib::error() << "Reading compressed page " << page_id
@@ -2460,9 +2466,8 @@ buf_page_get_low(
 	ut_ad(!mtr || !ibuf_inside(mtr)
 	      || ibuf_page_low(page_id, zip_size, FALSE, NULL));
 
-	++buf_pool.stat.n_page_gets;
-        mariadb_increment_pages_accessed();
-
+	ha_handler_stats* const stats = mariadb_stats;
+	buf_inc_get(stats);
 	auto& chain= buf_pool.page_hash.cell_get(page_id.fold());
 	page_hash_latch& hash_lock = buf_pool.page_hash.lock_get(chain);
 loop:
@@ -2533,7 +2538,7 @@ buf_page_get_low(
 	switch (dberr_t local_err = buf_read_page(page_id, zip_size)) {
 	case DB_SUCCESS:
 	case DB_SUCCESS_LOCKED_REC:
-                mariadb_increment_pages_read();
+		mariadb_increment_pages_read(stats);
 		buf_read_ahead_random(page_id, zip_size, ibuf_inside(mtr));
 		break;
 	default:
@@ -3127,8 +3132,7 @@ buf_block_t *buf_page_try_get(const page_id_t page_id, mtr_t *mtr)
   ut_ad(block->page.buf_fix_count());
   ut_ad(block->page.id() == page_id);
 
-  ++buf_pool.stat.n_page_gets;
-  mariadb_increment_pages_accessed();
+  buf_inc_get(mariadb_stats);
   return block;
 }
 

storage/innobase/buf/buf0rea.cc

@@ -300,12 +300,15 @@ buf_read_page_low(
 	}
 
 	ut_ad(bpage->in_file());
-	ulonglong mariadb_timer= 0;
+	ulonglong mariadb_timer = 0;
 
 	if (sync) {
 		thd_wait_begin(nullptr, THD_WAIT_DISKIO);
-		if (mariadb_stats_active())
-		  mariadb_timer= mariadb_measure();
+		if (const ha_handler_stats *stats = mariadb_stats) {
+			if (stats->active) {
+				mariadb_timer = mariadb_measure();
+			}
+		}
 	}
 
 	DBUG_LOG("ib_buf",
@@ -324,15 +327,16 @@ buf_read_page_low(
 	if (UNIV_UNLIKELY(fio.err != DB_SUCCESS)) {
 		buf_pool.corrupted_evict(bpage, buf_page_t::READ_FIX);
 	} else if (sync) {
-		thd_wait_end(NULL);
+		thd_wait_end(nullptr);
 		/* The i/o was already completed in space->io() */
 		fio.err = bpage->read_complete(*fio.node);
 		space->release();
 		if (fio.err == DB_FAIL) {
 			fio.err = DB_PAGE_CORRUPTED;
 		}
-		if (mariadb_timer)
-		  mariadb_increment_pages_read_time(mariadb_timer);
+		if (mariadb_timer) {
+			mariadb_increment_pages_read_time(mariadb_timer);
+		}
 	}
 
 	return fio.err;

storage/innobase/handler/ha_innodb.cc

@@ -110,8 +110,7 @@ extern my_bool opt_readonly;
 #include "ut0mem.h"
 #include "row0ext.h"
 #include "mariadb_stats.h"
-thread_local ha_handler_stats mariadb_dummy_stats;
-thread_local ha_handler_stats *mariadb_stats= &mariadb_dummy_stats;
+simple_thread_local ha_handler_stats *mariadb_stats;
 
 #include <limits>
 #include <myisamchk.h>                          // TT_FOR_UPGRADE

storage/innobase/include/mariadb_stats.h

@@ -16,54 +16,67 @@ this program; if not, write to the Free Software Foundation, Inc.,
 
 *****************************************************************************/
 
-#ifndef mariadb_stats_h
-#define mariadb_stats_h
-
-/* Include file to handle mariadbd handler specific stats */
+#pragma once
 
 #include "ha_handler_stats.h"
 #include "my_rdtsc.h"
 
-/* Not active threads are ponting to this structure */
-extern thread_local ha_handler_stats mariadb_dummy_stats;
+/* We do not want a dynamic initialization function to be
+conditionally invoked on each access to a C++11 extern thread_local. */
+#if __cplusplus >= 202002L
+# define simple_thread_local constinit thread_local
+#else
+# define simple_thread_local IF_WIN(__declspec(thread),__thread)
+#endif
 
-/* Points to either THD->handler_stats or mariad_dummy_stats */
-extern thread_local ha_handler_stats *mariadb_stats;
+/** Pointer to handler::active_handler_stats or nullptr (via .tbss) */
+extern simple_thread_local ha_handler_stats *mariadb_stats;
 
 /*
-  Returns 1 if MariaDB wants engine status
+  Returns nonzero if MariaDB wants engine status
 */
 
-inline bool mariadb_stats_active()
+inline uint mariadb_stats_active()
 {
-  return mariadb_stats->active != 0;
+  if (ha_handler_stats *stats= mariadb_stats)
+    return stats->active;
+  return 0;
 }
 
-inline bool mariadb_stats_active(ha_handler_stats *stats)
+/* The following functions increment different engine status */
+
+inline void mariadb_increment_pages_accessed(ha_handler_stats *stats)
 {
-  return stats->active != 0;
+  if (stats)
+    stats->pages_accessed++;
 }
 
-/* The following functions increment different engine status */
-
 inline void mariadb_increment_pages_accessed()
 {
-  mariadb_stats->pages_accessed++;
+  mariadb_increment_pages_accessed(mariadb_stats);
 }
 
 inline void mariadb_increment_pages_updated(ulonglong count)
 {
-  mariadb_stats->pages_updated+= count;
+  if (ha_handler_stats *stats= mariadb_stats)
+    stats->pages_updated+= count;
+}
+
+inline void mariadb_increment_pages_read(ha_handler_stats *stats)
+{
+  if (stats)
+    stats->pages_read_count++;
 }
 
 inline void mariadb_increment_pages_read()
 {
-  mariadb_stats->pages_read_count++;
+  mariadb_increment_pages_read(mariadb_stats);
 }
 
 inline void mariadb_increment_undo_records_read()
 {
-  mariadb_stats->undo_records_read++;
+  if (ha_handler_stats *stats= mariadb_stats)
+    stats->undo_records_read++;
 }
 
 /*
@@ -92,7 +105,7 @@ inline void mariadb_increment_pages_read_time(ulonglong start_time)
   ulonglong end_time= mariadb_measure();
   /* Check that we only call this if active, see example! */
   DBUG_ASSERT(start_time);
-  DBUG_ASSERT(mariadb_stats_active(stats));
+  DBUG_ASSERT(stats->active);
 
   stats->pages_read_time+= (end_time - start_time);
 }
@@ -105,15 +118,12 @@ inline void mariadb_increment_pages_read_time(ulonglong start_time)
 class mariadb_set_stats
 {
 public:
-  uint flag;
   mariadb_set_stats(ha_handler_stats *stats)
   {
-    mariadb_stats= stats ? stats : &mariadb_dummy_stats;
+    mariadb_stats= stats;
   }
   ~mariadb_set_stats()
   {
-    mariadb_stats= &mariadb_dummy_stats;
+    mariadb_stats= nullptr;
   }
 };
-
-#endif /* mariadb_stats_h */