From: Ævar Arnfjörð Bjarmason Date: Fri, 07 Oct 2022 01:10:06 GMT Subject: [RFC PATCH] trace2 API: don't save a copy of constant "thread_name" Message-ID: In-Reply-To: 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 --- On Thu, Oct 06 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason 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