Improve efficiency of wait event reporting, remove proc.h dependency.

anarazel · anarazel · commit 225a22b19ed2 · 2021-04-03T12:03:45.000-07:00
pgstat_report_wait_start() and pgstat_report_wait_end() required two conditional branches so far. One to check if MyProc is NULL, the other to check if pgstat_track_activities is set. As wait events are used around comparatively lightweight operations, and are inlined (reducing branch predictor effectiveness), that's not great. The dependency on MyProc has a second disadvantage: Low-level subsystems, like storage/file/fd.c, report wait events, but architecturally it is preferable for them to not depend on inter-process subsystems like proc.h (defining PGPROC). After this change including pgstat.h (nor obviously its sub-components like backend_status.h, wait_event.h, ...) does not pull in IPC related headers anymore. These goals, efficiency and abstraction, are achieved by having pgstat_report_wait_start/end() not interact with MyProc, but instead a new my_wait_event_info variable. At backend startup it points to a local variable, removing the need to check for MyProc being NULL. During process initialization my_wait_event_info is redirected to MyProc->wait_event_info. At shutdown this is reversed. Because wait event reporting now does not need to know about where the wait event is stored, it does not need to know about PGPROC anymore. The removal of the branch for checking pgstat_track_activities is simpler: Don't check anymore. The cost due to the branch are often higher than the store - and even if not, pgstat_track_activities is rarely disabled. The main motivator to commit this work now is that removing the (indirect) pgproc.h include from pgstat.h simplifies a patch to move statistics reporting to shared memory (which still has a chance to get into 14). Author: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/20210402194458.2vu324hkk2djq6ce@alap3.anarazel.de
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
@@ -448,6 +448,9 @@ InitProcess(void)
 	OwnLatch(&MyProc->procLatch);
 	SwitchToSharedLatch();
 
+	/* now that we have a proc, report wait events to shared memory */
+	pgstat_set_wait_event_storage(&MyProc->wait_event_info);
+
 	/*
 	 * We might be reusing a semaphore that belonged to a failed process. So
 	 * be careful and reinitialize its value here.  (This is not strictly
@@ -601,6 +604,9 @@ InitAuxiliaryProcess(void)
 	OwnLatch(&MyProc->procLatch);
 	SwitchToSharedLatch();
 
+	/* now that we have a proc, report wait events to shared memory */
+	pgstat_set_wait_event_storage(&MyProc->wait_event_info);
+
 	/* Check that group locking fields are in a proper initial state. */
 	Assert(MyProc->lockGroupLeader == NULL);
 	Assert(dlist_is_empty(&MyProc->lockGroupMembers));
@@ -884,10 +890,15 @@ ProcKill(int code, Datum arg)
 	/*
 	 * Reset MyLatch to the process local one.  This is so that signal
 	 * handlers et al can continue using the latch after the shared latch
-	 * isn't ours anymore. After that clear MyProc and disown the shared
-	 * latch.
+	 * isn't ours anymore.
+	 *
+	 * Similarly, stop reporting wait events to MyProc->wait_event_info.
+	 *
+	 * After that clear MyProc and disown the shared latch.
 	 */
 	SwitchBackToLocalLatch();
+	pgstat_reset_wait_event_storage();
+
 	proc = MyProc;
 	MyProc = NULL;
 	DisownLatch(&proc->procLatch);
@@ -952,13 +963,10 @@ AuxiliaryProcKill(int code, Datum arg)
 	/* Cancel any pending condition variable sleep, too */
 	ConditionVariableCancelSleep();
 
-	/*
-	 * Reset MyLatch to the process local one.  This is so that signal
-	 * handlers et al can continue using the latch after the shared latch
-	 * isn't ours anymore. After that clear MyProc and disown the shared
-	 * latch.
-	 */
+	/* look at the equivalent ProcKill() code for comments */
 	SwitchBackToLocalLatch();
+	pgstat_reset_wait_event_storage();
+
 	proc = MyProc;
 	MyProc = NULL;
 	DisownLatch(&proc->procLatch);
diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
@@ -7,6 +7,17 @@
  *
  * IDENTIFICATION
  *	  src/backend/postmaster/wait_event.c
+ *
+ * NOTES
+ *
+ * To make pgstat_report_wait_start() and pgstat_report_wait_end() as
+ * lightweight as possible, they do not check if shared memory (MyProc
+ * specifically, where the wait event is stored) is already available. Instead
+ * we initially set my_wait_event_info to a process local variable, which then
+ * is redirected to shared memory using pgstat_set_wait_event_storage(). For
+ * the same reason pgstat_track_activities is not checked - the check adds
+ * more work than it saves.
+ *
  * ----------
  */
 #include "postgres.h"
@@ -23,6 +34,36 @@ static const char *pgstat_get_wait_timeout(WaitEventTimeout w);
 static const char *pgstat_get_wait_io(WaitEventIO w);
 
 
+static uint32 local_my_wait_event_info;
+uint32	   *my_wait_event_info = &local_my_wait_event_info;
+
+
+/*
+ * Configure wait event reporting to report wait events to *wait_event_info.
+ * *wait_event_info needs to be valid until pgstat_reset_wait_event_storage()
+ * is called.
+ *
+ * Expected to be called during backend startup, to point my_wait_event_info
+ * into shared memory.
+ */
+void
+pgstat_set_wait_event_storage(uint32 *wait_event_info)
+{
+	my_wait_event_info = wait_event_info;
+}
+
+/*
+ * Reset wait event storage location.
+ *
+ * Expected to be called during backend shutdown, before the location set up
+ * pgstat_set_wait_event_storage() becomes invalid.
+ */
+void
+pgstat_reset_wait_event_storage(void)
+{
+	my_wait_event_info = &local_my_wait_event_info;
+}
+
 /* ----------
  * pgstat_get_wait_event_type() -
  *
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
@@ -11,9 +11,6 @@
 #define WAIT_EVENT_H
 
 
-#include "storage/proc.h" /* for MyProc */
-
-
 /* ----------
  * Wait Classes
  * ----------
@@ -234,13 +231,10 @@ extern const char *pgstat_get_wait_event(uint32 wait_event_info);
 extern const char *pgstat_get_wait_event_type(uint32 wait_event_info);
 static inline void pgstat_report_wait_start(uint32 wait_event_info);
 static inline void pgstat_report_wait_end(void);
+extern void pgstat_set_wait_event_storage(uint32 *wait_event_info);
+extern void pgstat_reset_wait_event_storage(void);
 
-
-/*
- * Repeated here for the inline functions because it is declared in pgstat.h,
- * which includes this header.
- */
-extern PGDLLIMPORT bool pgstat_track_activities;
+extern PGDLLIMPORT uint32 *my_wait_event_info;
 
 
 /* ----------
@@ -254,47 +248,35 @@ extern PGDLLIMPORT bool pgstat_track_activities;
  *	for wait event which is sufficient for current usage, 1-byte is
  *	reserved for future usage.
  *
- * NB: this *must* be able to survive being called before MyProc has been
- * initialized.
+ *	Historically we used to make this reporting conditional on
+ *	pgstat_track_activities, but the check for that seems to add more cost
+ *	than it saves.
+ *
+ *	my_wait_event_info initially points to local memory, making it safe to
+ *	call this before MyProc has been initialized.
  * ----------
  */
 static inline void
 pgstat_report_wait_start(uint32 wait_event_info)
 {
-	volatile PGPROC *proc = MyProc;
-
-	if (!pgstat_track_activities || !proc)
-		return;
-
 	/*
 	 * Since this is a four-byte field which is always read and written as
 	 * four-bytes, updates are atomic.
 	 */
-	proc->wait_event_info = wait_event_info;
+	*(volatile uint32 *) my_wait_event_info = wait_event_info;
 }
 
 /* ----------
  * pgstat_report_wait_end() -
  *
  *	Called to report end of a wait.
- *
- * NB: this *must* be able to survive being called before MyProc has been
- * initialized.
  * ----------
  */
 static inline void
 pgstat_report_wait_end(void)
 {
-	volatile PGPROC *proc = MyProc;
-
-	if (!pgstat_track_activities || !proc)
-		return;
-
-	/*
-	 * Since this is a four-byte field which is always read and written as
-	 * four-bytes, updates are atomic.
-	 */
-	proc->wait_event_info = 0;
+	/* see pgstat_report_wait_start() */
+	*(volatile uint32 *) my_wait_event_info = 0;
 }
 
 
