Re: [PATCH 6/9] trace2: convert ctx.thread_name to flex array
- From
- Jeff Hostetler <git@jeffhostetler.com>
- Date
- Oct 10, 2022, 18:39 UTC
- 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:
Show 10 quoted lines
> > 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. >>>
[...]
Show 26 quoted lines
>
> 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