git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Nov 6, 2019, 10:46 UTC
Message-ID
<xmqqftj1th93.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<4bea7075cfcac013f5947cd3e9254d38e86e675c.1573016055.git.gitgitgadget@gmail.com>
"Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 5 quoted lines
> 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.
Show 21 quoted lines
> 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?
Show 11 quoted lines
> @@ -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.
Show 10 quoted lines
> @@ -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).

Show 7 quoted lines
> @@ -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...
Show 19 quoted lines
>  	} 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.

Show 8 quoted lines
>  		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.

Show 20 quoted lines
>  			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.
Show 10 quoted lines
>  	/*
>  	 * 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.
Previous: Utsav Shah via GitGitGadgetNext: Utsav Shah
Message 19 of 26 in “unpack-trees: skip lstat on files based on fsmonitor”
  1. 0/1 unpack-trees: skip lstat on files based on fsmonitorUtsav Shah via GitGitGadget, Oct 25, 2019
  2. 1/1 unpack-trees: skip lstat based on fsmonitorUtsav Shah via GitGitGadget, Oct 25, 2019
  3. Junio C HamanoOct 28, 2019
  4. Utsav ShahOct 28, 2019
  5. Kevin WillfordOct 28, 2019
  6. Utsav ShahOct 29, 2019
  7. Kevin WillfordOct 29, 2019
  8. Utsav ShahOct 29, 2019
  9. Junio C HamanoOct 30, 2019
  10. Utsav ShahOct 30, 2019
  11. Junio C HamanoNov 4, 2019
  12. 0/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 5, 2019
  13. 1/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 5, 2019
  14. Kevin WillfordNov 5, 2019
  15. Utsav ShahNov 6, 2019
  16. Kevin WillfordNov 6, 2019
  17. 0/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 6, 2019
  18. 1/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 6, 2019
  19. Junio C HamanoNov 6, 2019
  20. Utsav ShahNov 6, 2019
  21. Utsav ShahNov 8, 2019
  22. Junio C HamanoNov 8, 2019
  23. Junio C HamanoNov 6, 2019
  24. 0/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 20, 2019
  25. 1/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 20, 2019
  26. Junio C HamanoNov 21, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.