[RFC PATCH] trace2 API: don't save a copy of constant "thread_name"

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

Since ee4512ed481 (trace2: create new combined trace facility,
2019-02-22) the "thread_name" member of "struct tr2tls_thread_ctx" has
been copied from the caller, but those callers have always passed a
constant string:

	$ git -P grep '^\s*trace2_thread_start\('
	Documentation/technical/api-trace2.txt: trace2_thread_start("preload_thread");
	builtin/fsmonitor--daemon.c:    trace2_thread_start("fsm-health");
	builtin/fsmonitor--daemon.c:    trace2_thread_start("fsm-listen");
	compat/simple-ipc/ipc-unix-socket.c:    trace2_thread_start("ipc-worker");
	compat/simple-ipc/ipc-unix-socket.c:    trace2_thread_start("ipc-accept");
	compat/simple-ipc/ipc-win32.c:  trace2_thread_start("ipc-server");
	t/helper/test-fsmonitor-client.c:       trace2_thread_start("hammer");
	t/helper/test-simple-ipc.c:     trace2_thread_start("multiple");

This isn't needed for optimization, but apparently[1] there's been
some confusion about the non-const-ness of the previous "struct
strbuf".

Using the caller's string here makes this more straightforward, as
it's now clear that we're not dynamically constructing these. It's
also what the progress API does with its "title" string.

Since we know we're hardcoding these thread names let's BUG() out when
we see that the length of the name plus the length of the prefix would
exceed the maximum length for the "perf" format.

1. https://lore.kernel.org/git/82f1672e180afcd876505a4354bd9952f70db49e.1664900407.git.gitgitgadget@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Thu, Oct 06 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> So, we don't need to strdup() and store that "preload_thread" anywhere.
>> It's already a constant string we have hardcoded in the program. We just
>> need to save a pointer to it.
>
> That sounds even simpler.

A cleaned up version of the test code I had on top of "master", RFC
because I may still be missing some context here. E.g. maybe there's a
plan to dynamically construct these thread names?

 json-writer.c          | 17 +++++++++++++++++
 json-writer.h          |  4 ++++
 trace2/tr2_tgt_event.c |  2 +-
 trace2/tr2_tgt_perf.c  | 10 +++++++---
 trace2/tr2_tls.c       | 14 +++++---------
 trace2/tr2_tls.h       |  9 +++++++--
 6 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/json-writer.c b/json-writer.c
index f1cfd8fa8c6..569a75bee51 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -161,6 +161,23 @@ void jw_object_string(struct json_writer *jw, const char *key, const char *value
 	append_quoted_string(&jw->json, value);
 }
 
+void jw_strbuf_add_thread_name(struct strbuf *sb, const char *thread_name,
+			       int thread_id)
+{
+	if (thread_id)
+		strbuf_addf(sb, "th%02d:", thread_id);
+	strbuf_addstr(sb, thread_name);
+}
+
+void jw_object_string_thread(struct json_writer *jw, const char *thread_name,
+			     int thread_id)
+{
+	object_common(jw, "thread");
+	strbuf_addch(&jw->json, '"');
+	jw_strbuf_add_thread_name(&jw->json, thread_name, thread_id);
+	strbuf_addch(&jw->json, '"');
+}
+
 void jw_object_intmax(struct json_writer *jw, const char *key, intmax_t value)
 {
 	object_common(jw, key);
diff --git a/json-writer.h b/json-writer.h
index 209355e0f12..269c203b119 100644
--- a/json-writer.h
+++ b/json-writer.h
@@ -75,6 +75,10 @@ void jw_release(struct json_writer *jw);
 void jw_object_begin(struct json_writer *jw, int pretty);
 void jw_array_begin(struct json_writer *jw, int pretty);
 
+void jw_strbuf_add_thread_name(struct strbuf *buf, const char *thread_name,
+			       int thread_id);
+void jw_object_string_thread(struct json_writer *jw, const char *thread_name,
+			     int thread_id);
 void jw_object_string(struct json_writer *jw, const char *key,
 		      const char *value);
 void jw_object_intmax(struct json_writer *jw, const char *key, intmax_t value);
diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index 37a3163be12..1308cf05df4 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -90,7 +90,7 @@ static void event_fmt_prepare(const char *event_name, const char *file,
 
 	jw_object_string(jw, "event", event_name);
 	jw_object_string(jw, "sid", tr2_sid_get());
-	jw_object_string(jw, "thread", ctx->thread_name.buf);
+	jw_object_string_thread(jw, ctx->thread_name, ctx->thread_id);
 
 	/*
 	 * In brief mode, only emit <time> on these 2 event types.
diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index 8cb792488c8..ab21277eb36 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -69,6 +69,8 @@ static void perf_fmt_prepare(const char *event_name,
 			     const char *category, struct strbuf *buf)
 {
 	int len;
+	size_t oldlen;
+	int padlen;
 
 	strbuf_setlen(buf, 0);
 
@@ -107,9 +109,11 @@ static void perf_fmt_prepare(const char *event_name,
 	}
 
 	strbuf_addf(buf, "d%d | ", tr2_sid_depth());
-	strbuf_addf(buf, "%-*s | %-*s | ", TR2_MAX_THREAD_NAME,
-		    ctx->thread_name.buf, TR2FMT_PERF_MAX_EVENT_NAME,
-		    event_name);
+	oldlen = buf->len;
+	jw_strbuf_add_thread_name(buf, ctx->thread_name, ctx->thread_id);
+	padlen = TR2_MAX_THREAD_NAME - (buf->len - oldlen);;
+	strbuf_addf(buf, "%-*s | %-*s | ", padlen, "",
+		    TR2FMT_PERF_MAX_EVENT_NAME, event_name);
 
 	len = buf->len + TR2FMT_PERF_REPO_WIDTH;
 	if (repo)
diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
index 7da94aba522..aa9aeb67fca 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -36,6 +36,9 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
 {
 	struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(*ctx));
 
+	if (strlen(thread_name) + TR2_MAX_THREAD_NAME_PREFIX > TR2_MAX_THREAD_NAME)
+		BUG("too long thread name '%s'", thread_name);
+
 	/*
 	 * Implicitly "tr2tls_push_self()" to capture the thread's start
 	 * time in array_us_start[0].  For the main thread this gives us the
@@ -45,15 +48,9 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
 	ctx->array_us_start = (uint64_t *)xcalloc(ctx->alloc, sizeof(uint64_t));
 	ctx->array_us_start[ctx->nr_open_regions++] = us_thread_start;
 
+	ctx->thread_name = thread_name;
 	ctx->thread_id = tr2tls_locked_increment(&tr2_next_thread_id);
 
-	strbuf_init(&ctx->thread_name, 0);
-	if (ctx->thread_id)
-		strbuf_addf(&ctx->thread_name, "th%02d:", ctx->thread_id);
-	strbuf_addstr(&ctx->thread_name, thread_name);
-	if (ctx->thread_name.len > TR2_MAX_THREAD_NAME)
-		strbuf_setlen(&ctx->thread_name, TR2_MAX_THREAD_NAME);
-
 	pthread_setspecific(tr2tls_key, ctx);
 
 	return ctx;
@@ -95,7 +92,6 @@ void tr2tls_unset_self(void)
 
 	pthread_setspecific(tr2tls_key, NULL);
 
-	strbuf_release(&ctx->thread_name);
 	free(ctx->array_us_start);
 	free(ctx);
 }
@@ -113,7 +109,7 @@ void tr2tls_pop_self(void)
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
 
 	if (!ctx->nr_open_regions)
-		BUG("no open regions in thread '%s'", ctx->thread_name.buf);
+		BUG("no open regions in thread '%s'", ctx->thread_name);
 
 	ctx->nr_open_regions--;
 }
diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h
index b1e327a928e..f600eb22551 100644
--- a/trace2/tr2_tls.h
+++ b/trace2/tr2_tls.h
@@ -4,12 +4,17 @@
 #include "strbuf.h"
 
 /*
- * Arbitry limit for thread names for column alignment.
+ * Arbitry limit for thread names for column alignment. The overall
+ * max length is TR2_MAX_THREAD_NAME, and the
+ * TR2_MAX_THREAD_NAME_PREFIX is the length of the formatted
+ * '"th%02d:", ctx->thread_id' prefix which is added when "thread_id >
+ * 0".
  */
+#define TR2_MAX_THREAD_NAME_PREFIX (5)
 #define TR2_MAX_THREAD_NAME (24)
 
 struct tr2tls_thread_ctx {
-	struct strbuf thread_name;
+	const char *thread_name;
 	uint64_t *array_us_start;
 	int alloc;
 	int nr_open_regions; /* plays role of "nr" in ALLOC_GROW */
-- 
2.38.0.971.ge79ff6d20e7