[v2,5/9] trace2: add thread-name override to perf target

Commit Message

Jeff Hostetler Dec. 28, 2021, 7:36 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Teach the message formatter in the Trace2 perf target to accept an
optional thread name argument.  This will override the thread name
inherited from the thread local storage data block.

This will be used in a future commit for global events that should
not be tied to a particular thread, such as a global stopwatch timer.

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 trace2/tr2_tgt_perf.c | 64 +++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 29 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Dec. 29, 2021, 1:48 a.m. UTC | #1

On Tue, Dec 28 2021, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <[e-mail]>
>
> Teach the message formatter in the Trace2 perf target to accept an
> optional thread name argument.  This will override the thread name
> inherited from the thread local storage data block.
>
> This will be used in a future commit for global events that should
> not be tied to a particular thread, such as a global stopwatch timer.

We already have a "ctx", and that "ctx" has a "thread_name", but here
and in the preceding commit we're adding a "thread_name" to every caller
of these functions in case we'd like to override it.

Wouldn't it make more sense to just pass a "ctx" to these functions? One
of them already takes it, here's an (obviously incomplete) fixup on top
of your series to make the one that doesn't take a "ctx", and for the
only non-NULL users of "thread_name" to just use a trivial helper to
pass in a "ctx" with a new "thread_name", then to swap it back.

It would make for a smaller diffstat for this already large series, or
we could do exactly what we're doing now, but avoid the churn of
adjusting every caller by introducing a new sister function for those
who want this parameter to be non-NULL.

(The below patch is "broken" in that __FILE__ and __LINE__ need to be
passed in as parameters, but this is just a trivial change for
show/commentary)

diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index b9eb2cdb77a..7aaec83dff7 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -82,16 +82,15 @@ static void fn_term(void)
 static void event_fmt_prepare(const char *event_name, const char *file,
 			      int line, const struct repository *repo,
 			      struct json_writer *jw,
-			      const char *thread_name_override)
+			      struct tr2tls_thread_ctx *ctx)
 {
 	struct tr2_tbuf tb_now;
+	if (!ctx)
+		ctx = tr2tls_get_self();
 
 	jw_object_string(jw, "event", event_name);
 	jw_object_string(jw, "sid", tr2_sid_get());
-	jw_object_string(jw, "thread",
-			 ((thread_name_override && *thread_name_override)
-			  ? thread_name_override
-			  : tr2tls_get_self()->thread_name));
+	jw_object_string(jw, "thread", ctx->thread_name);
 
 	/*
 	 * In brief mode, only emit <time> on these 2 event types.
@@ -111,6 +110,20 @@ static void event_fmt_prepare(const char *event_name, const char *file,
 		jw_object_intmax(jw, "repo", repo->trace2_repo_id);
 }
 
+static void event_fmt_prepare_tn(const char *event_name, const char *file,
+				 int line, const struct repository *repo,
+				 struct json_writer *jw,
+				 const char *thread_name)
+{
+	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
+	const char *tmp;
+
+	tmp = ctx->thread_name;
+	ctx->thread_name = thread_name;
+	event_fmt_prepare(event_name, __FILE__, __LINE__, NULL, jw, ctx);
+	ctx->thread_name = tmp;
+}
+
 static void fn_too_many_files_fl(const char *file, int line)
 {
 	const char *event_name = "too_many_files";
@@ -629,7 +642,7 @@ static void fn_timer(uint64_t us_elapsed_absolute,
 	double t_abs = (double)us_elapsed_absolute / 1000000.0;
 
 	jw_object_begin(&jw, 0);
-	event_fmt_prepare(event_name, __FILE__, __LINE__, NULL, &jw, thread_name);
+	event_fmt_prepare_tn(event_name, __FILE__, __LINE__, NULL, &jw, thread_name);
 	jw_object_double(&jw, "t_abs", 6, t_abs);
 	jw_object_string(&jw, "name", timer_name);
 	jw_object_intmax(&jw, "count", interval_count);
@@ -654,7 +667,7 @@ static void fn_counter(uint64_t us_elapsed_absolute,
 	double t_abs = (double)us_elapsed_absolute / 1000000.0;
 
 	jw_object_begin(&jw, 0);
-	event_fmt_prepare(event_name, __FILE__, __LINE__, NULL, &jw, thread_name);
+	event_fmt_prepare_tn(event_name, __FILE__, __LINE__, NULL, &jw, thread_name);
 	jw_object_double(&jw, "t_abs", 6, t_abs);
 	jw_object_string(&jw, "name", counter_name);
 	jw_object_intmax(&jw, "value", value);

Jeff Hostetler Dec. 29, 2021, 5:15 p.m. UTC | #2

On 12/28/21 8:48 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Dec 28 2021, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <[e-mail]>
>>
>> Teach the message formatter in the Trace2 perf target to accept an
>> optional thread name argument.  This will override the thread name
>> inherited from the thread local storage data block.
>>
>> This will be used in a future commit for global events that should
>> not be tied to a particular thread, such as a global stopwatch timer.
> 
> We already have a "ctx", and that "ctx" has a "thread_name", but here
> and in the preceding commit we're adding a "thread_name" to every caller
> of these functions in case we'd like to override it.
> 
> Wouldn't it make more sense to just pass a "ctx" to these functions? One
> of them already takes it, here's an (obviously incomplete) fixup on top
> of your series to make the one that doesn't take a "ctx", and for the
> only non-NULL users of "thread_name" to just use a trivial helper to
> pass in a "ctx" with a new "thread_name", then to swap it back.
> 
> It would make for a smaller diffstat for this already large series, or
> we could do exactly what we're doing now, but avoid the churn of
> adjusting every caller by introducing a new sister function for those
> who want this parameter to be non-NULL.

I suppose it is possible to have a helper version of
`event_fmt_prepare()` that takes the extra argument and
fixup the existing function to call it with NULL.

I'll see if that makes sense.


[...]
>   
> +static void event_fmt_prepare_tn(const char *event_name, const char *file,
> +				 int line, const struct repository *repo,
> +				 struct json_writer *jw,
> +				 const char *thread_name)
> +{
> +	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
> +	const char *tmp;
> +
> +	tmp = ctx->thread_name;
> +	ctx->thread_name = thread_name;
> +	event_fmt_prepare(event_name, __FILE__, __LINE__, NULL, jw, ctx);
> +	ctx->thread_name = tmp;
> +}
[...]

This only works if we agree that thread name is a pointer inside
the structure and not a flex-array.

Personally, I think this is trying to do things backwards by
temporarily changing the ctx->thread_name field.  I think it
would be better to `event_fmt_prepare_tn()` do the actual
work with the supplied thread name and have the existing
`event_fmt_prepare()` just call it with ctx->thread_name.
Then we don't need to hack up the ctx.

I'll see if this makes the diffs a little cleaner.

Jeff

Patch

diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index fd6cce3efe5..c008fd08ae8 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -65,9 +65,14 @@  static void perf_fmt_prepare(const char *event_name,
 			     int line, const struct repository *repo,
 			     uint64_t *p_us_elapsed_absolute,
 			     uint64_t *p_us_elapsed_relative,
-			     const char *category, struct strbuf *buf)
+			     const char *category, struct strbuf *buf,
+			     const char *thread_name_override)
 {
 	int len;
+	const char *thread_name =
+		((thread_name_override && *thread_name_override)
+		 ? thread_name_override
+		 : ctx->thread_name);
 
 	strbuf_setlen(buf, 0);
 
@@ -107,7 +112,7 @@  static void perf_fmt_prepare(const char *event_name,
 
 	strbuf_addf(buf, "d%d | ", tr2_sid_depth());
 	strbuf_addf(buf, "%-*.*s | %-*s | ", TR2FMT_PERF_MAX_THREAD_NAME,
-		    TR2FMT_PERF_MAX_THREAD_NAME, ctx->thread_name,
+		    TR2FMT_PERF_MAX_THREAD_NAME, thread_name,
 		    TR2FMT_PERF_MAX_EVENT_NAME, event_name);
 
 	len = buf->len + TR2FMT_PERF_REPO_WIDTH;
@@ -141,14 +146,15 @@  static void perf_io_write_fl(const char *file, int line, const char *event_name,
 			     uint64_t *p_us_elapsed_absolute,
 			     uint64_t *p_us_elapsed_relative,
 			     const char *category,
-			     const struct strbuf *buf_payload)
+			     const struct strbuf *buf_payload,
+			     const char *thread_name_override)
 {
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
 	struct strbuf buf_line = STRBUF_INIT;
 
 	perf_fmt_prepare(event_name, ctx, file, line, repo,
 			 p_us_elapsed_absolute, p_us_elapsed_relative, category,
-			 &buf_line);
+			 &buf_line, thread_name_override);
 	strbuf_addbuf(&buf_line, buf_payload);
 	tr2_dst_write_line(&tr2dst_perf, &buf_line);
 	strbuf_release(&buf_line);
@@ -162,7 +168,7 @@  static void fn_version_fl(const char *file, int line)
 	strbuf_addstr(&buf_payload, git_version_string);
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
-			 &buf_payload);
+			 &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -175,7 +181,7 @@  static void fn_start_fl(const char *file, int line,
 	sq_append_quote_argv_pretty(&buf_payload, argv);
 
 	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 NULL, NULL, &buf_payload);
+			 NULL, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -188,7 +194,7 @@  static void fn_exit_fl(const char *file, int line, uint64_t us_elapsed_absolute,
 	strbuf_addf(&buf_payload, "code:%d", code);
 
 	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 NULL, NULL, &buf_payload);
+			 NULL, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -200,7 +206,7 @@  static void fn_signal(uint64_t us_elapsed_absolute, int signo)
 	strbuf_addf(&buf_payload, "signo:%d", signo);
 
 	perf_io_write_fl(__FILE__, __LINE__, event_name, NULL,
-			 &us_elapsed_absolute, NULL, NULL, &buf_payload);
+			 &us_elapsed_absolute, NULL, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -212,7 +218,7 @@  static void fn_atexit(uint64_t us_elapsed_absolute, int code)
 	strbuf_addf(&buf_payload, "code:%d", code);
 
 	perf_io_write_fl(__FILE__, __LINE__, event_name, NULL,
-			 &us_elapsed_absolute, NULL, NULL, &buf_payload);
+			 &us_elapsed_absolute, NULL, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -238,7 +244,7 @@  static void fn_error_va_fl(const char *file, int line, const char *fmt,
 	maybe_append_string_va(&buf_payload, fmt, ap);
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
-			 &buf_payload);
+			 &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -250,7 +256,7 @@  static void fn_command_path_fl(const char *file, int line, const char *pathname)
 	strbuf_addstr(&buf_payload, pathname);
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
-			 &buf_payload);
+			 &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -265,7 +271,7 @@  static void fn_command_ancestry_fl(const char *file, int line, const char **pare
 	strbuf_addch(&buf_payload, ']');
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
-			 &buf_payload);
+			 &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -280,7 +286,7 @@  static void fn_command_name_fl(const char *file, int line, const char *name,
 		strbuf_addf(&buf_payload, " (%s)", hierarchy);
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
-			 &buf_payload);
+			 &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -292,7 +298,7 @@  static void fn_command_mode_fl(const char *file, int line, const char *mode)
 	strbuf_addstr(&buf_payload, mode);
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
-			 &buf_payload);
+			 &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -307,7 +313,7 @@  static void fn_alias_fl(const char *file, int line, const char *alias,
 	strbuf_addch(&buf_payload, ']');
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
-			 &buf_payload);
+			 &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -343,7 +349,7 @@  static void fn_child_start_fl(const char *file, int line,
 	strbuf_addch(&buf_payload, ']');
 
 	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 NULL, NULL, &buf_payload);
+			 NULL, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -357,7 +363,7 @@  static void fn_child_exit_fl(const char *file, int line,
 	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);
+			 &us_elapsed_child, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -371,7 +377,7 @@  static void fn_child_ready_fl(const char *file, int line,
 	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);
+			 &us_elapsed_child, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -382,7 +388,7 @@  static void fn_thread_start_fl(const char *file, int line,
 	struct strbuf buf_payload = STRBUF_INIT;
 
 	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 NULL, NULL, &buf_payload);
+			 NULL, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -394,7 +400,7 @@  static void fn_thread_exit_fl(const char *file, int line,
 	struct strbuf buf_payload = STRBUF_INIT;
 
 	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 &us_elapsed_thread, NULL, &buf_payload);
+			 &us_elapsed_thread, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -415,7 +421,7 @@  static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,
 	strbuf_addch(&buf_payload, ']');
 
 	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 NULL, NULL, &buf_payload);
+			 NULL, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -431,7 +437,7 @@  static void fn_exec_result_fl(const char *file, int line,
 		strbuf_addf(&buf_payload, " err:%s", strerror(code));
 
 	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 NULL, NULL, &buf_payload);
+			 NULL, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -444,7 +450,7 @@  static void fn_param_fl(const char *file, int line, const char *param,
 	strbuf_addf(&buf_payload, "%s:%s", param, value);
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
-			 &buf_payload);
+			 &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -458,7 +464,7 @@  static void fn_repo_fl(const char *file, int line,
 	sq_quote_buf_pretty(&buf_payload, repo->worktree);
 
 	perf_io_write_fl(file, line, event_name, repo, NULL, NULL, NULL,
-			 &buf_payload);
+			 &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -480,7 +486,7 @@  static void fn_region_enter_printf_va_fl(const char *file, int line,
 	}
 
 	perf_io_write_fl(file, line, event_name, repo, &us_elapsed_absolute,
-			 NULL, category, &buf_payload);
+			 NULL, category, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -500,7 +506,7 @@  static void fn_region_leave_printf_va_fl(
 	}
 
 	perf_io_write_fl(file, line, event_name, repo, &us_elapsed_absolute,
-			 &us_elapsed_region, category, &buf_payload);
+			 &us_elapsed_region, category, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -515,7 +521,7 @@  static void fn_data_fl(const char *file, int line, uint64_t us_elapsed_absolute,
 	strbuf_addf(&buf_payload, "%s:%s", key, value);
 
 	perf_io_write_fl(file, line, event_name, repo, &us_elapsed_absolute,
-			 &us_elapsed_region, category, &buf_payload);
+			 &us_elapsed_region, category, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -531,7 +537,7 @@  static void fn_data_json_fl(const char *file, int line,
 	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);
+			 &us_elapsed_region, category, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }
 
@@ -545,7 +551,7 @@  static void fn_printf_va_fl(const char *file, int line,
 	maybe_append_string_va(&buf_payload, fmt, ap);
 
 	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
-			 NULL, NULL, &buf_payload);
+			 NULL, NULL, &buf_payload, NULL);
 	strbuf_release(&buf_payload);
 }