On Mon, Dec 20 2021, Jeff Hostetler via GitGitGadget wrote: > From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx> > > Use a 'char *' to hold the thread name rather than a 'struct strbuf'. > The thread name is set when the thread is created and should not be > be modified afterwards. Replace the strbuf with an allocated pointer > to make that more clear. > > This was discussed in: https://lore.kernel.org/all/xmqqa6kdwo24.fsf@gitster.g/ > > Signed-off-by: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx> > --- > trace2/tr2_tgt_event.c | 2 +- > trace2/tr2_tgt_perf.c | 2 +- > trace2/tr2_tls.c | 16 +++++++++------- > trace2/tr2_tls.h | 2 +- > 4 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c > index 3a0014417cc..ca48d00aebc 100644 > --- a/trace2/tr2_tgt_event.c > +++ b/trace2/tr2_tgt_event.c > @@ -88,7 +88,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(jw, "thread", ctx->thread_name); > > /* > * 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 e4acca13d64..c3e57fcb3c0 100644 > --- a/trace2/tr2_tgt_perf.c > +++ b/trace2/tr2_tgt_perf.c > @@ -106,7 +106,7 @@ 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, > + ctx->thread_name, TR2FMT_PERF_MAX_EVENT_NAME, > event_name); > > len = buf->len + TR2FMT_PERF_REPO_WIDTH; > diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c > index 7da94aba522..cd8b9f2f0a0 100644 > --- a/trace2/tr2_tls.c > +++ b/trace2/tr2_tls.c > @@ -35,6 +35,7 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name, > uint64_t us_thread_start) > { > struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(*ctx)); > + struct strbuf buf_name = STRBUF_INIT; > > /* > * Implicitly "tr2tls_push_self()" to capture the thread's start > @@ -47,12 +48,13 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *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); > + strbuf_addf(&buf_name, "th%02d:", ctx->thread_id); > + strbuf_addstr(&buf_name, thread_name); > + if (buf_name.len > TR2_MAX_THREAD_NAME) > + strbuf_setlen(&buf_name, TR2_MAX_THREAD_NAME); > + > + ctx->thread_name = strbuf_detach(&buf_name, NULL); > > pthread_setspecific(tr2tls_key, ctx); > > @@ -95,7 +97,7 @@ void tr2tls_unset_self(void) > > pthread_setspecific(tr2tls_key, NULL); > > - strbuf_release(&ctx->thread_name); > + free(ctx->thread_name); > free(ctx->array_us_start); > free(ctx); > } > @@ -113,7 +115,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 a90bd639d48..d968da6a679 100644 > --- a/trace2/tr2_tls.h > +++ b/trace2/tr2_tls.h > @@ -9,7 +9,7 @@ > #define TR2_MAX_THREAD_NAME (24) > > struct tr2tls_thread_ctx { > - struct strbuf thread_name; > + char *thread_name; > uint64_t *array_us_start; > size_t alloc; > size_t nr_open_regions; /* plays role of "nr" in ALLOC_GROW */ Junio's suggestion in the linked E-Mail was to make this a "const char *". Narrowly, I don't see why not just add a "const" to the "struct strbuf *" instead. But less narrowly if we're not going to change it why malloc a new one at all? Can't we just use the "const char *" passed into tr2tls_create_self(), and for the "th%02d:" case have the code that's formatting it handle that case? I.e. have the things that use it as a "%s" now call a function that formats things as a function of the "ctx->thread_id" (which may be 0) and limit it by TR2_MAX_THREAD_NAME?
