git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff Hostetler <git@jeffhostetler.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <stolee@gmail.com>,
	Matheus Tavares <matheus.bernardino@usp.br>,
	Johannes Sixt <j6t@kdbg.org>,
	Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH v2 0/9] Trace2 stopwatch timers and global counters
Date: Thu, 30 Dec 2021 11:42:13 -0500	[thread overview]
Message-ID: <a316a9e0-14dc-6277-b7d7-f6f115cc81da@jeffhostetler.com> (raw)
In-Reply-To: <211229.86ee5wgnug.gmgdl@evledraar.gmail.com>



On 12/28/21 8:54 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Dec 28 2021, Jeff Hostetler via GitGitGadget wrote:
> 
> I left some other comments on the series inline, just on the notes in
> the CL:
> 
>>   * Ævar proposed a large refactor of the "_perf" target to have a "fmt()"
>>     varargs function to reduce the amount of copy-n-pasted code in many of
>>     the "fn" event handlers. This looks like a good change based on the
>>     mockup but is a large refactor.
> 
> FWIW what I meant with [1] was not that this series needed to take the
> detour of refactoring trace2/tr2_tgt_perf.c to use such a helper, but
> that for the function additions in this series it might make sense to
> introduce one and use it for the new functions.
> 
> For this series I think it's probably not worth it, so I'm fine with
> leaving this for some other time. Just pointing out that rather than
> your reading of:
> 
>   1. We have some refactorable verbosity
>   2. Refactor all callers
>   3. Change existing code to use that refactoring
>   4. Add new code to use the refactoring
> 
> It's also perfectly fine to do just:
> 
>   1. We have some refactorable verbosity
>   2. Introduce a less verbose
>   3. Add new code to use the helper
> 
> And leave the "refactor all callers" for some other time.
> 
> Anyway, I think for the two callers just leaving it entirely for this
> series is the right thing to do. It was more of a "hrm, that's some odd
> and avoidable verbosity..." comment on me read-through of v1.
> 
> 1. https://lore.kernel.org/git/211220.86czlrurm6.gmgdl@evledraar.gmail.com/

Sorry, but I'm going to call BS on this.  You sent a ~200 line diff
showing how we could refactor and reduce some of the duplicated code.
You have a history of introducing unnecessary refactorings in the middle
of other topics, and this looks like another example of that.  Another
example of distracting everyone from reviewing the actual new code.

And when I say that it should be an independent topic in its own
series, you fall back to the your "oh, it was just a drive-by comment."
and/or "i didn't mean for you to actually do it." and/or "you just
read my email incorrectly."

Drive-by comments don't usually have ~200 line diffs attached....

A drive-by comment would just say that "there is an opportunity to
create a varargs version of the existing io function and reduce
some duplication in the bodies of the existing callers" and be done.
I don't need a 200 line diff to see how you spell that.

Again, sorry to rant, but I'm tired looking like the stupid half
in these conversations.

> 
>>   * Ævar proposed a new rationale for when/why we change the "_event" version
>>     number. That text can be added to the design document independently.
> 
> Hrm, no. In [1] I linked to some earlier musings of mine about what we
> should do about the TR2_EVENT_VERSION (mainly as an FYI since you added
> it, but hadn't commented on that post).
> 
> But my main comment there was that the series wasn't progressing as
> atomic changes. I.e. we promise to change the TR2_EVENT_VERSION version
> every time we change the event format, but v1 first changed the format
> and bumped the version, then made some more changes.

Did you really expect me to change it twice within a single 9 commit
patch series?

This series creates both "timers" and "counters" and will both appear
together if/when they are merged.  From an external point of view,
users would see version 4 added two new event types.  So I either
increment it for "timers" or I increment it for "counters" or I squash
the two commits together and increment it then.

I didn't want to squash them, so I chose the former.

> 
> I think that's probably fine per-se within a git release cycle, but it
> might be a symtom of commits that could be split up to be more atomic (I
> don't know, didn't look in detail).
> 
> However, in this v2 of the series the TR2_EVENT_VERSION bump is entirely
> gone.

You complained when/how I bumped it in V1.  So I removed it.

And I suggested that you commit your "earlier musings".  With
that in place, there would be no need for me to change the
version number (which is what you wanted all along, right?)


> 
> Maybe that means that you so vehemently agree with my proposal in [1] it
> that you'd like to start taking that view for trace2 changes right away
> :-)

s/so vehemently agree with/are tired of debating/

> 
> For me it's fine either way, I think TR2_EVENT_VERSION probably isn't
> that important.
> 
> But if that's the case it should probably be called out more explictly
> in the CL/commit. I.e. even if our "policy" (such as it is) about
> TR2_EVENT_VERSION currently says X we're going to start doing Y here
> intentionally.
> 
> And in that case I should probably turn that suggestion in [1] into a an
> actual PATCH sooner than later...
> 
> 1. https://lore.kernel.org/git/211220.86czlrurm6.gmgdl@evledraar.gmail.com/
> 

Right, I'll add a note to V3 stating that I did not update
the version number.

Jeff

      reply	other threads:[~2021-12-30 16:42 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-20 15:01 [PATCH 0/9] Trace2 stopwatch timers and global counters Jeff Hostetler via GitGitGadget
2021-12-20 15:01 ` [PATCH 1/9] trace2: use size_t alloc,nr_open_regions in tr2tls_thread_ctx Jeff Hostetler via GitGitGadget
2021-12-20 15:01 ` [PATCH 2/9] trace2: convert tr2tls_thread_ctx.thread_name from strbuf to char* Jeff Hostetler via GitGitGadget
2021-12-20 16:31   ` Ævar Arnfjörð Bjarmason
2021-12-20 19:07     ` Jeff Hostetler
2021-12-20 19:35       ` Ævar Arnfjörð Bjarmason
2021-12-22 16:32         ` Jeff Hostetler
2021-12-21  7:33     ` Junio C Hamano
2021-12-21  7:22   ` Junio C Hamano
2021-12-22 16:28     ` Jeff Hostetler
2021-12-22 19:57       ` Junio C Hamano
2021-12-20 15:01 ` [PATCH 3/9] trace2: defer free of TLS CTX until program exit Jeff Hostetler via GitGitGadget
2021-12-21  7:30   ` Junio C Hamano
2021-12-22 21:59     ` Jeff Hostetler
2021-12-22 22:56       ` Junio C Hamano
2021-12-22 23:04         ` Jeff Hostetler
2021-12-23  7:38         ` Johannes Sixt
2021-12-23 18:18           ` Junio C Hamano
2021-12-27 18:51             ` Jeff Hostetler
2021-12-20 15:01 ` [PATCH 4/9] trace2: add thread-name override to event target Jeff Hostetler via GitGitGadget
2021-12-20 15:01 ` [PATCH 5/9] trace2: add thread-name override to perf target Jeff Hostetler via GitGitGadget
2021-12-20 15:01 ` [PATCH 6/9] trace2: add timer events to perf and event target formats Jeff Hostetler via GitGitGadget
2021-12-20 16:39   ` Ævar Arnfjörð Bjarmason
2021-12-20 19:44     ` Jeff Hostetler
2021-12-21 14:20   ` Derrick Stolee
2021-12-20 15:01 ` [PATCH 7/9] trace2: add stopwatch timers Jeff Hostetler via GitGitGadget
2021-12-20 16:42   ` Ævar Arnfjörð Bjarmason
2021-12-22 21:38     ` Jeff Hostetler
2021-12-21 14:45   ` Derrick Stolee
2021-12-22 21:57     ` Jeff Hostetler
2021-12-20 15:01 ` [PATCH 8/9] trace2: add counter events to perf and event target formats Jeff Hostetler via GitGitGadget
2021-12-20 16:51   ` Ævar Arnfjörð Bjarmason
2021-12-22 22:56     ` Jeff Hostetler
2021-12-20 15:01 ` [PATCH 9/9] trace2: add global counters Jeff Hostetler via GitGitGadget
2021-12-20 17:14   ` Ævar Arnfjörð Bjarmason
2021-12-22 22:18     ` Jeff Hostetler
2021-12-21 14:51 ` [PATCH 0/9] Trace2 stopwatch timers and " Derrick Stolee
2021-12-21 23:27   ` Matheus Tavares
2021-12-28 19:36 ` [PATCH v2 " Jeff Hostetler via GitGitGadget
2021-12-28 19:36   ` [PATCH v2 1/9] trace2: use size_t alloc,nr_open_regions in tr2tls_thread_ctx Jeff Hostetler via GitGitGadget
2021-12-29  0:48     ` Ævar Arnfjörð Bjarmason
2021-12-28 19:36   ` [PATCH v2 2/9] trace2: convert tr2tls_thread_ctx.thread_name from strbuf to flex array Jeff Hostetler via GitGitGadget
2021-12-29  1:11     ` Ævar Arnfjörð Bjarmason
2021-12-29 16:46       ` Jeff Hostetler
2021-12-28 19:36   ` [PATCH v2 3/9] trace2: defer free of thread local storage until program exit Jeff Hostetler via GitGitGadget
2021-12-28 19:36   ` [PATCH v2 4/9] trace2: add thread-name override to event target Jeff Hostetler via GitGitGadget
2021-12-28 19:36   ` [PATCH v2 5/9] trace2: add thread-name override to perf target Jeff Hostetler via GitGitGadget
2021-12-29  1:48     ` Ævar Arnfjörð Bjarmason
2021-12-29 17:15       ` Jeff Hostetler
2021-12-28 19:36   ` [PATCH v2 6/9] trace2: add timer events to perf and event target formats Jeff Hostetler via GitGitGadget
2021-12-28 19:36   ` [PATCH v2 7/9] trace2: add stopwatch timers Jeff Hostetler via GitGitGadget
2021-12-28 19:36   ` [PATCH v2 8/9] trace2: add counter events to perf and event target formats Jeff Hostetler via GitGitGadget
2021-12-28 19:36   ` [PATCH v2 9/9] trace2: add global counters Jeff Hostetler via GitGitGadget
2021-12-29  1:54   ` [PATCH v2 0/9] Trace2 stopwatch timers and " Ævar Arnfjörð Bjarmason
2021-12-30 16:42     ` Jeff Hostetler [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a316a9e0-14dc-6277-b7d7-f6f115cc81da@jeffhostetler.com \
    --to=git@jeffhostetler.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=j6t@kdbg.org \
    --cc=jeffhost@microsoft.com \
    --cc=matheus.bernardino@usp.br \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).