| Message ID | pull.1373.git.1664900407.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Trace2 timers and counters and some cleanup | expand |
On Tue, Oct 04 2022, Jeff Hostetler via GitGitGadget wrote: > This patch series add stopwatch timers and global counters to the trace2 > logging facility. It also does a little housecleaning. > > This is basically a rewrite of the series that I submitted back in December > 2021: [1] and [2]. Hopefully, it addresses all of the concerns raised back > then and does it in a way that avoids the issues that stalled that effort. > > First we start with a few housecleaning commits: > > * The first 2 commits are unrelated to this effort, but were required to > get the existing code to compile on my Mac with Clang 11.0.0 with > DEVELOPER=1. Those can be dropped if there is a better way to do this. This seems like a good thing to have, but there's no subsequent changes to those two files on this topic, so is this just a "to get it building on my laptop..." stashed-on? I think if so it makes sense to split these up, and as feeback on 1-2/9: Let's note what compiler/version & what warning we got, the details there for anyone to dig this up later are missing, i.e. if we ever want to remove the workaround syntax. > * The 3rd commit is in response a concern about using int rather than > size_t for nr and alloc in an ALLOC_GROW() in existing trace2 code. This small bit of cleanup also could perhaps be submitted separately? It's unclear (and I read the concern in the initial thread) if this is required by anything that follows.
On 10/5/22 9:04 AM, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Oct 04 2022, Jeff Hostetler via GitGitGadget wrote: > >> This patch series add stopwatch timers and global counters to the trace2 >> logging facility. It also does a little housecleaning. >> >> This is basically a rewrite of the series that I submitted back in December >> 2021: [1] and [2]. Hopefully, it addresses all of the concerns raised back >> then and does it in a way that avoids the issues that stalled that effort. >> >> First we start with a few housecleaning commits: >> >> * The first 2 commits are unrelated to this effort, but were required to >> get the existing code to compile on my Mac with Clang 11.0.0 with >> DEVELOPER=1. Those can be dropped if there is a better way to do this. > > This seems like a good thing to have, but there's no subsequent changes > to those two files on this topic, so is this just a "to get it building > on my laptop..." stashed-on? Right. I needed them to get "main" to build on my laptop before I started hacking. I debated sending them in separately, but everyone was busy with the 2.38 release and didn't want to add to the noise for such a minor thing, since all the CI builds were green... But, yeah, I can do that. > > I think if so it makes sense to split these up, and as feeback on 1-2/9: > Let's note what compiler/version & what warning we got, the details > there for anyone to dig this up later are missing, i.e. if we ever want > to remove the workaround syntax. > >> * The 3rd commit is in response a concern about using int rather than >> size_t for nr and alloc in an ALLOC_GROW() in existing trace2 code. > > This small bit of cleanup also could perhaps be submitted separately? > It's unclear (and I read the concern in the initial thread) if this is > required by anything that follows. > Nothing requires this. It was just another "while I'm here" fixup. However, those lines are very close to new/changed lines that I added for the timers and counters, so it would probably cause collisions if sent independently. So I'd like to leave them in this series to simplify things. Thanks, Jeff
On 10/4/22 12:19 PM, Jeff Hostetler via GitGitGadget wrote: > This patch series add stopwatch timers and global counters to the trace2 > logging facility. It also does a little housecleaning. > > This is basically a rewrite of the series that I submitted back in December > 2021: [1] and [2]. Hopefully, it addresses all of the concerns raised back > then and does it in a way that avoids the issues that stalled that effort. Thanks for working on this again. As I mentioned earlier [3], this would be really helpful when doing performance investigations. I also plan to insert some timers and counters as a follow-up when this series stabilizes. [3] https://lore.kernel.org/git/pull.1365.git.1663938034607.gitgitgadget@gmail.com/ I was unable to find further improvements than the ones you already acknowledged for your v2. Thanks, -Stolee
This patch series add stopwatch timers and global counters to the trace2 logging facility. It also does a little housecleaning. This is basically a rewrite of the series that I submitted back in December 2021: [1] and [2]. Hopefully, it addresses all of the concerns raised back then and does it in a way that avoids the issues that stalled that effort. First we start with a few housecleaning commits: * The first 2 commits are unrelated to this effort, but were required to get the existing code to compile on my Mac with Clang 11.0.0 with DEVELOPER=1. Those can be dropped if there is a better way to do this. * The 3rd commit is in response a concern about using int rather than size_t for nr and alloc in an ALLOC_GROW() in existing trace2 code. * The 4th commit cleans up my use of the term "TLS" in my thread code. * The 5th and 6th commits (hopefully) clear up the misunderstandings around the thread_name variable in my thread context structures. My earlier attempts to clean and clarify this led to most of the controversies in the earlier patch series. Hopefully, these 2 commits will improve the clarify matters. * The 7th commit cleans up a mostly obsolete section in the trace2 API documentation. Finally, the last 2 commits add the stopwatch timers and the global counters. [1] https://lore.kernel.org/git/pull.1099.git.1640012469.gitgitgadget@gmail.com/ [2] https://lore.kernel.org/git/pull.1099.v2.git.1640720202.gitgitgadget@gmail.com/ Jeff Hostetler (9): builtin/merge-file: fix compiler warning on MacOS with clang 11.0.0 builtin/unpack-objects.c: fix compiler warning on MacOS with clang 11.0.0 trace2: use size_t alloc,nr_open_regions in tr2tls_thread_ctx tr2tls: clarify TLS terminology trace2: rename trace2 thread_name argument as name_hint trace2: convert ctx.thread_name to flex array api-trace2.txt: elminate section describing the public trace2 API trace2: add stopwatch timers trace2: add global counter mechanism Documentation/technical/api-trace2.txt | 190 +++++++++++++++++-------- Makefile | 2 + builtin/merge-file.c | 4 +- builtin/unpack-objects.c | 2 +- t/helper/test-trace2.c | 187 ++++++++++++++++++++++++ t/t0211-trace2-perf.sh | 95 +++++++++++++ t/t0211/scrub_perf.perl | 6 + trace2.c | 121 +++++++++++++++- trace2.h | 101 +++++++++++-- trace2/tr2_ctr.c | 101 +++++++++++++ trace2/tr2_ctr.h | 104 ++++++++++++++ trace2/tr2_tgt.h | 14 ++ trace2/tr2_tgt_event.c | 47 +++++- trace2/tr2_tgt_normal.c | 39 +++++ trace2/tr2_tgt_perf.c | 49 ++++++- trace2/tr2_tls.c | 43 +++--- trace2/tr2_tls.h | 52 ++++--- trace2/tr2_tmr.c | 182 +++++++++++++++++++++++ trace2/tr2_tmr.h | 140 ++++++++++++++++++ 19 files changed, 1366 insertions(+), 113 deletions(-) create mode 100644 trace2/tr2_ctr.c create mode 100644 trace2/tr2_ctr.h create mode 100644 trace2/tr2_tmr.c create mode 100644 trace2/tr2_tmr.h base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1373%2Fjeffhostetler%2Ftrace2-stopwatch-v4-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1373/jeffhostetler/trace2-stopwatch-v4-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1373