From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 10 Nov 2022 14:40:22 GMT
Subject: Re: [PATCH v2 5/5] rebase: use 'skip_cache_tree_update' option
Message-ID: <44b0331a-17e5-1528-2249-e89f0bdd6ffb@dunelm.org.uk>
In-Reply-To: <fffe2fc17ed3beb05376f1377ea193199c13c657.1668045438.git.gitgitgadget@gmail.com>

Hi Victoria

On 10/11/2022 01:57, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
> 
> Enable the 'skip_cache_tree_update' option in both 'do_reset()'
> ('sequencer.c') and 'reset_head()' ('reset.c'). Both of these callers invoke
> 'prime_cache_tree()' after 'unpack_trees()', so we can remove an unnecessary
> cache tree rebuild by skipping 'cache_tree_update()'.
> 
> When testing with 'p3400-rebase.sh' and 'p3404-rebase-interactive.sh', the
> performance change of this update was negligible, likely due to the
> operation being dominated by more expensive operations (like checking out
> trees).

Yes, we only call this once at the beginning of the rebase and then for 
any reset commands and the run time will be dominated by picking commits.

> However, since the change doesn't harm performance, it's worth
> keeping this 'unpack_trees()' usage consistent with others that subsequently
> invoke 'prime_cache_tree()'.

That makes sense

> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>   reset.c     | 1 +
>   sequencer.c | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/reset.c b/reset.c
> index e3383a93343..5ded23611f3 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -128,6 +128,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
	unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
>   	unpack_tree_opts.update = 1;
>   	unpack_tree_opts.merge = 1;
>   	unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
> +	unpack_tree_opts.skip_cache_tree_update = 1;

I've added an extra context line above to show that we do either a 
one-way or two-way merge - is it safe to skip the cache_tree_update for 
the two-way merge? (I'm afraid I seem to have forgotten everything I 
learnt about prime_cache_tree() and cache_tree_update() when we 
discussed this optimization before).

Best Wishes

Phillip

>   	init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
>   	if (reset_hard)
>   		unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
> diff --git a/sequencer.c b/sequencer.c
> index e658df7e8ff..3f7a73ce4e1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3750,6 +3750,7 @@ static int do_reset(struct repository *r,
>   	unpack_tree_opts.merge = 1;
>   	unpack_tree_opts.update = 1;
>   	unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
> +	unpack_tree_opts.skip_cache_tree_update = 1;
>   	init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);
>   
>   	if (repo_read_index_unmerged(r)) {