diff mbox series

[v2,2/9] trace2: convert tr2tls_thread_ctx.thread_name from strbuf to flex array

Message ID ff8df1b148ec486f565a59cb8a227bfa2ef48313.1640720202.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Trace2 stopwatch timers and global counters | expand

Commit Message

Jeff Hostetler Dec. 28, 2021, 7:36 p.m. UTC 
From: Jeff Hostetler <jeffhost@microsoft.com>

Move the thread name to a flex array at the bottom of the Trace2
thread local storage data and get rid of the strbuf.

Let the flex array have the full computed value of the thread name
without truncation.

Change the PERF target to truncate the thread name so that the columns
still line up.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tgt_event.c |  2 +-
 trace2/tr2_tgt_perf.c  |  7 ++++---
 trace2/tr2_tls.c       | 25 +++++++++++++------------
 trace2/tr2_tls.h       | 10 +---------
 4 files changed, 19 insertions(+), 25 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Dec. 29, 2021, 1:11 a.m. UTC | #1 
On Tue, Dec 28 2021, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Move the thread name to a flex array at the bottom of the Trace2
> thread local storage data and get rid of the strbuf.
>
> Let the flex array have the full computed value of the thread name
> without truncation.
>
> Change the PERF target to truncate the thread name so that the columns
> still line up.

This commit message really doesn't help in explaining what we're trying
to do here and why it's needed. I'm not saying it's not, but why not a
strbuf, why a flex array? The diff below also shows changes unrelated to
this.

I tried this local fixup on top of this series which works, so I wonder
if we're just trying to get rid of the strbuf to signal that this
shouldn't change why not just strbuf_detach() and keep a "const char
*thread_name"?

diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
index 28ea55863d1..35d49b27b2e 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -48,7 +48,7 @@ void tr2tls_start_process_clock(void)
 struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
 					     uint64_t us_thread_start)
 {
-	struct tr2tls_thread_ctx *ctx;
+	struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(struct tr2tls_thread_ctx));
 	struct strbuf buf_name = STRBUF_INIT;
 	int thread_id = tr2tls_locked_increment(&tr2_next_thread_id);
 
@@ -56,8 +56,7 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
 		strbuf_addf(&buf_name, "th%02d:", thread_id);
 	strbuf_addstr(&buf_name, thread_name);
 
-	FLEX_ALLOC_MEM(ctx, thread_name, buf_name.buf, buf_name.len);
-	strbuf_release(&buf_name);
+	ctx->thread_name = strbuf_detach(&buf_name, NULL);
 
 	ctx->thread_id = thread_id;
 
@@ -188,6 +187,7 @@ void tr2tls_release(void)
 	while (ctx) {
 		struct tr2tls_thread_ctx *next = ctx->next_ctx;
 
+		free((char *)ctx->thread_name);
 		free(ctx->array_us_start);
 		free(ctx);
 
diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h
index 503829bbd44..bc6c6f12e38 100644
--- a/trace2/tr2_tls.h
+++ b/trace2/tr2_tls.h
@@ -6,6 +6,7 @@
 #include "trace2/tr2_tmr.h"
 
 struct tr2tls_thread_ctx {
+	const char *thread_name;
 	struct tr2tls_thread_ctx *next_ctx;
 	uint64_t *array_us_start;
 	size_t alloc;
@@ -14,8 +15,6 @@ struct tr2tls_thread_ctx {
 
 	struct tr2_timer_block timers;
 	struct tr2_counter_block counters;
-
-	char thread_name[FLEX_ARRAY];
 };
 
 /*

> [...]
> index 7da94aba522..ed99a234b95 100644
> --- a/trace2/tr2_tls.c
> +++ b/trace2/tr2_tls.c
> @@ -34,7 +34,18 @@ void tr2tls_start_process_clock(void)
>  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 tr2tls_thread_ctx *ctx;
> +	struct strbuf buf_name = STRBUF_INIT;
> +	int thread_id = tr2tls_locked_increment(&tr2_next_thread_id);

Here's the looks-to-be-unrelated to this strbuf conversion code I
mentioned above.

> +
> +	if (thread_id)
> +		strbuf_addf(&buf_name, "th%02d:", thread_id);
> +	strbuf_addstr(&buf_name, thread_name);
> +
> +	FLEX_ALLOC_MEM(ctx, thread_name, buf_name.buf, buf_name.len);
> +	strbuf_release(&buf_name);
> +
> +	ctx->thread_id = thread_id;
>  
>  	/*
> [...]
Jeff Hostetler Dec. 29, 2021, 4:46 p.m. UTC | #2 
On 12/28/21 8:11 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Dec 28 2021, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Move the thread name to a flex array at the bottom of the Trace2
>> thread local storage data and get rid of the strbuf.
>>
>> Let the flex array have the full computed value of the thread name
>> without truncation.
>>
>> Change the PERF target to truncate the thread name so that the columns
>> still line up.
> 
> This commit message really doesn't help in explaining what we're trying
> to do here and why it's needed. I'm not saying it's not, but why not a
> strbuf, why a flex array? The diff below also shows changes unrelated to
> this.
> 
> I tried this local fixup on top of this series which works, so I wonder
> if we're just trying to get rid of the strbuf to signal that this
> shouldn't change why not just strbuf_detach() and keep a "const char
> *thread_name"?
> 
> diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
> index 28ea55863d1..35d49b27b2e 100644
> --- a/trace2/tr2_tls.c
> +++ b/trace2/tr2_tls.c
> @@ -48,7 +48,7 @@ void tr2tls_start_process_clock(void)
>   struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
>   					     uint64_t us_thread_start)
>   {
> -	struct tr2tls_thread_ctx *ctx;
> +	struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(struct tr2tls_thread_ctx));
>   	struct strbuf buf_name = STRBUF_INIT;
>   	int thread_id = tr2tls_locked_increment(&tr2_next_thread_id);
>   
> @@ -56,8 +56,7 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
>   		strbuf_addf(&buf_name, "th%02d:", thread_id);
>   	strbuf_addstr(&buf_name, thread_name);
>   
> -	FLEX_ALLOC_MEM(ctx, thread_name, buf_name.buf, buf_name.len);
> -	strbuf_release(&buf_name);
> +	ctx->thread_name = strbuf_detach(&buf_name, NULL);
>   
>   	ctx->thread_id = thread_id;
>   
> @@ -188,6 +187,7 @@ void tr2tls_release(void)
>   	while (ctx) {
>   		struct tr2tls_thread_ctx *next = ctx->next_ctx;
>   
> +		free((char *)ctx->thread_name);
>   		free(ctx->array_us_start);
>   		free(ctx);
>   
> diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h
> index 503829bbd44..bc6c6f12e38 100644
> --- a/trace2/tr2_tls.h
> +++ b/trace2/tr2_tls.h
> @@ -6,6 +6,7 @@
>   #include "trace2/tr2_tmr.h"
>   
>   struct tr2tls_thread_ctx {
> +	const char *thread_name;
>   	struct tr2tls_thread_ctx *next_ctx;
>   	uint64_t *array_us_start;
>   	size_t alloc;
> @@ -14,8 +15,6 @@ struct tr2tls_thread_ctx {
>   
>   	struct tr2_timer_block timers;
>   	struct tr2_counter_block counters;
> -
> -	char thread_name[FLEX_ARRAY];
>   };
>   
>   /*

I have to admit that I really don't know how to please you.

In V1 I converted the "strbuf" to a "char *" inside the structure
because there was concern that one might assume that the thread
name could be changed after the thread was created.  You complained
that I made it a "char *" rather than a "const char *".  I explained
pointer ownership and you completely ignored that.  You explained
that I should just "cast away the const during the free" because
other places in the code use that "anti-pattern".  You also complained
that I didn't use a callback to get the thread name dynamically rather
than having a string field in the thread's TLS.  I explained that it
was faster to compute it once than to generate it on every logging
call.  You ignored that and hinted that the message formatting in
each of the target destinations would make that cost irrelevant.
I convert the field to a flex-array to avoid all of the allocation and
ownership issues and now you send me a "fixup" patch that undoes
the flex-array change and makes it look mostly like my previous
version -- but WITH the "const" and the "cast" (that I've already
talked about in this paragraph).

So, where does this leave us?  I'm really trying to "assume good
intentions" here, but we've spent way toooooooo long discussing
this thread_name field.  It's starting to feel like you're going
to just keep nagging me about this field until I make it look
exactly like you would have written it.

So, sorry to rant, but I don't know what else to say about this
field.  It is especially troubling that this "issue" has taken
so much time -- time that would be better spent actually looking
at the new timers and counters feature.


> 
>> [...]
>> index 7da94aba522..ed99a234b95 100644
>> --- a/trace2/tr2_tls.c
>> +++ b/trace2/tr2_tls.c
>> @@ -34,7 +34,18 @@ void tr2tls_start_process_clock(void)
>>   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 tr2tls_thread_ctx *ctx;
>> +	struct strbuf buf_name = STRBUF_INIT;
>> +	int thread_id = tr2tls_locked_increment(&tr2_next_thread_id);
> 
> Here's the looks-to-be-unrelated to this strbuf conversion code I
> mentioned above.

In the flex-array version, we defer the alloc of "ctx" until
after we have computed the thread name -- we to do that so that
we know the length of the thread name (and thus the size of the
flex-array).  To do that we need to know the thread id that we
will be formatting into the thread name.  And to do that we need
to reserve a thread id -- which is a global and requires a lock.

So the call to tr2tls_locked_increment() (as well as the formatting
of the name itself) was moved up to the top of the function rather
than after the "ctx" was allocated.

> 
>> +
>> +	if (thread_id)
>> +		strbuf_addf(&buf_name, "th%02d:", thread_id);
>> +	strbuf_addstr(&buf_name, thread_name);
>> +
>> +	FLEX_ALLOC_MEM(ctx, thread_name, buf_name.buf, buf_name.len);
>> +	strbuf_release(&buf_name);
>> +
>> +	ctx->thread_id = thread_id;
>>   
>>   	/*
>> [...]

Jeff
diff mbox series

Patch

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..fd6cce3efe5 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -23,6 +23,7 @@  static int tr2env_perf_be_brief;
 
 #define TR2FMT_PERF_FL_WIDTH (28)
 #define TR2FMT_PERF_MAX_EVENT_NAME (12)
+#define TR2FMT_PERF_MAX_THREAD_NAME (24)
 #define TR2FMT_PERF_REPO_WIDTH (3)
 #define TR2FMT_PERF_CATEGORY_WIDTH (12)
 
@@ -105,9 +106,9 @@  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);
+	strbuf_addf(buf, "%-*.*s | %-*s | ", TR2FMT_PERF_MAX_THREAD_NAME,
+		    TR2FMT_PERF_MAX_THREAD_NAME, ctx->thread_name,
+		    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..ed99a234b95 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -34,7 +34,18 @@  void tr2tls_start_process_clock(void)
 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 tr2tls_thread_ctx *ctx;
+	struct strbuf buf_name = STRBUF_INIT;
+	int thread_id = tr2tls_locked_increment(&tr2_next_thread_id);
+
+	if (thread_id)
+		strbuf_addf(&buf_name, "th%02d:", thread_id);
+	strbuf_addstr(&buf_name, thread_name);
+
+	FLEX_ALLOC_MEM(ctx, thread_name, buf_name.buf, buf_name.len);
+	strbuf_release(&buf_name);
+
+	ctx->thread_id = thread_id;
 
 	/*
 	 * Implicitly "tr2tls_push_self()" to capture the thread's start
@@ -45,15 +56,6 @@  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_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 +97,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 +114,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..64d97c5ac03 100644
--- a/trace2/tr2_tls.h
+++ b/trace2/tr2_tls.h
@@ -3,17 +3,12 @@ 
 
 #include "strbuf.h"
 
-/*
- * Arbitry limit for thread names for column alignment.
- */
-#define TR2_MAX_THREAD_NAME (24)
-
 struct tr2tls_thread_ctx {
-	struct strbuf thread_name;
 	uint64_t *array_us_start;
 	size_t alloc;
 	size_t nr_open_regions; /* plays role of "nr" in ALLOC_GROW */
 	int thread_id;
+	char thread_name[FLEX_ARRAY];
 };
 
 /*
@@ -25,9 +20,6 @@  struct tr2tls_thread_ctx {
  * non-zero thread-ids to help distinguish messages from concurrent
  * threads.
  *
- * Truncate the thread name if necessary to help with column alignment
- * in printf-style messages.
- *
  * In this and all following functions the term "self" refers to the
  * current thread.
  */