[PATCH 2/2] wrapper: use trace2 counters to collect fsync stats

From: Beat Bolli <dev+git@drbeat.li>
Date: Jul 19, 2023, 23:24 UTC
Message-ID: <20230719232444.555838-2-dev+git@drbeat.li>
In-Reply-To: <20230719232444.555838-1-dev+git@drbeat.li>

As mentioned in the subthread starting at [1], trace2 counters should be used to count events instead of ad-hoc static variables.

Convert the static variables that count fsync calls to trace2 counters, reducing the coupling between wrapper.c and the trace2 subsystem.

The counters are not per-thread because the ones being replaced also were not.

[1] https://lore.kernel.org/git/20230627195251.1973421-2-calvinwan@google.com/

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
I have based this series on master, so this patch will create a trivial
merge conflict with c489f47a649d (refs/packed-backend.c: add trace2
counters for jump list, 2023-07-10) on next, which also adds a new
counter.

 trace2.c         |  1 -
 trace2.h         |  4 ++++
 trace2/tr2_ctr.c | 10 ++++++++++
 wrapper.c        | 19 ++-----------------
 wrapper.h        |  5 -----
 5 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/trace2.c b/trace2.c
index 49c23bfd05a7..6dc74dff4c73 100644
--- a/trace2.c
+++ b/trace2.c
@@ -276,7 +276,6 @@ void trace2_cmd_exit_fl(const char *file, int line, int code)
 	if (!trace2_enabled)
 		return;
 
-	trace_git_fsync_stats();
 	trace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT);
 
 	tr2main_exit_code = code;
diff --git a/trace2.h b/trace2.h
index 64c747c1df1b..12211d3bd61b 100644
--- a/trace2.h
+++ b/trace2.h
@@ -552,6 +552,10 @@ enum trace2_counter_id {
 	TRACE2_COUNTER_ID_TEST1 = 0, /* emits summary event only */
 	TRACE2_COUNTER_ID_TEST2,     /* emits summary and thread events */
 
+	/* counts number of fsyncs */
+	TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY,
+	TRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH,
+
 	/* Add additional counter definitions before here. */
 	TRACE2_NUMBER_OF_COUNTERS
 };
diff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c
index b342d3b1a3c0..14a082651001 100644
--- a/trace2/tr2_ctr.c
+++ b/trace2/tr2_ctr.c
@@ -27,6 +27,16 @@ static struct tr2_counter_metadata tr2_counter_metadata[TRACE2_NUMBER_OF_COUNTER
 		.name = "test2",
 		.want_per_thread_events = 1,
 	},
+	[TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY] = {
+		.category = "fsync",
+		.name = "writeout_only",
+		.want_per_thread_events = 0,
+	},
+	[TRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH] = {
+		.category = "fsync",
+		.name = "hardware_flush",
+		.want_per_thread_events = 0,
+	},
 
 	/* Add additional metadata before here. */
 };
diff --git a/wrapper.c b/wrapper.c
index 22be9812a724..dea54a326073 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -10,9 +10,6 @@
 #include "strbuf.h"
 #include "trace2.h"
 
-static intmax_t count_fsync_writeout_only;
-static intmax_t count_fsync_hardware_flush;
-
 #ifdef HAVE_RTLGENRANDOM
 /* This is required to get access to RtlGenRandom. */
 #define SystemFunction036 NTAPI SystemFunction036
@@ -551,7 +548,7 @@ int git_fsync(int fd, enum fsync_action action)
 {
 	switch (action) {
 	case FSYNC_WRITEOUT_ONLY:
-		count_fsync_writeout_only += 1;
+		trace2_counter_add(TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY, 1);
 
 #ifdef __APPLE__
 		/*
@@ -583,7 +580,7 @@ int git_fsync(int fd, enum fsync_action action)
 		return -1;
 
 	case FSYNC_HARDWARE_FLUSH:
-		count_fsync_hardware_flush += 1;
+		trace2_counter_add(TRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH, 1);
 
 		/*
 		 * On macOS, a special fcntl is required to really flush the
@@ -600,18 +597,6 @@ int git_fsync(int fd, enum fsync_action action)
 	}
 }
 
-static void log_trace_fsync_if(const char *key, intmax_t value)
-{
-	if (value)
-		trace2_data_intmax("fsync", the_repository, key, value);
-}
-
-void trace_git_fsync_stats(void)
-{
-	log_trace_fsync_if("fsync/writeout-only", count_fsync_writeout_only);
-	log_trace_fsync_if("fsync/hardware-flush", count_fsync_hardware_flush);
-}
-
 static int warn_if_unremovable(const char *op, const char *file, int rc)
 {
 	int err;
diff --git a/wrapper.h b/wrapper.h
index c85b1328d163..79a9c1b5077b 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -87,11 +87,6 @@ enum fsync_action {
  */
 int git_fsync(int fd, enum fsync_action action);
 
-/*
- * Writes out trace statistics for fsync using the trace2 API.
- */
-void trace_git_fsync_stats(void);
-
 /*
  * Preserves errno, prints a message, but gives no warning for ENOENT.
  * Returns 0 on success, which includes trying to unlink an object that does

-- 
2.41.0