wrapper: use trace2 counters to collect fsync stats

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
maint
Beat Bolli 2023-07-20 18:48:23 +02:00 committed by Junio C Hamano
parent cba07a324d
commit a27eecea75
6 changed files with 19 additions and 26 deletions

View File

@ -55,7 +55,7 @@ check_fsync_events () {


cat >expect && cat >expect &&
sed -n \ sed -n \
-e '/^{"event":"data",.*"category":"fsync",/ { -e '/^{"event":"counter",.*"category":"fsync",/ {
s/.*"category":"fsync",//; s/.*"category":"fsync",//;
s/}$//; s/}$//;
p; p;
@ -78,8 +78,8 @@ test_expect_success 'unpack big object in stream (core.fsyncmethod=batch)' '
flush_count=1 flush_count=1
fi && fi &&
check_fsync_events trace2.txt <<-EOF && check_fsync_events trace2.txt <<-EOF &&
"key":"fsync/writeout-only","value":"6" "name":"writeout-only","count":6
"key":"fsync/hardware-flush","value":"$flush_count" "name":"hardware-flush","count":$flush_count
EOF EOF


test_dir_is_empty dest.git/objects/pack && test_dir_is_empty dest.git/objects/pack &&

View File

@ -276,7 +276,6 @@ void trace2_cmd_exit_fl(const char *file, int line, int code)
if (!trace2_enabled) if (!trace2_enabled)
return; return;


trace_git_fsync_stats();
trace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT); trace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT);


tr2main_exit_code = code; tr2main_exit_code = code;

View File

@ -552,6 +552,10 @@ enum trace2_counter_id {
TRACE2_COUNTER_ID_TEST1 = 0, /* emits summary event only */ TRACE2_COUNTER_ID_TEST1 = 0, /* emits summary event only */
TRACE2_COUNTER_ID_TEST2, /* emits summary and thread events */ 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. */ /* Add additional counter definitions before here. */
TRACE2_NUMBER_OF_COUNTERS TRACE2_NUMBER_OF_COUNTERS
}; };

View File

@ -27,6 +27,16 @@ static struct tr2_counter_metadata tr2_counter_metadata[TRACE2_NUMBER_OF_COUNTER
.name = "test2", .name = "test2",
.want_per_thread_events = 1, .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. */ /* Add additional metadata before here. */
}; };

View File

@ -10,9 +10,6 @@
#include "strbuf.h" #include "strbuf.h"
#include "trace2.h" #include "trace2.h"


static intmax_t count_fsync_writeout_only;
static intmax_t count_fsync_hardware_flush;

#ifdef HAVE_RTLGENRANDOM #ifdef HAVE_RTLGENRANDOM
/* This is required to get access to RtlGenRandom. */ /* This is required to get access to RtlGenRandom. */
#define SystemFunction036 NTAPI SystemFunction036 #define SystemFunction036 NTAPI SystemFunction036
@ -551,7 +548,7 @@ int git_fsync(int fd, enum fsync_action action)
{ {
switch (action) { switch (action) {
case FSYNC_WRITEOUT_ONLY: case FSYNC_WRITEOUT_ONLY:
count_fsync_writeout_only += 1; trace2_counter_add(TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY, 1);


#ifdef __APPLE__ #ifdef __APPLE__
/* /*
@ -583,7 +580,7 @@ int git_fsync(int fd, enum fsync_action action)
return -1; return -1;


case FSYNC_HARDWARE_FLUSH: 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 * 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) static int warn_if_unremovable(const char *op, const char *file, int rc)
{ {
int err; int err;

View File

@ -87,11 +87,6 @@ enum fsync_action {
*/ */
int git_fsync(int fd, enum fsync_action 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. * Preserves errno, prints a message, but gives no warning for ENOENT.
* Returns 0 on success, which includes trying to unlink an object that does * Returns 0 on success, which includes trying to unlink an object that does