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

From: Jeff Hostetler <git@jeffhostetler.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
	Jeff Hostetler <jeffhost@microsoft.com>,
	Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com>
Subject: Re: [RFC PATCH] trace2 API: don't save a copy of constant "thread_name"
Date: Mon, 10 Oct 2022 15:05:42 -0400	[thread overview]
Message-ID: <cb9f8321-d9e6-6f80-a590-a9ad49c7f557@jeffhostetler.com> (raw)
In-Reply-To: <RFC-patch-1.1-8563d017137-20221007T010829Z-avarab@gmail.com>

On 10/6/22 9:10 PM, Ævar Arnfjörð Bjarmason wrote:
> 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>

PLEASE DON'T DO THIS.

If you don't like my patch, fine.  Let's discuss it.  But DON'T submit
a new one to replace it.  Or worse, try to inject it into the middle
of an existing series.

Yes, current callers are passing a string literal and thread-start
could take a "const char*" to it, but there is no way to guarantee
that that is safe if someone decides to dynamically construct their
thread-name and pass it in (since we don't know the lifetime of that
pointer).  So it is safer to copy it into the thread context so that
it can be used by later trace messages.

[...]
> +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);

This violates a separation of concerns.  json-writer is ONLY concerned
with formatting valid JSON from basic data types.  It does not know
about threads or thread contexts.

`js_strbuf_add_thread_name()` also violates the json-writer conventions
-- that it takes a "struct json_writer *" pointer.  There is nothing
about JSON here.

You might write a helper (inside of tr2_tgt_event.c) that formats a
thread-name from the id and hint, but that is specific to the Event
target -- not to JSON, nor the JSON writer.

But then again, why make every trace message from every target format
that "th%0d:%s" when we could save some time and format it in the
thread-start and just USE it.

[...]
> @@ -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);

This stands out as very wrong.  The _Perf target does not use JSON
at all, yet here we are calling a jw_ routine.  Again, that code is
in the wrong place.

I'm going to clip the rest of this commit, since the above invalidates
it.

Jeff