[PATCH 1/2] trace2: fix a comment

[PATCH 1/2] trace2: fix a comment

[Beat Bolli]

When the trace2 counter mechanism was added in 81071626ba (trace2: add
global counter mechanism, 2022-10-24), the name of the file where new
counters are added was misspelled in a comment.

Use the correct file name.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 trace2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/trace2.h b/trace2.h
index f5c5a9e6bac5..64c747c1df1b 100644
--- a/trace2.h
+++ b/trace2.h
@@ -541,7 +541,7 @@ void trace2_timer_stop(enum trace2_timer_id tid);
  * elsewhere as array indexes).
  *
  * Any values added to this enum be also be added to the
- * `tr2_counter_metadata[]` in `trace2/tr2_tr2_ctr.c`.
+ * `tr2_counter_metadata[]` in `trace2/tr2_ctr.c`.
  */
 enum trace2_counter_id {
 	/*
-- 
2.41.0

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

[Beat Bolli]

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

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

[Junio C Hamano]

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.

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

[Beat Bolli]

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

Convert the two fsync static variables to trace2 counters, reducing the
coupling between wrapper.c and the trace2 subsystem. Adjust t/t5351 to
match the trace2 counter output format.

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>
---
v2:
- Adjust t/t5351
- Update commit message

 t/t5351-unpack-large-objects.sh |  6 +++---
 trace2.c                        |  1 -
 trace2.h                        |  4 ++++
 trace2/tr2_ctr.c                | 10 ++++++++++
 wrapper.c                       | 19 ++-----------------
 wrapper.h                       |  5 -----
 6 files changed, 19 insertions(+), 26 deletions(-)

diff --git a/t/t5351-unpack-large-objects.sh b/t/t5351-unpack-large-objects.sh
index 8c8af99b844b..43cbcd5d497e 100755
--- a/t/t5351-unpack-large-objects.sh
+++ b/t/t5351-unpack-large-objects.sh
@@ -55,7 +55,7 @@ check_fsync_events () {
 
 	cat >expect &&
 	sed -n \
-		-e '/^{"event":"data",.*"category":"fsync",/ {
+		-e '/^{"event":"counter",.*"category":"fsync",/ {
 			s/.*"category":"fsync",//;
 			s/}$//;
 			p;
@@ -78,8 +78,8 @@ test_expect_success 'unpack big object in stream (core.fsyncmethod=batch)' '
 		flush_count=1
 	fi &&
 	check_fsync_events trace2.txt <<-EOF &&
-	"key":"fsync/writeout-only","value":"6"
-	"key":"fsync/hardware-flush","value":"$flush_count"
+	"name":"writeout-only","count":6
+	"name":"hardware-flush","count":$flush_count
 	EOF
 
 	test_dir_is_empty dest.git/objects/pack &&
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..6491d25396e0 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

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

[Junio C Hamano]

Beat Bolli <dev+git@drbeat.li> writes:

> As mentioned in the thread starting at [1], trace2 counters should be
> used to count events instead of ad-hoc static variables.
>
> Convert the two fsync static variables to trace2 counters, reducing the
> coupling between wrapper.c and the trace2 subsystem. Adjust t/t5351 to
> match the trace2 counter output format.
>
> 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>
> ---
> v2:
> - Adjust t/t5351
> - Update commit message

I also spotted this change since v1:

- Rename trace2 counters to use "-" (not "_") as inter-word separators.

Since I do not seem to be able to find any review comments regarding
the variable naming in the v1's thread, let's ask stakeholders.

Are folks involved in the trace2 subsystem (especially Jeff
Hostetler---already CC:ed---who presumably has the most stake in it)
OK with the naming convention of the multi-word variable?  This is
the first use of multi-word variable name in tr2_ctr, and thus will
establish whatever convention you guys want to use.  I do have a
slight preference of "writeout-only" over "writeout_only" but that
is purely from visual appearance.  If there is a desire to keep the
names literally reusable as identifiers in some languages used to
postprocess trace output, or something, that might weigh
differently.

>  t/t5351-unpack-large-objects.sh |  6 +++---
>  trace2.c                        |  1 -
>  trace2.h                        |  4 ++++
>  trace2/tr2_ctr.c                | 10 ++++++++++
>  wrapper.c                       | 19 ++-----------------
>  wrapper.h                       |  5 -----
>  6 files changed, 19 insertions(+), 26 deletions(-)

Very nice to see clean-up patch that reduces the amount of code.
Nicely done.

Thanks, will queue.  If folks do not find issues in a few days,
let's merge it to 'next'.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I also spotted this change since v1:
>
> - Rename trace2 counters to use "-" (not "_") as inter-word separators.
>
> Since I do not seem to be able to find any review comments regarding
> the variable naming in the v1's thread, let's ask stakeholders.
>
> Are folks involved in the trace2 subsystem (especially Jeff
> Hostetler---already CC:ed---who presumably has the most stake in it)
> OK with the naming convention of the multi-word variable?  This is
> the first use of multi-word variable name in tr2_ctr, and thus will
> establish whatever convention you guys want to use.  I do have a
> slight preference of "writeout-only" over "writeout_only" but that
> is purely from visual appearance.  If there is a desire to keep the
> names literally reusable as identifiers in some languages used to
> postprocess trace output, or something, that might weigh
> differently.

I heard absolutely nothing since I asked the above question last
week, so I'll take the absense of response as absense of interest in
the way how names are spelled.

Therefore, let me make a unilateral declaration here ;-)  The trace2
counters with multi-word names are to be named using "-" as their
inter-word separators.  Any patch that adds new counters that do not
follow the convention will silently dropped on the floor from now on.

Let's move this patch forward by merging to 'next' soonish.

Thanks.

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

[Beat Bolli]

On 25.07.23 21:31, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> I also spotted this change since v1:
>>
>> - Rename trace2 counters to use "-" (not "_") as inter-word separators.
>>
>> Since I do not seem to be able to find any review comments regarding
>> the variable naming in the v1's thread, let's ask stakeholders.
>>
>> Are folks involved in the trace2 subsystem (especially Jeff
>> Hostetler---already CC:ed---who presumably has the most stake in it)
>> OK with the naming convention of the multi-word variable?  This is
>> the first use of multi-word variable name in tr2_ctr, and thus will
>> establish whatever convention you guys want to use.  I do have a
>> slight preference of "writeout-only" over "writeout_only" but that
>> is purely from visual appearance.  If there is a desire to keep the
>> names literally reusable as identifiers in some languages used to
>> postprocess trace output, or something, that might weigh
>> differently.
> 
> I heard absolutely nothing since I asked the above question last
> week, so I'll take the absense of response as absense of interest in
> the way how names are spelled.
> 
> Therefore, let me make a unilateral declaration here ;-)  The trace2
> counters with multi-word names are to be named using "-" as their
> inter-word separators.  Any patch that adds new counters that do not
> follow the convention will silently dropped on the floor from now on.
> 
> Let's move this patch forward by merging to 'next' soonish.

Works for me :-)

Cheers!

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

[Jeff Hostetler]

On 7/25/23 7:03 PM, Beat Bolli wrote:
> On 25.07.23 21:31, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> I also spotted this change since v1:
>>>
>>> - Rename trace2 counters to use "-" (not "_") as inter-word separators.
>>>
>>> Since I do not seem to be able to find any review comments regarding
>>> the variable naming in the v1's thread, let's ask stakeholders.
>>>
>>> Are folks involved in the trace2 subsystem (especially Jeff
>>> Hostetler---already CC:ed---who presumably has the most stake in it)
>>> OK with the naming convention of the multi-word variable?  This is
>>> the first use of multi-word variable name in tr2_ctr, and thus will
>>> establish whatever convention you guys want to use.  I do have a
>>> slight preference of "writeout-only" over "writeout_only" but that
>>> is purely from visual appearance.  If there is a desire to keep the
>>> names literally reusable as identifiers in some languages used to
>>> postprocess trace output, or something, that might weigh
>>> differently.
>>
>> I heard absolutely nothing since I asked the above question last
>> week, so I'll take the absense of response as absense of interest in
>> the way how names are spelled.
>>
>> Therefore, let me make a unilateral declaration here ;-)  The trace2
>> counters with multi-word names are to be named using "-" as their
>> inter-word separators.  Any patch that adds new counters that do not
>> follow the convention will silently dropped on the floor from now on.
>>
>> Let's move this patch forward by merging to 'next' soonish.
> 
> Works for me :-)
> 
> Cheers!
> 

Agreed.

Thanks
Jeff

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

[Jeff Hostetler]

On 7/25/23 3:31 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> I also spotted this change since v1:
>>
>> - Rename trace2 counters to use "-" (not "_") as inter-word separators.
>>
>> Since I do not seem to be able to find any review comments regarding
>> the variable naming in the v1's thread, let's ask stakeholders.
>>
>> Are folks involved in the trace2 subsystem (especially Jeff
>> Hostetler---already CC:ed---who presumably has the most stake in it)
>> OK with the naming convention of the multi-word variable?  This is
>> the first use of multi-word variable name in tr2_ctr, and thus will
>> establish whatever convention you guys want to use.  I do have a
>> slight preference of "writeout-only" over "writeout_only" but that
>> is purely from visual appearance.  If there is a desire to keep the
>> names literally reusable as identifiers in some languages used to
>> postprocess trace output, or something, that might weigh
>> differently.
> 
> I heard absolutely nothing since I asked the above question last
> week, so I'll take the absense of response as absense of interest in
> the way how names are spelled.
> 
> Therefore, let me make a unilateral declaration here ;-)  The trace2
> counters with multi-word names are to be named using "-" as their
> inter-word separators.  Any patch that adds new counters that do not
> follow the convention will silently dropped on the floor from now on.
> 
> Let's move this patch forward by merging to 'next' soonish.
> 
> Thanks.

Sorry I missed before I left for vacation.

Multi-word terms have unfortunately used both "-" and "_"
separators in the past (e.g. builtin/pack-objects.c)
I don't think it really matters one way or the other.

Originally, I used "_" because there were places where the
post-processing could more easily extract or query a nested JSON
or Kusto expression without needing escapes. For example
`<record>.<category>.<item>` rather than something like
`<record>["<category>"]["<item>"]` to avoid having the dash
interpreted as subtraction on a local variable).

But as I and others have added other categories and messages,
we've drifted from that usage.  And that is fine.

Thanks
Jeff