| Message ID | 20230720164823.625815-1-dev+git@drbeat.li (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a27eecea75b3858b4052b191143f144a7e966869 |
| Headers | show |
| Series | None | expand |
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'.
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.
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!
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
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
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
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(-)