On Wed, Nov 6, 2019 at 2:46 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Utsav Shah via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Utsav Shah <utsav@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> > 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.
