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

On Mon, Oct 10 2022, Jeff Hostetler wrote:

I'm not seeking to replace your series, or to tick you off, sorry if it came across like that.

I just thought (and still think) that we were at a point in the discussion where it seemed clear that I wasn't quite managing to get across to you what I meant, so sending that in the form of working code should clarify things.

Per Junio's "That's nice to learn, indeed." in <xmqqo7uoh1q0.fsf@gitster.g> it seems to have had that intended effect on him. It's marked as an RFC, so not-a-thing-to-pick-up, but just for discussion.

I think that's a defensible opinion, but I also think it's fair to say that:

 * This seems to be *the* motivation for why you're doing things the way
   you're doing them, and at least to this reviewer that wasn't really
   coming across...

 * ...nor the context of why we'd need that sort of guarded API in this
   case, but not e.g. for another widely-used API like start_progress().

   See 791afae2924 (progress.c tests: make start/stop commands on stdin,
   2022-02-03) for a case where we're using that where we need to work
   around its behavior (and no, I didn't make the underlying API that
   way, it's just a commit of mine where I'm having to work with it).

I think designing our internal APIs to not be quite so guarded is fine, and we do that in various other contexts (progress, etc.). We control both the API and its users, so just leaving a "this must be a constant" should be enough.

But even if you want to be paranoid about it there's much easier ways to
do that which give you more of the safety you seem to want. E.g. this on
top of master (and easily adjusted on top of this RFC patch):
	
	diff --git a/trace2.h b/trace2.h
	index 88d906ea830..1c3a98fb30f 100644
	--- a/trace2.h
	+++ b/trace2.h
	@@ -306,12 +306,18 @@ void trace2_exec_result_fl(const char *file, int line, int exec_id, int code);
	  *
	  * Thread names should be descriptive, like "preload_index".
	  * Thread names will be decorated with an instance number automatically.
	+ * Thread names must point to data that won't change after it's passed
	+ * into this function. Once trace2_thread_exit() is called it can be
	+ * free'd.
	  */
	 void trace2_thread_start_fl(const char *file, int line,
	 			    const char *thread_name);
	 
	+/*
	+ * The "" is to assure us that API users pass only constant strings
	+ */
	 #define trace2_thread_start(thread_name) \
	-	trace2_thread_start_fl(__FILE__, __LINE__, (thread_name))
	+	trace2_thread_start_fl(__FILE__, __LINE__, (thread_name ""))
	 
	 /*
	  * Emit a 'thread_exit' event.  This must be called from inside the

Will pass, as we only pass it constant strings, but if someone were to pass a variable it'll blow up, at which point we could provide some inline macro/function that would do the required xstrdup().

All of which I think is *still* being too paranoid, but which I think *if* you want the paranoia is much more explicit about what we're trying to accomplish with said paranoida, and where the compiler will help you.

That's fair, more on that below.

If you actually care about this being fasterer -- and only reason for posting this RFC patch is to try to tease out *why* that is -- then this part of your concern can be trivially mitigated with having a struct member like:

	char thread_id_str[3];

We'd then just snprintf() into that in tr2tls_create_self(). Then when we print the thread to the JSON or log you'd do so without any strbuf_addf(), just a strbuf_addstr() or strbuf_add().

I think that micro-optimization isn't needed in this case, but it *is* easy to do .

A helper function being in the wrong place invalidates the whole commit?

I think you're right that this jw_strbuf_add_thread_name() helper should live somewhere else, probably in thread-utils.c.

So, pretending that it's in whatever place you'd be comfortable with, and using whatever naming convention you'd prefer. What do you think about the rest of the commit?

You snippet it just as you were getting to the meaty part of it, namely:

 * With this approach we can BUG() out as soon as we try to construct
   the main thread if its name is bad, we don't need to wait until
   runtime when a child thread runs into the limit.

 * We no longer need the whole thread-creation-time string duplication,
   associated storage in the struct etc.

 * That struct member is "const", addresing your initial concern of
   (from the upthread commit message):

	Using a (non-const) `strbuf` structure for it caused some
	confusion in the past because it implied that someone could
	rename a thread after it was created.  That usage was not
	intended.

   Although I think (and I'm possibly misreading it) that your
   commentary here is saying that even that's not enough, i.e. we can't
   just leave it at a "const" here, but must assume that an API user
   will disregard that and modify it after it's passed to us anyway.