Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files

[Utsav Shah <utsav@dropbox.com>]

On Wed, Nov 6, 2019 at 2:46 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Utsav Shah <utsav@dropbox.com>
> >
> > The index might be aware that a file hasn't modified via fsmonitor, but
> > unpack-trees did not pay attention to it and checked via ie_match_stat
> > which can be inefficient on certain filesystems. This significantly slows
> > down commands that run oneway_merge, like checkout and reset --hard.
>
> s/hasn't/& been/;
>
> Otherwise, well written.
>
> > This patch makes oneway_merge check whether a file is considered
> > unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees
> > also now correctly copies over fsmonitor validity state from the source
> > index. Finally, for correctness, we force a refresh of fsmonitor state in
> > tweak_fsmonitor.
>
> s/This patch makes/Make/; order the person who is updating the code
> what to do to the codebase in imperative mood.
>
> Otherwise, well written.
>
> > After this change, commands like stash (that use reset --hard
> > internally) go from 8s or more to ~2s on a 250k file repository on a
> > mac.
> >
> > Signed-off-by: Utsav Shah <utsav@dropbox.com>
> > ---
> >  fsmonitor.c                 | 39 ++++++++++++++++++++++++-------------
> >  t/t7519-status-fsmonitor.sh |  9 +++++++--
> >  unpack-trees.c              |  6 +++++-
> >  3 files changed, 37 insertions(+), 17 deletions(-)
> >
> > diff --git a/fsmonitor.c b/fsmonitor.c
> > index 1f4aa1b150..04d6232531 100644
> > --- a/fsmonitor.c
> > +++ b/fsmonitor.c
> > @@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)
> >
> >       if (pos >= istate->cache_nr)
> >               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
> > -                 (uintmax_t)pos, istate->cache_nr);
> > +                     (uintmax_t)pos, istate->cache_nr);
>
> Unintended whitespace change?
>
> > @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
> >       }
> >       istate->fsmonitor_dirty = fsmonitor_dirty;
> >
> > -     if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > -             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> > -                 (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> > +     if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > +             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
> > +                     (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
> > +
>
> The patch disables this sanity check under "split index" mode and it
> must be done for good reasons, but readers (imagine, somebody found
> a bug on this line 6 months down the road, ran "git blame" and found
> this commit and reading it via "git show") would want to know why
> this change was made.
>
> I recall seeing no mention of "split index" in the proposed log
> message.  Is this a fix for unrelated issue that needs to be
> explained in a separate patch, perhaps?
>
> The hunk also has the unintended whitespace change, it seems.
>
> > @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
> >       uint32_t ewah_size = 0;
> >       int fixup = 0;
> >
> > -     if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > -             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> > -                 (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> > +     if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > +             BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
> > +                     (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
>
> Likewise (both indentation of the second line and the unexplained
> change to the sanity check condition we saw above).
>
> > @@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate)
> >               }
> >               if (bol < query_result.len)
> >                       fsmonitor_refresh_callback(istate, buf + bol);
> > +
> > +             if (istate->untracked)
> > +                     istate->untracked->use_fsmonitor = 1;
>
> Unexplained.  We used to do this in tweak_fsmonitor() but now we do
> this here, as we are making tweak_fsmonitor() to call this function
> anyway.  If there are other callers that call refresh_fsmonitor()
> and they did not do this, this would be a behaviour change to them.
> As there is no explanation why this change is done, readers cannot
> tell if it is a good change.  If this were explained like so:
>
>     Any caller of refresh_fsmonitor() must set use_fsmonitor bit in
>     istate when istate->untracked exists FOR SUCH AND SUCH REASONS.
>     Move the code to do so in tweak_fsmonitor() to near the
>     beginning of refresh_fsmonitor(), which would fix SUCH AND SUCH
>     other callers that forgets to do this.
>
> in the proposed log message, that might help justifying the change.
>
> If use_fsmonitor is not set, why is the caller calling
> refresh_fsmonitor() in the first place, by the way?
>
> Isn't it more like "we are told to use fsmonitor via
> istate->untracked->use_fsmonitor bit being true, so we call
> refresh_fsmonitor, and if the bit is false, we do not have to bother
> with fsmonitor and no point calling refresh_fsmonitor"?
>
> If a careless caller makes a call to refresh_fsmonitor() when the
> configuration tells us not to use fsmonitor, wouldn't this cause us
> to use fsmonitor anyway?  Which sounds bad, so perhaps all callers
> are careful to first check if use_fsmonitor is set before deciding
> to call refresh_fsmonitor()---but if that is the case, is there a
> point in setting it here to true?
>
> Puzzled by an unexplained code...
>
> >       } else {
> > +
> > +             /* We only want to run the post index changed hook if we've actually changed entries, so keep track
> > +              * if we actually changed entries or not */
> > +             int is_cache_changed = 0;
> >               /* Mark all entries invalid */
> > -             for (i = 0; i < istate->cache_nr; i++)
> > -                     istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> > +             for (i = 0; i < istate->cache_nr; i++) {
> > +                     if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {
> > +                             is_cache_changed = 1;
> > +                             istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
> > +                     }
> > +             }
> >
> >               /* If we're going to check every file, ensure we save the results */
> > -             istate->cache_changed |= FSMONITOR_CHANGED;
> > +             if (is_cache_changed)
> > +                     istate->cache_changed |= FSMONITOR_CHANGED;
>
> This part (and a call to refresh_fsmonitor() we see blow) is the
> "Finally, we force a refresh" explained in the proposed log message,
> I presume.
>
> >               if (istate->untracked)
> >                       istate->untracked->use_fsmonitor = 0;
> > @@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate)
> >                       /* Mark all previously saved entries as dirty */
> >                       if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> >                               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> > -                                 (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> > +                                     (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
>
> This shares the same indentation issue but does not disable the
> sanity check for split index case.  Intended?  Without explanation
> in the proposed log message, readers cannot tell.
>
> >                       ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
> >
> > -                     /* Now mark the untracked cache for fsmonitor usage */
> > -                     if (istate->untracked)
> > -                             istate->untracked->use_fsmonitor = 1;
> > +                     refresh_fsmonitor(istate);
> >               }
> >
> >               ewah_free(istate->fsmonitor_dirty);
> > diff --git a/unpack-trees.c b/unpack-trees.c
> > index 33ea7810d8..fc5ceb932c 100644
> > --- a/unpack-trees.c
> > +++ b/unpack-trees.c
> > @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> >       o->merge_size = len;
> >       mark_all_ce_unused(o->src_index);
> >
> > +     if (o->src_index->fsmonitor_last_update)
> > +             o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;
> > +
>
> This is the "correctly copies" part, which was well explained.
>
> >       /*
> >        * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries
> >        */
> > @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,
> >
> >       if (old && same(old, a)) {
> >               int update = 0;
> > -             if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
> > +             if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
> > +                     !(old->ce_flags & CE_FSMONITOR_VALID)) {
>
> This is the "skip when we know it is valid" part, which was well
> explained.
>
> >                       struct stat st;
> >                       if (lstat(old->name, &st) ||
> >                           ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>
> Thanks.

Thanks for the feedback. The lines with the indentation changes had
tabs and spaces mixed up, but I'll revert those changes.