Re: [PATCH v2] stash: reuse cached index entries in --patch temporary index

[Junio C Hamano <gitster@pobox.com>]

"Adam Johnson via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Adam Johnson <me@adamj.eu>
>
> `git stash -p` prepares the interactive selection by creating a
> temporary index at HEAD, switching `GIT_INDEX_FILE` to it, and then
> running the `add -p` machinery.
>
> That temporary index was created by running `git read-tree HEAD`.  The
> resulting index had no useful cached stat data or fsmonitor-valid bits
> from the real index.  When `run_add_p()` refreshed that temporary index
> before showing the first prompt, it could end up lstat(2)-ing every
> tracked file, even in a repository where `git diff` and `git restore -p`
> can use fsmonitor to avoid that work.
>
> Create the temporary index in-process instead.  Use `unpack_trees()` to
> reset the real index contents to HEAD while writing the result to the
> temporary index path.  For paths whose index entries already match HEAD,
> `oneway_merge()` reuses the existing cache entries, preserving their
> cached stat data and `CE_FSMONITOR_VALID` state.
>
> This makes the refresh performed by `run_add_p()` behave like the one
> used by `git restore -p`: unchanged paths can be skipped via fsmonitor
> instead of being scanned again.
>
> In a 206k file repository with `core.fsmonitor` enabled and a one-line
> change in one file, time to first prompt dropped from 34.774 seconds to
> 0.659 seconds. The new perf test file demonstrates similar improvements,
> with maen times for without- and with-fsmonitor cases dropping from 6.90
> and 6.83 seconds to 0.55 and 0.28 seconds, respectively.
>
> Signed-off-by: Adam Johnson <me@adamj.eu>
> ---
>     stash: reuse cached index entries in --patch temporary index
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2306%2Fadamchainz%2Faj%2Foptimize-stash-patch-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2306/adamchainz/aj/optimize-stash-patch-v2
> Pull-Request: https://github.com/git/git/pull/2306

The diff relative to the previous round looked good.  I am not a
"stash -p" user myself, but I suspect that there are people who
heavily use it, so I'd feel safer if an extra set of eye looks at
the patch and gives an Ack, but other than that I have no comments
on the patch.  Looking good.

Thanks.


>  builtin/stash.c             | 70 +++++++++++++++++++++++++++++++++----
>  t/perf/p3904-stash-patch.sh | 43 +++++++++++++++++++++++
>  2 files changed, 107 insertions(+), 6 deletions(-)
>  create mode 100755 t/perf/p3904-stash-patch.sh
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 32dbc97b47..c4809f299a 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -372,6 +372,56 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
>  	return 0;
>  }
>  
> +static int create_index_from_tree(const struct object_id *tree_id,
> +				  const char *index_path)
> +{
> +	int nr_trees = 1;
> +	int ret = 0;
> +	struct unpack_trees_options opts;
> +	struct tree_desc t[MAX_UNPACK_TREES];
> +	struct tree *tree;
> +	struct index_state dst_istate = INDEX_STATE_INIT(the_repository);
> +	struct lock_file lock_file = LOCK_INIT;
> +
> +	repo_read_index_preload(the_repository, NULL, 0);
> +	refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
> +
> +	hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
> +
> +	memset(&opts, 0, sizeof(opts));
> +
> +	tree = repo_parse_tree_indirect(the_repository, tree_id);
> +	if (!tree || repo_parse_tree(the_repository, tree)) {
> +		ret = -1;
> +		goto done;
> +	}
> +
> +	init_tree_desc(t, &tree->object.oid, tree->buffer, tree->size);
> +
> +	opts.head_idx = 1;
> +	opts.src_index = the_repository->index;
> +	opts.dst_index = &dst_istate;
> +	opts.merge = 1;
> +	opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
> +	opts.fn = oneway_merge;
> +
> +	if (unpack_trees(nr_trees, t, &opts)) {
> +		ret = -1;
> +		goto done;
> +	}
> +
> +	if (write_locked_index(&dst_istate, &lock_file, COMMIT_LOCK)) {
> +		ret = error(_("unable to write new index file"));
> +		goto done;
> +	}
> +
> +done:
> +	release_index(&dst_istate);
> +	if (ret)
> +		rollback_lock_file(&lock_file);
> +	return ret;
> +}
> +
>  static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
> @@ -1321,18 +1371,26 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
>  		       struct interactive_options *interactive_opts)
>  {
>  	int ret = 0;
> -	struct child_process cp_read_tree = CHILD_PROCESS_INIT;
>  	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
> +	struct commit *head_commit;
> +	const struct object_id *head_tree;
>  	struct index_state istate = INDEX_STATE_INIT(the_repository);
>  	char *old_index_env = NULL, *old_repo_index_file;
>  
>  	remove_path(stash_index_path.buf);
>  
> -	cp_read_tree.git_cmd = 1;
> -	strvec_pushl(&cp_read_tree.args, "read-tree", "HEAD", NULL);
> -	strvec_pushf(&cp_read_tree.env, "GIT_INDEX_FILE=%s",
> -		     stash_index_path.buf);
> -	if (run_command(&cp_read_tree)) {
> +	head_commit = lookup_commit(the_repository, &info->b_commit);
> +	if (!head_commit || repo_parse_commit(the_repository, head_commit)) {
> +		ret = -1;
> +		goto done;
> +	}
> +	head_tree = get_commit_tree_oid(head_commit);
> +	if (!head_tree) {
> +		ret = -1;
> +		goto done;
> +	}
> +
> +	if (create_index_from_tree(head_tree, stash_index_path.buf)) {
>  		ret = -1;
>  		goto done;
>  	}
> diff --git a/t/perf/p3904-stash-patch.sh b/t/perf/p3904-stash-patch.sh
> new file mode 100755
> index 0000000000..4cfce638be
> --- /dev/null
> +++ b/t/perf/p3904-stash-patch.sh
> @@ -0,0 +1,43 @@
> +#!/bin/sh
> +
> +test_description="Performance tests for git stash -p"
> +
> +. ./perf-lib.sh
> +
> +test_perf_fresh_repo
> +
> +test_expect_success "setup" '
> +	mkdir files &&
> +	test_seq 1 100000 | while read i; do
> +		echo "content $i" >files/$i.txt || return 1
> +	done &&
> +	git add files/ &&
> +	git commit -q -m "add tracked files" &&
> +	echo modified >files/1.txt
> +'
> +
> +test_perf "stash -p, no fsmonitor" \
> +	--setup 'echo modified >files/1.txt' '
> +	printf "q\n" | git stash -p >/dev/null 2>&1 || true
> +'
> +
> +if test_have_prereq FSMONITOR_DAEMON
> +then
> +	test_expect_success "enable builtin fsmonitor" '
> +		git config core.fsmonitor true &&
> +		git fsmonitor--daemon start &&
> +		git update-index --fsmonitor &&
> +		git status >/dev/null 2>&1
> +	'
> +
> +	test_perf "stash -p, builtin fsmonitor" \
> +		--setup 'echo modified >files/1.txt && git status >/dev/null 2>&1' '
> +		printf "q\n" | git stash -p >/dev/null 2>&1 || true
> +	'
> +
> +	test_expect_success "stop builtin fsmonitor" '
> +		git fsmonitor--daemon stop
> +	'
> +fi
> +
> +test_done
>
> base-commit: 7bcaabddcf68bd0702697da5904c3b68c52f94cf