Re: [PATCH v2 5/5] rebase: use 'skip_cache_tree_update' option
From: Phillip Wood <hidden>
Date: 2022-11-10 14:40:30
Hi Victoria On 10/11/2022 01:57, Victoria Dye via GitGitGadget wrote:
From: Victoria Dye <redacted>
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
quoted hunk ↗ jump to hunk
Signed-off-by: Victoria Dye <redacted> --- 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
quoted hunk ↗ jump to hunk
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)) {