git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 8/9] trace2: add counter events to perf and event target formats

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Dec 20, 2021, 16:51 UTC
Message-ID
<211220.86czlrurm6.gmgdl@evledraar.gmail.com>
In-Reply-To
<3e39c8172f5de572c7b67f381d51631623a41762.1640012469.git.gitgitgadget@gmail.com>
On Mon, Dec 20 2021, Jeff Hostetler via GitGitGadget wrote:
Show 15 quoted lines
> From: Jeff Hostetler <jeffhost@microsoft.com>
> [...]
> diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
> index 9b3905b920c..ca36d44dfd7 100644
> --- a/trace2/tr2_tgt_event.c
> +++ b/trace2/tr2_tgt_event.c
> @@ -23,7 +23,7 @@ static struct tr2_dst tr2dst_event = { TR2_SYSENV_EVENT, 0, 0, 0, 0 };
>   * Verison 1: original version
>   * Version 2: added "too_many_files" event
>   * Version 3: added "child_ready" event
> - * Version 4: added "timer" event
> + * Version 4: added "timer" and "counter" events
>   */
>  #define TR2_EVENT_VERSION "4"
>

Nit on series structure: Earlier we bumped the version to 4, but here we're changing existing version 4 behavior. Would be better IMO just bump it at the end (if at all needed, per: https://lore.kernel.org/git/211201.86zgpk9u3t.gmgdl@evledraar.gmail.com/)

Show 11 quoted lines
> +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()...
Show 5 quoted lines
> +
> +	perf_io_write_fl(__FILE__, __LINE__, event_name, NULL,
> +			 &us_elapsed_absolute, NULL,
> +			 category, &buf_payload, thread_name);
> +	strbuf_release(&buf_payload);

...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.

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):

diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index 2ff9cf70835..bcbb0d8a250 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -153,16 +153,33 @@ static void perf_io_write_fl(const char *file, int line, const char *event_name,
 	strbuf_release(&buf_line);
 }
 
+__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);
 }
 
 static void fn_start_fl(const char *file, int line,
@@ -182,37 +199,25 @@ static void fn_exit_fl(const char *file, int line, uint64_t us_elapsed_absolute,
 		       int code)
 {
 	const char *event_name = "exit";
-	struct strbuf buf_payload = STRBUF_INIT;
 
-	strbuf_addf(&buf_payload, "code:%d", code);
-
-	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 NULL, NULL, &buf_payload);
-	strbuf_release(&buf_payload);
+	perf_io_write_fl_fmt(file, line, event_name, NULL, &us_elapsed_absolute,
+			     NULL, NULL, "code:%d", code);
 }
 
 static void fn_signal(uint64_t us_elapsed_absolute, int signo)
 {
 	const char *event_name = "signal";
-	struct strbuf buf_payload = STRBUF_INIT;
 
-	strbuf_addf(&buf_payload, "signo:%d", signo);
-
-	perf_io_write_fl(__FILE__, __LINE__, event_name, NULL,
-			 &us_elapsed_absolute, NULL, NULL, &buf_payload);
-	strbuf_release(&buf_payload);
+	perf_io_write_fl_fmt(__FILE__, __LINE__, event_name, NULL,
+			 &us_elapsed_absolute, NULL, NULL, "signo:%d", signo);
 }
 
 static void fn_atexit(uint64_t us_elapsed_absolute, int code)
 {
 	const char *event_name = "atexit";
-	struct strbuf buf_payload = STRBUF_INIT;
-
-	strbuf_addf(&buf_payload, "code:%d", code);
 
-	perf_io_write_fl(__FILE__, __LINE__, event_name, NULL,
-			 &us_elapsed_absolute, NULL, NULL, &buf_payload);
-	strbuf_release(&buf_payload);
+	perf_io_write_fl_fmt(__FILE__, __LINE__, event_name, NULL,
+			 &us_elapsed_absolute, NULL, NULL, "code:%d", code);
 }
 
 static void maybe_append_string_va(struct strbuf *buf, const char *fmt,
@@ -244,13 +249,9 @@ static void fn_error_va_fl(const char *file, int line, const char *fmt,
 static void fn_command_path_fl(const char *file, int line, const char *pathname)
 {
 	const char *event_name = "cmd_path";
-	struct strbuf buf_payload = STRBUF_INIT;
-
-	strbuf_addstr(&buf_payload, pathname);
 
-	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", pathname);
 }
 
 static void fn_command_ancestry_fl(const char *file, int line, const char **parent_names)
@@ -286,13 +287,9 @@ static void fn_command_name_fl(const char *file, int line, const char *name,
 static void fn_command_mode_fl(const char *file, int line, const char *mode)
 {
 	const char *event_name = "cmd_mode";
-	struct strbuf buf_payload = STRBUF_INIT;
-
-	strbuf_addstr(&buf_payload, mode);
 
-	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", mode);
 }
 
 static void fn_alias_fl(const char *file, int line, const char *alias,
@@ -351,13 +348,10 @@ static void fn_child_exit_fl(const char *file, int line,
 			     int code, uint64_t us_elapsed_child)
 {
 	const char *event_name = "child_exit";
-	struct strbuf buf_payload = STRBUF_INIT;
 
-	strbuf_addf(&buf_payload, "[ch%d] pid:%d code:%d", cid, pid, code);
-
-	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 &us_elapsed_child, NULL, &buf_payload);
-	strbuf_release(&buf_payload);
+	perf_io_write_fl_fmt(file, line, event_name, NULL, &us_elapsed_absolute,
+			     &us_elapsed_child, NULL, "[ch%d] pid:%d code:%d",
+			     cid, pid, code);
 }
 
 static void fn_child_ready_fl(const char *file, int line,
@@ -365,24 +359,19 @@ static void fn_child_ready_fl(const char *file, int line,
 			      const char *ready, uint64_t us_elapsed_child)
 {
 	const char *event_name = "child_ready";
-	struct strbuf buf_payload = STRBUF_INIT;
-
-	strbuf_addf(&buf_payload, "[ch%d] pid:%d ready:%s", cid, pid, ready);
 
-	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 &us_elapsed_child, NULL, &buf_payload);
-	strbuf_release(&buf_payload);
+	perf_io_write_fl_fmt(file, line, event_name, NULL, &us_elapsed_absolute,
+			     &us_elapsed_child, NULL,
+			     "[ch%d] pid:%d ready:%s", cid, pid, ready);
 }
 
 static void fn_thread_start_fl(const char *file, int line,
 			       uint64_t us_elapsed_absolute)
 {
 	const char *event_name = "thread_start";
-	struct strbuf buf_payload = STRBUF_INIT;
 
-	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 NULL, NULL, &buf_payload);
-	strbuf_release(&buf_payload);
+	perf_io_write_fl_fmt(file, line, event_name, NULL, &us_elapsed_absolute,
+			     NULL, NULL, "%s", ""); /* TODO: No payload, support NULL? */
 }
 
 static void fn_thread_exit_fl(const char *file, int line,
@@ -390,11 +379,9 @@ static void fn_thread_exit_fl(const char *file, int line,
 			      uint64_t us_elapsed_thread)
 {
 	const char *event_name = "thread_exit";
-	struct strbuf buf_payload = STRBUF_INIT;
 
-	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 &us_elapsed_thread, NULL, &buf_payload);
-	strbuf_release(&buf_payload);
+	perf_io_write_fl_fmt(file, line, event_name, NULL, &us_elapsed_absolute,
+			     &us_elapsed_thread, NULL, "%s", ""); /* TODO: No payload, support NULL ? */
 }
 
 static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,
@@ -438,13 +425,9 @@ static void fn_param_fl(const char *file, int line, const char *param,
 			const char *value)
 {
 	const char *event_name = "def_param";
-	struct strbuf buf_payload = STRBUF_INIT;
-
-	strbuf_addf(&buf_payload, "%s:%s", param, value);
 
-	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:%s", param, value);
 }
 
 static void fn_repo_fl(const char *file, int line,
@@ -525,13 +508,10 @@ static void fn_data_json_fl(const char *file, int line,
 			    const struct json_writer *value)
 {
 	const char *event_name = "data_json";
-	struct strbuf buf_payload = STRBUF_INIT;
-
-	strbuf_addf(&buf_payload, "%s:%s", key, value->json.buf);
 
-	perf_io_write_fl(file, line, event_name, repo, &us_elapsed_absolute,
-			 &us_elapsed_region, category, &buf_payload);
-	strbuf_release(&buf_payload);
+	perf_io_write_fl_fmt(file, line, event_name, repo, &us_elapsed_absolute,
+			     &us_elapsed_region, category,
+			     "%s:%s", key, value->json.buf);
 }
 
 static void fn_printf_va_fl(const char *file, int line,
Previous: Jeff Hostetler via GitGitGadgetNext: Jeff Hostetler
Message 27 of 55 in “Trace2 stopwatch timers and global counters”
  1. 0/9 Trace2 stopwatch timers and global countersJeff Hostetler via GitGitGadget, Dec 20, 2021
  2. 1/9 trace2: use size_t alloc,nr_open_regions in tr2tls_thread_ctxJeff Hostetler via GitGitGadget, Dec 20, 2021
  3. 2/9 trace2: convert tr2tls_thread_ctx.thread_name from strbuf to char*Jeff Hostetler via GitGitGadget, Dec 20, 2021
  4. Ævar Arnfjörð BjarmasonDec 20, 2021
  5. Jeff HostetlerDec 20, 2021
  6. Ævar Arnfjörð BjarmasonDec 20, 2021
  7. Jeff HostetlerDec 22, 2021
  8. Junio C HamanoDec 21, 2021
  9. Junio C HamanoDec 21, 2021
  10. Jeff HostetlerDec 22, 2021
  11. Junio C HamanoDec 22, 2021
  12. 3/9 trace2: defer free of TLS CTX until program exit.Jeff Hostetler via GitGitGadget, Dec 20, 2021
  13. Junio C HamanoDec 21, 2021
  14. Jeff HostetlerDec 22, 2021
  15. Junio C HamanoDec 22, 2021
  16. Jeff HostetlerDec 22, 2021
  17. Johannes SixtDec 23, 2021
  18. Junio C HamanoDec 23, 2021
  19. Jeff HostetlerDec 27, 2021
  20. 4/9 trace2: add thread-name override to event targetJeff Hostetler via GitGitGadget, Dec 20, 2021
  21. 5/9 trace2: add thread-name override to perf targetJeff Hostetler via GitGitGadget, Dec 20, 2021
  22. 6/9 trace2: add timer events to perf and event target formatsJeff Hostetler via GitGitGadget, Dec 20, 2021
  23. Ævar Arnfjörð BjarmasonDec 20, 2021
  24. Jeff HostetlerDec 20, 2021
  25. Derrick StoleeDec 21, 2021
  26. 8/9 trace2: add counter events to perf and event target formatsJeff Hostetler via GitGitGadget, Dec 20, 2021
  27. Ævar Arnfjörð BjarmasonDec 20, 2021
  28. Jeff HostetlerDec 22, 2021
  29. 7/9 trace2: add stopwatch timersJeff Hostetler via GitGitGadget, Dec 20, 2021
  30. Ævar Arnfjörð BjarmasonDec 20, 2021
  31. Jeff HostetlerDec 22, 2021
  32. Derrick StoleeDec 21, 2021
  33. Jeff HostetlerDec 22, 2021
  34. 9/9 trace2: add global countersJeff Hostetler via GitGitGadget, Dec 20, 2021
  35. Ævar Arnfjörð BjarmasonDec 20, 2021
  36. Jeff HostetlerDec 22, 2021
  37. Derrick StoleeDec 21, 2021
  38. Matheus TavaresDec 21, 2021
  39. 0/9 Trace2 stopwatch timers and global countersJeff Hostetler via GitGitGadget, Dec 28, 2021
  40. 1/9 trace2: use size_t alloc,nr_open_regions in tr2tls_thread_ctxJeff Hostetler via GitGitGadget, Dec 28, 2021
  41. Ævar Arnfjörð BjarmasonDec 29, 2021
  42. 2/9 trace2: convert tr2tls_thread_ctx.thread_name from strbuf to flex arrayJeff Hostetler via GitGitGadget, Dec 28, 2021
  43. Ævar Arnfjörð BjarmasonDec 29, 2021
  44. Jeff HostetlerDec 29, 2021
  45. 3/9 trace2: defer free of thread local storage until program exit.Jeff Hostetler via GitGitGadget, Dec 28, 2021
  46. 4/9 trace2: add thread-name override to event targetJeff Hostetler via GitGitGadget, Dec 28, 2021
  47. 7/9 trace2: add stopwatch timersJeff Hostetler via GitGitGadget, Dec 28, 2021
  48. 5/9 trace2: add thread-name override to perf targetJeff Hostetler via GitGitGadget, Dec 28, 2021
  49. Ævar Arnfjörð BjarmasonDec 29, 2021
  50. Jeff HostetlerDec 29, 2021
  51. 6/9 trace2: add timer events to perf and event target formatsJeff Hostetler via GitGitGadget, Dec 28, 2021
  52. 8/9 trace2: add counter events to perf and event target formatsJeff Hostetler via GitGitGadget, Dec 28, 2021
  53. 9/9 trace2: add global countersJeff Hostetler via GitGitGadget, Dec 28, 2021
  54. Ævar Arnfjörð BjarmasonDec 29, 2021
  55. Jeff HostetlerDec 30, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.