diff mbox series

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

Message ID 20230719232444.555838-2-dev+git@drbeat.li (mailing list archive)
State New, archived
Headers show
Series [1/2] trace2: fix a comment | expand

Commit Message

Beat Bolli July 19, 2023, 11:24 p.m. UTC 
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(-)

Comments

Junio C Hamano July 20, 2023, 12:12 a.m. UTC | #1 
Beat Bolli <dev+git@drbeat.li> writes:

> 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.

Thanks for leaving a note.  This one was trivial enough to resolve,
but it is a good discipline to always make trial merges to 'next'
and with other topics in flight.

Did t5351 pass for you with this patch?  Any other test breakages
that the patch needs to also adjust?

Thanks.
diff mbox series

Patch

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