diff mbox series

[v2,4/7] trace2: rename the thread_name argument to trace2_thread_start

Message ID 637b422b8606b3b6d954e6a1959aae450507cdfa.1665600750.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Trace2 timers and counters and some cleanup | expand

Commit Message

Jeff Hostetler Oct. 12, 2022, 6:52 p.m. UTC 
From: Jeff Hostetler <jeffhost@microsoft.com>

Rename the `thread_name` argument in `tr2tls_create_self()` and
`trace2_thread_start()` to be `thread_base_name` to make it clearer
that the passed argument is a component used in the construction of
the actual `struct tr2tls_thread_ctx.thread_name` variable.

The base name will be used along with the thread id to create a
unique thread name.

This commit does not change how the `thread_name` field is
allocated or stored within the `tr2tls_thread_ctx` structure.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2.c         |  6 +++---
 trace2.h         | 11 ++++++-----
 trace2/tr2_tls.c |  4 ++--
 trace2/tr2_tls.h | 17 ++++++++++-------
 4 files changed, 21 insertions(+), 17 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Oct. 12, 2022, 9:06 p.m. UTC | #1 
On Wed, Oct 12 2022, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Rename the `thread_name` argument in `tr2tls_create_self()` and
> `trace2_thread_start()` to be `thread_base_name` to make it clearer
> that the passed argument is a component used in the construction of
> the actual `struct tr2tls_thread_ctx.thread_name` variable.
>
> The base name will be used along with the thread id to create a
> unique thread name.

Makes sense.

> This commit does not change how the `thread_name` field is
> allocated or stored within the `tr2tls_thread_ctx` structure.

What this commit does change though, which isn't mentioned here, is...

> diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h
> index 1297509fd23..7d1f03a2ea6 100644
> --- a/trace2/tr2_tls.h
> +++ b/trace2/tr2_tls.h
> @@ -25,17 +25,20 @@ struct tr2tls_thread_ctx {
>  /*
>   * Create thread-local storage for the current thread.
>   *
> - * We assume the first thread is "main".  Other threads are given
> - * 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.
> + * The first thread in the process will have:
> + *     { .thread_id=0, .thread_name="main" }
> + * Subsequent threads are given a non-zero thread_id and a thread_name
> + * constructed from the id and a thread base name (which is usually just
> + * the name of the thread-proc function).  For example:
> + *     { .thread_id=10, .thread_name="th10fsm-listen" }
> + * This helps to identify and distinguish messages from concurrent threads.
> + * The ctx.thread_name field is truncated if necessary to help with column
> + * alignment in printf-style messages.

...this documentation, which I'd argue should be a separate change, as
nothing's changed about the state of the world with this rename of the
field, this was all true before this rename.
Junio C Hamano Oct. 13, 2022, 9:12 p.m. UTC | #2 
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Rename the `thread_name` argument in `tr2tls_create_self()` and
> `trace2_thread_start()` to be `thread_base_name` to make it clearer
> that the passed argument is a component used in the construction of
> the actual `struct tr2tls_thread_ctx.thread_name` variable.
>
> The base name will be used along with the thread id to create a
> unique thread name.
> ...
> -struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
> +struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,
>  					     uint64_t us_thread_start)
>  {
>  	struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(*ctx));
> @@ -50,7 +50,7 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
>  	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);
> +	strbuf_addstr(&ctx->thread_name, thread_base_name);
>  	if (ctx->thread_name.len > TR2_MAX_THREAD_NAME)
>  		strbuf_setlen(&ctx->thread_name, TR2_MAX_THREAD_NAME);

This hunk is very illustrative and highlights the difference between
thread_base_name parameter and .thread_name member in the context.

Good.
Jeff Hostetler Oct. 20, 2022, 2:40 p.m. UTC | #3 
On 10/12/22 5:06 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Oct 12 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Rename the `thread_name` argument in `tr2tls_create_self()` and
>> `trace2_thread_start()` to be `thread_base_name` to make it clearer
>> that the passed argument is a component used in the construction of
>> the actual `struct tr2tls_thread_ctx.thread_name` variable.
>>
>> The base name will be used along with the thread id to create a
>> unique thread name.
> 
> Makes sense.
> 
>> This commit does not change how the `thread_name` field is
>> allocated or stored within the `tr2tls_thread_ctx` structure.
> 
> What this commit does change though, which isn't mentioned here, is...
> 
>> diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h
>> index 1297509fd23..7d1f03a2ea6 100644
>> --- a/trace2/tr2_tls.h
>> +++ b/trace2/tr2_tls.h
>> @@ -25,17 +25,20 @@ struct tr2tls_thread_ctx {
>>   /*
>>    * Create thread-local storage for the current thread.
>>    *
>> - * We assume the first thread is "main".  Other threads are given
>> - * 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.
>> + * The first thread in the process will have:
>> + *     { .thread_id=0, .thread_name="main" }
>> + * Subsequent threads are given a non-zero thread_id and a thread_name
>> + * constructed from the id and a thread base name (which is usually just
>> + * the name of the thread-proc function).  For example:
>> + *     { .thread_id=10, .thread_name="th10fsm-listen" }
>> + * This helps to identify and distinguish messages from concurrent threads.
>> + * The ctx.thread_name field is truncated if necessary to help with column
>> + * alignment in printf-style messages.
> 
> ...this documentation, which I'd argue should be a separate change, as
> nothing's changed about the state of the world with this rename of the
> field, this was all true before this rename.
> 

good point.  i'll split it and resend.
thanks
Jeff
diff mbox series

Patch

diff --git a/trace2.c b/trace2.c
index c1244e45ace..165264dc79a 100644
--- a/trace2.c
+++ b/trace2.c
@@ -466,7 +466,7 @@  void trace2_exec_result_fl(const char *file, int line, int exec_id, int code)
 				file, line, us_elapsed_absolute, exec_id, code);
 }
 
-void trace2_thread_start_fl(const char *file, int line, const char *thread_name)
+void trace2_thread_start_fl(const char *file, int line, const char *thread_base_name)
 {
 	struct tr2_tgt *tgt_j;
 	int j;
@@ -488,14 +488,14 @@  void trace2_thread_start_fl(const char *file, int line, const char *thread_name)
 		 */
 		trace2_region_enter_printf_fl(file, line, NULL, NULL, NULL,
 					      "thread-proc on main: %s",
-					      thread_name);
+					      thread_base_name);
 		return;
 	}
 
 	us_now = getnanotime() / 1000;
 	us_elapsed_absolute = tr2tls_absolute_elapsed(us_now);
 
-	tr2tls_create_self(thread_name, us_now);
+	tr2tls_create_self(thread_base_name, us_now);
 
 	for_each_wanted_builtin (j, tgt_j)
 		if (tgt_j->pfn_thread_start_fl)
diff --git a/trace2.h b/trace2.h
index af3c11694cc..74cdb1354f7 100644
--- a/trace2.h
+++ b/trace2.h
@@ -304,14 +304,15 @@  void trace2_exec_result_fl(const char *file, int line, int exec_id, int code);
  * thread-proc to allow the thread to create its own thread-local
  * storage.
  *
- * Thread names should be descriptive, like "preload_index".
- * Thread names will be decorated with an instance number automatically.
+ * The thread base name should be descriptive, like "preload_index" or
+ * taken from the thread-proc function.  A unique thread name will be
+ * created from the given base name and the thread id automatically.
  */
 void trace2_thread_start_fl(const char *file, int line,
-			    const char *thread_name);
+			    const char *thread_base_name);
 
-#define trace2_thread_start(thread_name) \
-	trace2_thread_start_fl(__FILE__, __LINE__, (thread_name))
+#define trace2_thread_start(thread_base_name) \
+	trace2_thread_start_fl(__FILE__, __LINE__, (thread_base_name))
 
 /*
  * Emit a 'thread_exit' event.  This must be called from inside the
diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
index 8d2182fbdbb..4f7c516ecb6 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -31,7 +31,7 @@  void tr2tls_start_process_clock(void)
 	tr2tls_us_start_process = getnanotime() / 1000;
 }
 
-struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
+struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,
 					     uint64_t us_thread_start)
 {
 	struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(*ctx));
@@ -50,7 +50,7 @@  struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
 	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);
+	strbuf_addstr(&ctx->thread_name, thread_base_name);
 	if (ctx->thread_name.len > TR2_MAX_THREAD_NAME)
 		strbuf_setlen(&ctx->thread_name, TR2_MAX_THREAD_NAME);
 
diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h
index 1297509fd23..7d1f03a2ea6 100644
--- a/trace2/tr2_tls.h
+++ b/trace2/tr2_tls.h
@@ -25,17 +25,20 @@  struct tr2tls_thread_ctx {
 /*
  * Create thread-local storage for the current thread.
  *
- * We assume the first thread is "main".  Other threads are given
- * 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.
+ * The first thread in the process will have:
+ *     { .thread_id=0, .thread_name="main" }
+ * Subsequent threads are given a non-zero thread_id and a thread_name
+ * constructed from the id and a thread base name (which is usually just
+ * the name of the thread-proc function).  For example:
+ *     { .thread_id=10, .thread_name="th10fsm-listen" }
+ * This helps to identify and distinguish messages from concurrent threads.
+ * The ctx.thread_name field is truncated 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.
  */
-struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name,
+struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,
 					     uint64_t us_thread_start);
 
 /*