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

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Oct 12, 2022, 13:31 UTC
Message-ID: <e138b178-99d9-e537-2cb1-c240962a35f2@jeffhostetler.com>
In-Reply-To: <221011.86h70ao4g6.gmgdl@evledraar.gmail.com>

On 10/11/22 9:31 AM, Ævar Arnfjörð Bjarmason wrote:

Show 12 quoted lines

> 
> On Mon, Oct 10 2022, Jeff Hostetler wrote:
> 
>> On 10/7/22 6:03 AM, Ævar Arnfjörð Bjarmason wrote:
>>> On Thu, Oct 06 2022, Junio C Hamano wrote:
>>>
>>>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>>>
>>>>> A cleaned up version of the test code I had on top of "master", RFC
>>>>> because I may still be missing some context here. E.g. maybe there's a
>>>>> plan to dynamically construct these thread names?
>>>>

[...]

Show 23 quoted lines

> I left more extensive commentary in the side-thread in
> https://lore.kernel.org/git/221011.86lepmo5dn.gmgdl@evledraar.gmail.com/,
> just a quick reply here.
> 
>> WRT optimizing memory usage.  We're talking about ~25 byte buffer
>> per thread.  Most commands execute in 1 thread -- if they read the
>> index they may have ~10 threads (depending on the size of the index
>> and if preload-index is enabled).  So, I don't think we really need
>> to optimize this.  Threading is used extensively in fsmonitor-daemon,
>> but it creates a fixed thread-pool at startup, so it may have ~12
>> threads.  Again, not worth optimizing for the thread-name field.
> 
> Yes, I agree it's not worth optimizing.
> 
> The reason for commenting on this part is that it isn't clear to me why
> your proposed patch then isn't doing the more obvious "it's not worth
> optimizing" pattern, per Junio's [1] comment on the initial version.
> 
> The "flex array" method is seemingly taking pains to reduce the runtime
> memory use of these by embedding this string in the space reserved for
> the struct.
> 
> So it's just meant as a question for you & the proposed patch.

I think we're converging on some common understanding (and I think we've gone around on this topic more than enough). :-)

I really was just trying to get rid of the strbuf and make it a fixed string -- I chose a flex-array rather than just detaching the buffer from a local in the thread-start code. I should have done the latter. I saw the flex-array as a fixed-size object that can't be replaced or extended (without recreating the thread-local storage) -- yes, people could overwrite existing bytes in-place in the flex-array, but who does that??

I understood what you were asking (illustrated in your RFC). That is, I understood the "what/how" you wanted to do to refactor / redesign the field, but I couldn't understand the "why". That is, why you've taken such interest in this field (and such a relatively unimportant change). We've spent nearly a week discussing it and we both agree that the optimization that I didn't suggest isn't worth doing. (I'm paraphrasing slightly.) :-)

So, rather than continuing with the back-n-forth, let me skip over the remaining questions in this thread and prepare a re-roll. Hopefully, I can simplify and more clearly explain the method to my madness and we can move on.

Show 6 quoted lines

>> Now, if you want to optimize over all trace2 events (a completely
>> different topic), you could create a large scratch strbuf buffer in
>> each thread context and use it so that we don't have to malloc/free
>> during each trace message.  That might be worth while.
> 
> *nod*

I'll make a note to revisit this idea in a future series.

Thanks Jeff