Re: [PATCH v4 09/15] diff --color-moved: call comparison function directly
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Nov 23, 2021, 15:09 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2111231605520.63@tvgsbejvaqbjf.bet>
- In-Reply-To
- <c3e5dce1910b3d640757e0845d646c3b040a8e28.1637056179.git.gitgitgadget@gmail.com>
Hi Phillip,
On Tue, 16 Nov 2021, Phillip Wood via GitGitGadget wrote:
Show 6 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk> > > This change will allow us to easily combine pmb_advance_or_null() and > pmb_advance_or_null_multi_match() in the next commit. Calling > xdiff_compare_lines() directly rather than using a function pointer > from the hash map has little effect on the run time.
Good. I verified that the function `moved_entry_cmp()` (https://github.com/gitgitgadget/git/blob/c3e5dce191/diff.c#L918-L944) calls `xdiff_compare_lines()`, and it is this function that is used for both `del_lines` and `add_lines` which would have been passed as `hm`: https://github.com/gitgitgadget/git/blob/c3e5dce191/diff.c#L6339-L6340
Show 9 quoted lines
> > Test HEAD^ HEAD > ------------------------------------------------------------------------------------------------------------- > 4002.1: diff --no-color-moved --no-color-moved-ws large change 0.38(0.35+0.03) 0.38(0.32+0.06) +0.0% > 4002.2: diff --color-moved --no-color-moved-ws large change 0.87(0.83+0.04) 0.87(0.80+0.06) +0.0% > 4002.3: diff --color-moved-ws=allow-indentation-change large change 0.97(0.92+0.04) 0.97(0.93+0.04) +0.0% > 4002.4: log --no-color-moved --no-color-moved-ws 1.17(1.06+0.10) 1.16(1.10+0.05) -0.9% > 4002.5: log --color-moved --no-color-moved-ws 1.32(1.24+0.08) 1.31(1.22+0.09) -0.8% > 4002.6: log --color-moved-ws=allow-indentation-change 1.36(1.25+0.10) 1.35(1.25+0.10) -0.7%
Honestly, I would have expected an improvement, given that `moved_entry_cmp()` has to do a few things before it can call `xdiff_compare_lines()`.
I love your attention to detail, providing performance numbers in the commit message to prove that it at least has no negative impact on the speed.
Thanks, Dscho
Show 47 quoted lines
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> diff.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 78a486021ab..22e0edac173 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -994,17 +994,20 @@ static void add_lines_to_move_detection(struct diff_options *o,
> }
>
> static void pmb_advance_or_null(struct diff_options *o,
> - struct moved_entry *match,
> - struct hashmap *hm,
> + struct emitted_diff_symbol *l,
> struct moved_block *pmb,
> int pmb_nr)
> {
> int i;
> + unsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;
> +
> for (i = 0; i < pmb_nr; i++) {
> struct moved_entry *prev = pmb[i].match;
> struct moved_entry *cur = (prev && prev->next_line) ?
> prev->next_line : NULL;
> - if (cur && !hm->cmpfn(o, &cur->ent, &match->ent, NULL)) {
> + if (cur && xdiff_compare_lines(cur->es->line, cur->es->len,
> + l->line, l->len,
> + flags)) {
> pmb[i].match = cur;
> } else {
> pmb[i].match = NULL;
> @@ -1195,7 +1198,7 @@ static void mark_color_as_moved(struct diff_options *o,
> COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)
> pmb_advance_or_null_multi_match(o, l, pmb, pmb_nr);
> else
> - pmb_advance_or_null(o, match, hm, pmb, pmb_nr);
> + pmb_advance_or_null(o, l, pmb, pmb_nr);
>
> pmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr);
>
> --
> gitgitgadget
>
>