From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
Git Mailing List <git@vger.kernel.org>,
Eric Sunshine <sunshine@sunshineco.com>,
Derrick Stolee <stolee@gmail.com>, Jeff King <peff@peff.net>,
Philip Oakley <philipoakley@iee.email>,
Jeff Hostetler <jeffhost@microsoft.com>,
Josh Steadmon <steadmon@google.com>
Subject: Re: [PATCH v3 1/8] [RFC] dir: convert trace calls to trace2 equivalents
Date: Tue, 11 May 2021 10:23:48 -0700 [thread overview]
Message-ID: <CABPp-BFk_VNv7jzDP8Ep0Yk2e_nNg7-EAK1rbpS=9y9oNmi0kg@mail.gmail.com> (raw)
In-Reply-To: <xmqqk0o7ry5c.fsf@gitster.g>
On Sun, May 9, 2021 at 9:49 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +static void trace2_read_directory_statistics(struct dir_struct *dir,
> > + struct repository *repo)
> > +{
> > + if (!dir->untracked)
> > + return;
> > + trace2_data_intmax("read_directory", repo,
> > + "node-creation", dir->untracked->dir_created);
> > + trace2_data_intmax("read_directory", repo,
> > + "gitignore-invalidation",
> > + dir->untracked->gitignore_invalidated);
> > + trace2_data_intmax("read_directory", repo,
> > + "directory-invalidation",
> > + dir->untracked->dir_invalidated);
> > + trace2_data_intmax("read_directory", repo,
> > + "opendir", dir->untracked->dir_opened);
> > +}
> > +
>
> This obviously looks like an equivalent to what happens in the
> original inside the "if (dir->untracked)" block.
>
> And we have a performance_{enter,leave} pair replaced with
> a region_[enter,leave} pair.
>
> > - trace_performance_enter();
> > + trace2_region_enter("dir", "read_directory", istate->repo);
> > ...
> > - trace_performance_leave("read directory %.*s", len, path);
> > + trace2_region_leave("dir", "read_directory", istate->repo);
>
> > if (dir->untracked) {
> > static int force_untracked_cache = -1;
> > - static struct trace_key trace_untracked_stats = TRACE_KEY_INIT(UNTRACKED_STATS);
> >
> > if (force_untracked_cache < 0)
> > force_untracked_cache =
> > git_env_bool("GIT_FORCE_UNTRACKED_CACHE", 0);
> > - trace_printf_key(&trace_untracked_stats,
> > - "node creation: %u\n"
> > - "gitignore invalidation: %u\n"
> > - "directory invalidation: %u\n"
> > - "opendir: %u\n",
> > - dir->untracked->dir_created,
> > - dir->untracked->gitignore_invalidated,
> > - dir->untracked->dir_invalidated,
> > - dir->untracked->dir_opened);
> > if (force_untracked_cache &&
> > dir->untracked == istate->untracked &&
> > (dir->untracked->dir_opened ||
>
> Removal of the trace_printf() in the middle made the body of this
> if() statement much less distracting, which is good.
>
> > @@ -2811,6 +2818,9 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,
> > FREE_AND_NULL(dir->untracked);
> > }
> > }
> > +
> > + if (trace2_is_enabled())
> > + trace2_read_directory_statistics(dir, istate->repo);
>
> This slightly changes the semantics in that the original did an
> equivalent emitting from inside the "if (dir->untracked)" block, but
> this call is hoisted outside, and the new helper knows how to be
> silent when untracked thing is not in effect, so the net effect at
> this step is the same. And if we ever add tracing statics that is
> relevant when !dir->untracked is true, the new code organization is
> easier to work with.
>
> The only curious thing is the guard "if (trace2_is_enabled())";
> correctness-wise, are there bad things going to happen if it is not
> here, or is this a performance hack, or is it more for its
> documentation value (meaning, it would be a bug if we later added
> things that are irrelevant when trace is not enabled to the helper)?
No, there's nothing bad that would happen here. It was a combination
of a performance hack and documentation in case
trace2_read_directory_statistics() started gaining other code besides
trace2_*() calls, but which code was only relevant when trace2 was
enabled.
Turns out, though, that Jeff's suggestion to also print the path in
the statistics is going to require me creating a temporary strbuf so
that I can get a NUL-terminated string. We only want to do that when
trace2_is_enabled(), so that will make the introduction of that check
a bit more natural.
> > @@ -57,6 +57,19 @@ iuc () {
> > return $ret
> > }
> >
> > +get_relevant_traces() {
>
> Style. SP on both sides of "()".
Will fix.
>
> > + # From the GIT_TRACE2_PERF data of the form
> > + # $TIME $FILE:$LINE | d0 | main | data | r1 | ? | ? | read_directo | $RELEVANT_STAT
> > + # extract the $RELEVANT_STAT fields. We don't care about region_enter
> > + # or region_leave, or stats for things outside read_directory.
> > + INPUT_FILE=$1
> > + OUTPUT_FILE=$2
> > + grep data.*read_directo $INPUT_FILE \
> > + | cut -d "|" -f 9 \
> > + >$OUTPUT_FILE
>
> Style. Wrapping the line after pipe '|' will allow you to omit the
> backslash. Also quote the redirection target, i.e. >"$OUTPUT_FILE",
> to help certain vintage of bash.
Will fix.
next prev parent reply other threads:[~2021-05-11 17:24 UTC|newest]
Thread overview: 90+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-07 4:04 [PATCH 0/5] Directory traversal fixes Elijah Newren via GitGitGadget
2021-05-07 4:04 ` [PATCH 1/5] t7300: add testcase showing unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-07 4:27 ` Eric Sunshine
2021-05-07 5:00 ` Elijah Newren
2021-05-07 5:31 ` Eric Sunshine
2021-05-07 5:42 ` Elijah Newren
2021-05-07 5:56 ` Eric Sunshine
2021-05-07 23:05 ` Jeff King
2021-05-07 23:15 ` Eric Sunshine
2021-05-08 0:04 ` Elijah Newren
2021-05-08 0:10 ` Eric Sunshine
2021-05-08 17:20 ` Elijah Newren
2021-05-08 11:13 ` Philip Oakley
2021-05-08 17:20 ` Elijah Newren
2021-05-07 4:04 ` [PATCH 2/5] t3001, t7300: add testcase showcasing missed directory traversal Elijah Newren via GitGitGadget
2021-05-07 4:04 ` [PATCH 3/5] dir: avoid unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-07 4:04 ` [PATCH 4/5] dir: traverse into untracked directories if they may have ignored subfiles Elijah Newren via GitGitGadget
2021-05-07 4:05 ` [PATCH 5/5] [RFC] ls-files: error out on -i unless -o or -c are specified Elijah Newren via GitGitGadget
2021-05-07 16:22 ` [PATCH 6/5] dir: update stale description of treat_directory() Derrick Stolee
2021-05-07 17:57 ` Elijah Newren
2021-05-07 16:27 ` [PATCH 0/5] Directory traversal fixes Derrick Stolee
2021-05-08 0:08 ` [PATCH v2 0/8] " Elijah Newren via GitGitGadget
2021-05-08 0:08 ` [PATCH v2 1/8] t7300: add testcase showing unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-08 10:13 ` Junio C Hamano
2021-05-08 17:34 ` Elijah Newren
2021-05-08 10:19 ` Junio C Hamano
2021-05-08 17:41 ` Elijah Newren
2021-05-08 0:08 ` [PATCH v2 2/8] t3001, t7300: add testcase showcasing missed directory traversal Elijah Newren via GitGitGadget
2021-05-08 0:08 ` [PATCH v2 3/8] dir: avoid unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-08 0:08 ` [PATCH v2 4/8] dir: traverse into untracked directories if they may have ignored subfiles Elijah Newren via GitGitGadget
2021-05-08 0:08 ` [PATCH v2 5/8] [RFC] ls-files: error out on -i unless -o or -c are specified Elijah Newren via GitGitGadget
2021-05-08 0:08 ` [PATCH v2 6/8] dir: update stale description of treat_directory() Derrick Stolee via GitGitGadget
2021-05-08 0:08 ` [PATCH v2 7/8] [RFC] dir: convert trace calls to trace2 equivalents Elijah Newren via GitGitGadget
2021-05-08 0:08 ` [PATCH v2 8/8] [RFC] dir: reported number of visited directories and paths with trace2 Elijah Newren via GitGitGadget
2021-05-08 19:58 ` [PATCH v3 0/8] Directory traversal fixes Elijah Newren via GitGitGadget
2021-05-08 19:58 ` [PATCH v3 1/8] [RFC] dir: convert trace calls to trace2 equivalents Elijah Newren via GitGitGadget
2021-05-10 4:49 ` Junio C Hamano
2021-05-11 17:23 ` Elijah Newren [this message]
2021-05-11 16:17 ` Jeff Hostetler
2021-05-11 17:29 ` Elijah Newren
2021-05-08 19:58 ` [PATCH v3 2/8] [RFC] dir: report number of visited directories and paths with trace2 Elijah Newren via GitGitGadget
2021-05-10 5:00 ` Junio C Hamano
2021-05-08 19:58 ` [PATCH v3 3/8] [RFC] ls-files: error out on -i unless -o or -c are specified Elijah Newren via GitGitGadget
2021-05-10 5:09 ` Junio C Hamano
2021-05-11 17:40 ` Elijah Newren
2021-05-11 22:32 ` Junio C Hamano
2021-05-08 19:59 ` [PATCH v3 4/8] t7300: add testcase showing unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-10 5:28 ` Junio C Hamano
2021-05-11 17:45 ` Elijah Newren
2021-05-11 22:43 ` Junio C Hamano
2021-05-12 2:07 ` Elijah Newren
2021-05-12 3:17 ` Junio C Hamano
2021-05-08 19:59 ` [PATCH v3 5/8] t3001, t7300: add testcase showcasing missed directory traversal Elijah Newren via GitGitGadget
2021-05-08 19:59 ` [PATCH v3 6/8] dir: avoid unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-10 5:48 ` Junio C Hamano
2021-05-11 17:57 ` Elijah Newren
2021-05-08 19:59 ` [PATCH v3 7/8] dir: traverse into untracked directories if they may have ignored subfiles Elijah Newren via GitGitGadget
2021-05-08 19:59 ` [PATCH v3 8/8] dir: update stale description of treat_directory() Derrick Stolee via GitGitGadget
2021-05-11 18:34 ` [PATCH v4 0/8] Directory traversal fixes Elijah Newren via GitGitGadget
2021-05-11 18:34 ` [PATCH v4 1/8] dir: convert trace calls to trace2 equivalents Elijah Newren via GitGitGadget
2021-05-11 19:06 ` Jeff Hostetler
2021-05-11 20:12 ` Elijah Newren
2021-05-11 23:12 ` Jeff Hostetler
2021-05-12 0:44 ` Elijah Newren
2021-05-12 12:26 ` Jeff Hostetler
2021-05-12 15:24 ` Elijah Newren
2021-05-11 18:34 ` [PATCH v4 2/8] dir: report number of visited directories and paths with trace2 Elijah Newren via GitGitGadget
2021-05-11 18:34 ` [PATCH v4 3/8] ls-files: error out on -i unless -o or -c are specified Elijah Newren via GitGitGadget
2021-05-11 18:34 ` [PATCH v4 4/8] t7300: add testcase showing unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-11 18:34 ` [PATCH v4 5/8] t3001, t7300: add testcase showcasing missed directory traversal Elijah Newren via GitGitGadget
2021-05-11 18:34 ` [PATCH v4 6/8] dir: avoid unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-11 18:34 ` [PATCH v4 7/8] dir: traverse into untracked directories if they may have ignored subfiles Elijah Newren via GitGitGadget
2021-05-11 18:34 ` [PATCH v4 8/8] dir: update stale description of treat_directory() Derrick Stolee via GitGitGadget
2021-05-12 17:28 ` [PATCH v5 0/9] Directory traversal fixes Elijah Newren via GitGitGadget
2021-05-12 17:28 ` [PATCH v5 1/9] dir: convert trace calls to trace2 equivalents Elijah Newren via GitGitGadget
2021-05-12 17:28 ` [PATCH v5 2/9] dir: report number of visited directories and paths with trace2 Elijah Newren via GitGitGadget
2021-05-12 17:28 ` [PATCH v5 3/9] ls-files: error out on -i unless -o or -c are specified Elijah Newren via GitGitGadget
2021-05-12 17:28 ` [PATCH v5 4/9] t7300: add testcase showing unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-12 17:28 ` [PATCH v5 5/9] t3001, t7300: add testcase showcasing missed directory traversal Elijah Newren via GitGitGadget
2021-05-12 17:28 ` [PATCH v5 6/9] dir: avoid unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-12 17:28 ` [PATCH v5 7/9] dir: traverse into untracked directories if they may have ignored subfiles Elijah Newren via GitGitGadget
2021-05-12 17:28 ` [PATCH v5 8/9] dir: update stale description of treat_directory() Derrick Stolee via GitGitGadget
2021-05-17 17:20 ` Derrick Stolee
2021-05-17 19:44 ` Junio C Hamano
2021-05-18 3:32 ` Elijah Newren
2021-05-19 1:44 ` Junio C Hamano
2021-05-12 17:28 ` [PATCH v5 9/9] dir: introduce readdir_skip_dot_and_dotdot() helper Elijah Newren via GitGitGadget
2021-05-17 17:22 ` Derrick Stolee
2021-05-18 3:34 ` Elijah Newren
2021-05-17 17:23 ` [PATCH v5 0/9] Directory traversal fixes Derrick Stolee
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='CABPp-BFk_VNv7jzDP8Ep0Yk2e_nNg7-EAK1rbpS=9y9oNmi0kg@mail.gmail.com' \
--to=newren@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=jeffhost@microsoft.com \
--cc=peff@peff.net \
--cc=philipoakley@iee.email \
--cc=steadmon@google.com \
--cc=stolee@gmail.com \
--cc=sunshine@sunshineco.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.