Re: [PATCH v2 5/5] rebase: use 'skip_cache_tree_update' option
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Nov 10, 2022, 14:40 UTC
- 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:
Show 11 quoted lines
> 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
Show 11 quoted lines
> 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
Show 15 quoted lines
> 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)) {