From: Jeff Hostetler Date: Wed, 22 Dec 2021 22:56:03 GMT Subject: Re: [PATCH 8/9] trace2: add counter events to perf and event target formats Message-ID: In-Reply-To: <211220.86czlrurm6.gmgdl@evledraar.gmail.com> On 12/20/21 11:51 AM, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Dec 20 2021, Jeff Hostetler via GitGitGadget wrote: > >> From: Jeff Hostetler >> [...] > >> +static void fn_counter(uint64_t us_elapsed_absolute, >> + const char *thread_name, >> + const char *category, >> + const char *counter_name, >> + uint64_t value) >> +{ >> + const char *event_name = "counter"; >> + struct strbuf buf_payload = STRBUF_INIT; >> + >> + strbuf_addf(&buf_payload, "name:%s", counter_name); >> + strbuf_addf(&buf_payload, " value:%"PRIu64, value); > > Odd to have these be two seperate strbuf_addf()... yeah, i'll combine. and in the body of fn_timer in 6/9. > ....but more generally, and I see from e.g. the existing fn_version_fl > that you're just using existing patterns, but it seems odd not to have a > trivial varargs fmt helper for perf_io_write_fl that would avoid the > whole strbuf/addf/release dance. [...] yeah, cut-n-paste was used here and i was maintaining consistency with the other functions -- rather than inventing something new and refactoring stuff that didn't need be refactored in the middle of an on-going patch series. > I did a quick experiment to do that, patch on "master" below. A lot of > the boilerplate could be simplified by factoring out the > sq_quote_buf_pretty() case, and even this approach (re)allocs in a way > that looks avoidable in many cases if perf_fmt_prepare() were improved > (but it looks like it nedes its if/while loops in some cases still): > [...] > > +__attribute__((format (printf, 8, 9))) > +static void perf_io_write_fl_fmt(const char *file, int line, const char *event_name, > + const struct repository *repo, > + uint64_t *p_us_elapsed_absolute, > + uint64_t *p_us_elapsed_relative, > + const char *category, > + const char *fmt, ...) > +{ > + va_list ap; > + struct strbuf sb = STRBUF_INIT; > + > + va_start(ap, fmt); > + strbuf_vaddf(&sb, fmt, ap); > + va_end(ap); > + > + perf_io_write_fl(file, line, event_name, repo, p_us_elapsed_absolute, > + p_us_elapsed_relative, category, &sb); > + > + strbuf_release(&sb); > +} > + > static void fn_version_fl(const char *file, int line) > { > const char *event_name = "version"; > - struct strbuf buf_payload = STRBUF_INIT; > - > - strbuf_addstr(&buf_payload, git_version_string); > > - perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL, > - &buf_payload); > - strbuf_release(&buf_payload); > + perf_io_write_fl_fmt(file, line, event_name, NULL, NULL, NULL, NULL, > + "%s", git_version_string); > } [...] Yes, it might be nice to have a _fmt() version as you suggest and simplify many of the existing fn_*() function bodies. It seems like I keep saying this today, but can we discuss that in a new top-level topic and not down inside commit 8/9 of this series? Thanks, Jeff