From: Jeff Hostetler <git@jeffhostetler.com>
Date: Mon, 10 Oct 2022 18:39:37 GMT
Subject: Re: [PATCH 6/9] trace2: convert ctx.thread_name to flex array
Message-ID: <3fbc1d79-4b7d-ef5d-1056-0512a3aa4e9a@jeffhostetler.com>
In-Reply-To: <221006.86ilkwr6wy.gmgdl@evledraar.gmail.com>



On 10/6/22 5:05 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Oct 05 2022, Junio C Hamano wrote:
> 
>> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>>
>>> Convert the `tr2tls_thread_ctx.thread_name` field from a `strbuf`
>>> to a "flex array" at the end of the context structure.
>>>
[...]
> 
> We don't even need that, AFAICT. My reply at [1] is rather long, but the
> tl;dr is that the interface for this API is:
> 	
> 	$ git 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");
> 	trace2.h:       trace2_thread_start_fl((thread_hint), __FILE__, __LINE__)
> 
> And we are taking e.g. "preload_thread" and turning it into strings like
> these, and saving it into "struct tr2tls_thread_ctx".
> 
> 	"preload_thread", // main thread
> 	"th01:preload_thread", // 1st thread
> 	"th02:preload_thread" // 2nd thread
> 	[...]
> 
> 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.

Current callers tend to pass a string literal.  There's nothing
to say that they will continue to do so in the future.


> Then we just format the "%s" or (if ".thread_id" == 0) or "th%02d:%s"
> (if ".thread_id" > 0) on-the-fly, the two codepaths that end up using
> this are already using strbuf_addf(), so just adding to the format there
> is easy.
[...]

But then you'd be formatting this "th%0d:%s" on every message
printer.  Whereas we can format it once in the thread-start and
save the extra work -- at the expense of a string buffer in the
thread context.

Granted, the event handlers are generating output lines with many
"%" fields, so they are doing non-trivial amounts of work on every
event, but by using a pre-formatted thread-name, we don't need to
increase that workload.

Jeff