Skip to content

Commit a27eece

Browse files
bbolligitster
authored andcommitted
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/[email protected]/ Signed-off-by: Beat Bolli <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent cba07a3 commit a27eece

File tree

6 files changed

+19
-26
lines changed

6 files changed

+19
-26
lines changed

t/t5351-unpack-large-objects.sh

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ check_fsync_events () {
5555

5656
cat >expect &&
5757
sed -n \
58-
-e '/^{"event":"data",.*"category":"fsync",/ {
58+
-e '/^{"event":"counter",.*"category":"fsync",/ {
5959
s/.*"category":"fsync",//;
6060
s/}$//;
6161
p;
@@ -78,8 +78,8 @@ test_expect_success 'unpack big object in stream (core.fsyncmethod=batch)' '
7878
flush_count=1
7979
fi &&
8080
check_fsync_events trace2.txt <<-EOF &&
81-
"key":"fsync/writeout-only","value":"6"
82-
"key":"fsync/hardware-flush","value":"$flush_count"
81+
"name":"writeout-only","count":6
82+
"name":"hardware-flush","count":$flush_count
8383
EOF
8484
8585
test_dir_is_empty dest.git/objects/pack &&

trace2.c

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,6 @@ void trace2_cmd_exit_fl(const char *file, int line, int code)
276276
if (!trace2_enabled)
277277
return;
278278

279-
trace_git_fsync_stats();
280279
trace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT);
281280

282281
tr2main_exit_code = code;

trace2.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,10 @@ enum trace2_counter_id {
552552
TRACE2_COUNTER_ID_TEST1 = 0, /* emits summary event only */
553553
TRACE2_COUNTER_ID_TEST2, /* emits summary and thread events */
554554

555+
/* counts number of fsyncs */
556+
TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY,
557+
TRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH,
558+
555559
/* Add additional counter definitions before here. */
556560
TRACE2_NUMBER_OF_COUNTERS
557561
};

trace2/tr2_ctr.c

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,16 @@ static struct tr2_counter_metadata tr2_counter_metadata[TRACE2_NUMBER_OF_COUNTER
2727
.name = "test2",
2828
.want_per_thread_events = 1,
2929
},
30+
[TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY] = {
31+
.category = "fsync",
32+
.name = "writeout-only",
33+
.want_per_thread_events = 0,
34+
},
35+
[TRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH] = {
36+
.category = "fsync",
37+
.name = "hardware-flush",
38+
.want_per_thread_events = 0,
39+
},
3040

3141
/* Add additional metadata before here. */
3242
};

wrapper.c

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,6 @@
1010
#include "strbuf.h"
1111
#include "trace2.h"
1212

13-
static intmax_t count_fsync_writeout_only;
14-
static intmax_t count_fsync_hardware_flush;
15-
1613
#ifdef HAVE_RTLGENRANDOM
1714
/* This is required to get access to RtlGenRandom. */
1815
#define SystemFunction036 NTAPI SystemFunction036
@@ -551,7 +548,7 @@ int git_fsync(int fd, enum fsync_action action)
551548
{
552549
switch (action) {
553550
case FSYNC_WRITEOUT_ONLY:
554-
count_fsync_writeout_only += 1;
551+
trace2_counter_add(TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY, 1);
555552

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

585582
case FSYNC_HARDWARE_FLUSH:
586-
count_fsync_hardware_flush += 1;
583+
trace2_counter_add(TRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH, 1);
587584

588585
/*
589586
* On macOS, a special fcntl is required to really flush the
@@ -600,18 +597,6 @@ int git_fsync(int fd, enum fsync_action action)
600597
}
601598
}
602599

603-
static void log_trace_fsync_if(const char *key, intmax_t value)
604-
{
605-
if (value)
606-
trace2_data_intmax("fsync", the_repository, key, value);
607-
}
608-
609-
void trace_git_fsync_stats(void)
610-
{
611-
log_trace_fsync_if("fsync/writeout-only", count_fsync_writeout_only);
612-
log_trace_fsync_if("fsync/hardware-flush", count_fsync_hardware_flush);
613-
}
614-
615600
static int warn_if_unremovable(const char *op, const char *file, int rc)
616601
{
617602
int err;

wrapper.h

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,6 @@ enum fsync_action {
8787
*/
8888
int git_fsync(int fd, enum fsync_action action);
8989

90-
/*
91-
* Writes out trace statistics for fsync using the trace2 API.
92-
*/
93-
void trace_git_fsync_stats(void);
94-
9590
/*
9691
* Preserves errno, prints a message, but gives no warning for ENOENT.
9792
* Returns 0 on success, which includes trying to unlink an object that does

0 commit comments

Comments
 (0)