git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Jeff Hostetler <git@jeffhostetler.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:29:47 -0700	[thread overview]
Message-ID: <CABPp-BEa0hgrTXWAA0hVAZ3uRhsfLY=jEe17+2X5pQpRKiejMg@mail.gmail.com> (raw)
In-Reply-To: <96679919-f6ea-adcc-b8fc-9eaa0134c876@jeffhostetler.com>

On Tue, May 11, 2021 at 9:17 AM Jeff Hostetler <git@jeffhostetler.com> wrote:
>
> On 5/8/21 3:58 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >   dir.c                             |  34 ++++--
> >   t/t7063-status-untracked-cache.sh | 193 +++++++++++++++++-------------
> >   t/t7519-status-fsmonitor.sh       |   8 +-
> >   3 files changed, 135 insertions(+), 100 deletions(-)
> >
> > diff --git a/dir.c b/dir.c
> > index 3474e67e8f3c..9f7c8debeab3 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -2760,12 +2760,29 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
> >       return root;
> >   }
> >
> > +static void trace2_read_directory_statistics(struct dir_struct *dir,
> > +                                          struct repository *repo)
> > +{
> > +     if (!dir->untracked)
> > +             return;
>
> Is there value to also printing the path?
> The existing `trace_performance_leave()` calls were, but
> I'm familiar enough with this code to say if the output
> wasn't always something like ".".

The path will most likely just be "" (i.e. the empty string) for the
toplevel directory, but not always so it may be useful to print it.
I'll add it.

> > +     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);
> > +}
> > +
>
> The existing code was quite tangled and I think this helps
> make things more clear.
>
>
> >   int read_directory(struct dir_struct *dir, struct index_state *istate,
> >                  const char *path, int len, const struct pathspec *pathspec)
> >   {
> >       struct untracked_cache_dir *untracked;
> >
> > -     trace_performance_enter();
> > +     trace2_region_enter("dir", "read_directory", istate->repo);
> >
> >       if (has_symlink_leading_path(path, len)) {
> >               trace_performance_leave("read directory %.*s", len, path);
>
> This `trace_performance_leave()` inside the `if` needs to be
> converted too.

Ooh, good catch.  Will fix.

> > @@ -2784,23 +2801,13 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,
> >       QSORT(dir->entries, dir->nr, cmp_dir_entry);
> >       QSORT(dir->ignored, dir->ignored_nr, cmp_dir_entry);
> >
> > -     trace_performance_leave("read directory %.*s", len, path);
> > +     trace2_region_leave("dir", "read_directory", istate->repo);
>
> Can we put the call to `trace2_read_directory_statistics()` before
> the above `trace2_region_leave()` call?   Then those stats will
> appear indented between the begin- and end-region events in the output.
>
> That way, the following `if (dir-untracked) {...}` is only
> concerned with the untracked cache and/or freeing that data.

Makes sense, I'll move it.

> >       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 ||
> > @@ -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);
>
> Also, I think it'd be ok to move the `trace2_is_enabled()` call
> inside the function.  Since we're also testing `!dir->untracked`
> inside the function.

Actually, I can't do that.  The path passed to this function is not
going to always be (and will often not be) NUL-terminated, but
trace2_data_string() expects a NUL-terminated string.  So, I'm going
to make a temporary strbuf and copy the path into it, but of course I
only want to spend time doing that if trace2_is_enabled().

> The more that I look at the before and after versions, the
> more I think the `trace2_read_directory_statistics()` call
> should be up before the `trace2_region_leave()`.  Here at the
> bottom of the function, we may have already freed `dir->untracked`.
> I'm not familiar enough with this code to know if that is a
> good or bad thing.

Yeah, the statistics really need to be moved earlier, both for the
nesting reasons you point out and because otherwise the statistics
won't print whenever dir->untracked != istate->untracked.  I'll move
them.

  reply	other threads:[~2021-05-11 17:30 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
2021-05-11 16:17       ` Jeff Hostetler
2021-05-11 17:29         ` Elijah Newren [this message]
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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to='CABPp-BEa0hgrTXWAA0hVAZ3uRhsfLY=jEe17+2X5pQpRKiejMg@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).